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 <jbeul...@suse.com>
>> ---
>> 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
