[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH RFC] lib: extend ASSERT()
On 05.01.2021 14:56, Andrew Cooper wrote: > On 05/01/2021 12:45, Jan Beulich wrote: >> The increasing amount of constructs along the lines of >> >> if ( !condition ) >> { >> ASSERT_UNREACHABLE(); >> return; >> } >> >> is not only longer than necessary, but also doesn't produce incident >> specific console output (except for file name and line number). Allow >> the intended effect to be achieved with ASSERT(), by giving it a second >> parameter allowing specification of the action to take in release builds >> in case an assertion would have triggered in a debug one. The example >> above then becomes >> >> ASSERT(condition, return); >> >> Make sure the condition will continue to not get evaluated when just a >> single argument gets passed to ASSERT(). >> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> >> --- >> RFC: The use of a control flow construct as a macro argument may be >> controversial. > > So I had been putting some consideration towards this. I agree that the > current use of ASSERT_UNREACHABLE() isn't great, and that we ought to do > something to improve the status quo. > > However, the more interesting constructs to consider are the ones with > printk()'s and/or domain_crash(). While a straight return or goto in > alt... is perhaps acceptable, anything more complicated probably isn't. Since syntactically this is no problem, it is up to us whether we consider this unacceptable. Personally I wouldn't mind as long as the set of statements doesn't get excessive. Otoh I'm not sure the more complex uses are in need of such a transformation - the relatively simple ones are where the current arrangement has most significant impact. > I also found, with my still-pending domain_crash() cleanup series, that > domain_crash()/ASSERT_UNREACHABE()/return/goto became an increasingly > common combination. Which may call for further abstraction along the lines of what I'm doing here? >> --- a/xen/include/xen/lib.h >> +++ b/xen/include/xen/lib.h >> @@ -55,12 +55,14 @@ >> #endif >> >> #ifndef NDEBUG >> -#define ASSERT(p) \ >> +#define ASSERT(p, ...) \ >> do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0) >> #define ASSERT_UNREACHABLE() assert_failed("unreachable") >> #define debug_build() 1 >> #else >> -#define ASSERT(p) do { if ( 0 && (p) ) {} } while (0) >> +#define ASSERT(p, alt...) do { \ >> + if ( !count_args(alt) || unlikely(!(p)) ) { alt; } \ > > I'd strongly recommend naming this failsafe... rather than alt, to make > it clear what its purpose is. No problem. Albeit I wonder whether "failsafe" is really better - maybe "fallback"? > Also, we really can't have (p) conditionally evaluated depending on > whether there are any failsafe arguments or not. It is already bad > enough that its state of evaluation differs between debug and release > builds. Are you suggesting to evaluate it unconditionally, producing unnecessary binary code in release builds _and_ diverging from the C standard's assert()? I'm not outright against, but I'm also not sure this is a good idea. But like you I also don't really like the new asymmetry, but I thought it would be best to leave existing uses of ASSERT() entirely unchanged in their behavior. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |