Re: [PATCH RFC] lib: extend ASSERT()

[Jan Beulich <jbeulich@xxxxxxxx>]

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