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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan, Julien,

> On 15 Feb 2022, at 21:00, Julien Grall <julien@xxxxxxx> wrote:
> 
> (+ Bertrand)
> 
> Hi Jan,
> 
> On 27/01/2022 14:34, 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).
> 
> So I agree that this construct will always result to a minimum 5 lines. Which 
> is not nice. But the proposed change is...
> 
>> 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>
>> ---
>> v2: Rename new macro parameter.
>> ---
>> RFC: The use of a control flow construct as a macro argument may be
>>      controversial.
> 
> indeed controversial. I find this quite confusing and not something I would 
> request a user to switch to if they use the longer version.
> 
> That said, this is mainly a matter of taste. So I am interested to hear 
> others view.
> 
> I have also CCed Bertrand to have an opinions from the Fusa Group (I suspect 
> this will go backward for them).

Thanks and here is my feedback in regards to Fusa here.

Most certification standards are forbidding completely macros including
conditions (and quite a number are forbidding static inline with conditions).
The main reason for that is MCDC coverage (condition/decisions and not only
code line) is not possible to do anymore down to the source code and has to be
done down to the pre-processed code.

Out of Fusa considerations, one thing I do not like in this solution is the 
fact that
you put some code as parameter of the macro (the return).

To make this a bit better you could put the return code as parameter
instead of having “return CODE” as parameter.

An other thing is that Xen ASSERT after this change will be quite different from
any ASSERT found in other projects which could make it misleading for 
developers.
Maybe we could introduce an ASSERT_RETURN macros instead of modifying the
behaviour of the standard ASSERT ?

Regards
Bertrand

> 
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -826,11 +826,7 @@ int xenmem_add_to_physmap(struct domain
>>      union add_to_physmap_extra extra = {};
>>      struct page_info *pages[16];
>>  -    if ( !paging_mode_translate(d) )
>> -    {
>> -        ASSERT_UNREACHABLE();
>> -        return -EACCES;
>> -    }
>> +    ASSERT(paging_mode_translate(d), return -EACCES);
>>        if ( xatp->space == XENMAPSPACE_gmfn_foreign )
>>          extra.foreign_domid = DOMID_INVALID;
>> @@ -920,11 +916,7 @@ static int xenmem_add_to_physmap_batch(s
>>       * call doesn't succumb to dead-code-elimination. Duplicate the 
>> short-circut
>>       * from xatp_permission_check() to try and help the compiler out.
>>       */
>> -    if ( !paging_mode_translate(d) )
>> -    {
>> -        ASSERT_UNREACHABLE();
>> -        return -EACCES;
>> -    }
>> +    ASSERT(paging_mode_translate(d), return -EACCES);
>>        if ( unlikely(xatpb->size < extent) )
>>          return -EILSEQ;
>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -49,11 +49,13 @@
>>  #endif
>>    #ifndef NDEBUG
>> -#define ASSERT(p) \
>> +#define ASSERT(p, ...) \
>>      do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
>>  #define ASSERT_UNREACHABLE() assert_failed("unreachable")
>>  #else
>> -#define ASSERT(p) do { if ( 0 && (p) ) {} } while (0)
>> +#define ASSERT(p, failsafe...) do { \
>> +        if ( !count_args(failsafe) || unlikely(!(p)) ) { failsafe; } \
>> +    } while ( 0 )
>>  #define ASSERT_UNREACHABLE() do { } while (0)
>>  #endif
>>  
> 
> Cheers,
> 
> -- 
> Julien Grall