[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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.