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

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On Feb 16, 2022, at 9:31 AM, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 16.02.2022 10:25, Bertrand Marquis wrote:
>> 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.
> 
> Except that it's not always "return" what you want, and hence it
> can't be made implicit.
> 
>> 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 ?
> 
> Along the lines of the above, this would then mean a multitude of
> new macros.

Out of curiosity, what kinds of other actions?

I am opposed to overloading “ASSERT” for this new kind of macro; I think it 
would not only be unnecessarily confusing to people not familiar with our 
codebase, but it would be too easy for people to fail to notice which macro was 
being used.

ASSERT_ACTION(condition, code) (or even ASSERT_OR_ACTION()) would be a bare 
minimum for me.

But I can’t imagine that there are more than a handful of actions we might want 
to take, so defining a macro for each one shouldn’t be too burdensome.

Furthermore, the very flexibility seems dangerous; you’re not seeing what 
actual code is generated, so it’s to easy to be “clever”, and/or write code 
that ends up doing something different than you expect.

At the moment I think ASSERT_OR_RETURN(condition, code), plus other new macros 
for the other behavior is needed, would be better.

 -George

Attachment: signature.asc
Description: Message signed with OpenPGP