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@suse.com>
---
v2: Rename new macro parameter.
---
RFC: The use of a control flow construct as a macro argument may be
     controversial.

--- 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
 

(+ 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@suse.com>
> ---
> 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).

> 
> --- 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

Hi Jan, Julien,

> On 15 Feb 2022, at 21:00, Julien Grall <julien@xen.org> 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@suse.com>
>> ---
>> 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

On 16.02.2022 10:25, Bertrand Marquis wrote:
> Hi Jan, Julien,
> 
>> On 15 Feb 2022, at 21:00, Julien Grall <julien@xen.org> 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@suse.com>
>>> ---
>>> 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.

Jan

> On Feb 16, 2022, at 9:31 AM, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 16.02.2022 10:25, Bertrand Marquis wrote:
>> Hi Jan, Julien,
>> 
>>> On 15 Feb 2022, at 21:00, Julien Grall <julien@xen.org> 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@suse.com>
>>>> ---
>>>> 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

On 16.02.2022 12:34, George Dunlap wrote:
>> On Feb 16, 2022, at 9:31 AM, Jan Beulich <jbeulich@suse.com> wrote:
>> On 16.02.2022 10:25, Bertrand Marquis wrote:
>>>> On 15 Feb 2022, at 21:00, Julien Grall <julien@xen.org> wrote:
>>>> 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@suse.com>
>>>>> ---
>>>>> 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?

continue, break, assignments of e.g. error codes, just to name a
few immediately obvious ones.

> 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.

Hmm, while I see your point of things possibly looking confusing or
unexpected, something like ASSERT_OR_RETURN() (shouldn't it be
ASSERT_AND_RETURN()?) is imo less readable. In particular I dislike
the larger amount of uppercase text. But yes, I could accept this
as a compromise as it still seems better to me than the multi-line
constructs we currently use.

Jan

> On Feb 16, 2022, at 11:42 AM, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 16.02.2022 12:34, George Dunlap wrote:
> 
>> 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.
> 
> Hmm, while I see your point of things possibly looking confusing or
> unexpected, something like ASSERT_OR_RETURN() (shouldn't it be
> ASSERT_AND_RETURN()?) is imo less readable. In particular I dislike
> the larger amount of uppercase text. But yes, I could accept this
> as a compromise as it still seems better to me than the multi-line
> constructs we currently use.

I see what you’re saying with AND/OR; I personally still prefer OR but wouldn’t argue to hard against AND if others preferred it.

As far as I’m concerned, the fact that we’re reducing lines of code isn’t a reason to use this at all.  As our CODING_STYLE says, ASSERT() is just a louder printk.  We would never consider writing PRINTK_AND_RETURN(), and we would never consider writing a macro like CONDRET(condition, retval) to replace

if (condition)
    return retval;

The only justification for this kind of macro, in my opinion, is to avoid duplication errors; i.e. replacing your code segment with the following:

if (condition) {
    ASSERT(!condition);
    return foo;
}

is undesirable because there’s too much risk that the conditions will drift or be inverted incorrectly. But having control statements like ‘return’ and ‘continue’ in a macro is also undesirable in my opinion; I’m personally not sure which I find most undesirable.

I guess one advantage of something like ASSERT_OR(condition, return foo); or ASSERT_OR(condition, continue); is that searching for “return” or “continue” will come up even if you’re doing a case-sensitive search.  But I’m still wary of unintended side effects.

Bertrand / Julien, any more thoughts?

 -George

Hi,

> On 16 Feb 2022, at 12:23, George Dunlap <George.Dunlap@citrix.com> wrote:
> 
> 
> 
>> On Feb 16, 2022, at 11:42 AM, Jan Beulich <jbeulich@suse.com> wrote:
>> 
>> On 16.02.2022 12:34, George Dunlap wrote:
>> 
>>> 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.
>> 
>> Hmm, while I see your point of things possibly looking confusing or
>> unexpected, something like ASSERT_OR_RETURN() (shouldn't it be
>> ASSERT_AND_RETURN()?) is imo less readable. In particular I dislike
>> the larger amount of uppercase text. But yes, I could accept this
>> as a compromise as it still seems better to me than the multi-line
>> constructs we currently use.
> 
> I see what you’re saying with AND/OR; I personally still prefer OR but wouldn’t argue to hard against AND if others preferred it.
> 
> As far as I’m concerned, the fact that we’re reducing lines of code isn’t a reason to use this at all.  As our CODING_STYLE says, ASSERT() is just a louder printk.  We would never consider writing PRINTK_AND_RETURN(), and we would never consider writing a macro like CONDRET(condition, retval) to replace
> 
> if (condition)
>    return retval;
> 
> The only justification for this kind of macro, in my opinion, is to avoid duplication errors; i.e. replacing your code segment with the following:
> 
> if (condition) {
>    ASSERT(!condition);
>    return foo;
> }
> 
> is undesirable because there’s too much risk that the conditions will drift or be inverted incorrectly. But having control statements like ‘return’ and ‘continue’ in a macro is also undesirable in my opinion; I’m personally not sure which I find most undesirable.
> 
> I guess one advantage of something like ASSERT_OR(condition, return foo); or ASSERT_OR(condition, continue); is that searching for “return” or “continue” will come up even if you’re doing a case-sensitive search.  But I’m still wary of unintended side effects.
> 
> Bertrand / Julien, any more thoughts?

I think that having macros which are magic like that one which includes a possible return and the fact that the macro is taking code as argument is making the code really hard to read and understand for someone not knowing this.
Even the code is longer right now, it is more readable and easy to understand which means less chance for errors so I do not think the macro will avoid errors but might in fact introduce some in the future.

So I am voting to keep the current macro as it is.

Bertrand

> 
> -George

On 16.02.2022 14:57, Bertrand Marquis wrote:
>> On 16 Feb 2022, at 12:23, George Dunlap <George.Dunlap@citrix.com> wrote:
>>> On Feb 16, 2022, at 11:42 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 16.02.2022 12:34, George Dunlap wrote:
>>>> 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.
>>>
>>> Hmm, while I see your point of things possibly looking confusing or
>>> unexpected, something like ASSERT_OR_RETURN() (shouldn't it be
>>> ASSERT_AND_RETURN()?) is imo less readable. In particular I dislike
>>> the larger amount of uppercase text. But yes, I could accept this
>>> as a compromise as it still seems better to me than the multi-line
>>> constructs we currently use.
>>
>> I see what you’re saying with AND/OR; I personally still prefer OR but wouldn’t argue to hard against AND if others preferred it.
>>
>> As far as I’m concerned, the fact that we’re reducing lines of code isn’t a reason to use this at all.  As our CODING_STYLE says, ASSERT() is just a louder printk.  We would never consider writing PRINTK_AND_RETURN(), and we would never consider writing a macro like CONDRET(condition, retval) to replace
>>
>> if (condition)
>>    return retval;
>>
>> The only justification for this kind of macro, in my opinion, is to avoid duplication errors; i.e. replacing your code segment with the following:
>>
>> if (condition) {
>>    ASSERT(!condition);
>>    return foo;
>> }
>>
>> is undesirable because there’s too much risk that the conditions will drift or be inverted incorrectly. But having control statements like ‘return’ and ‘continue’ in a macro is also undesirable in my opinion; I’m personally not sure which I find most undesirable.
>>
>> I guess one advantage of something like ASSERT_OR(condition, return foo); or ASSERT_OR(condition, continue); is that searching for “return” or “continue” will come up even if you’re doing a case-sensitive search.  But I’m still wary of unintended side effects.
>>
>> Bertrand / Julien, any more thoughts?
> 
> I think that having macros which are magic like that one which includes a possible return and the fact that the macro is taking code as argument is making the code really hard to read and understand for someone not knowing this.
> Even the code is longer right now, it is more readable and easy to understand which means less chance for errors so I do not think the macro will avoid errors but might in fact introduce some in the future.
> 
> So I am voting to keep the current macro as it is.

But you recall that there were two aspects to me wanting the switch?
(Source) code size was only one. The other was that ASSERT_UNREACHABLE()
doesn't show the original expression which has triggered the failure,
unlike ASSERT() does.

Jan

Hi Jan,

> On 16 Feb 2022, at 14:03, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 16.02.2022 14:57, Bertrand Marquis wrote:
>>> On 16 Feb 2022, at 12:23, George Dunlap <George.Dunlap@citrix.com> wrote:
>>>> On Feb 16, 2022, at 11:42 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 16.02.2022 12:34, George Dunlap wrote:
>>>>> 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.
>>>> 
>>>> Hmm, while I see your point of things possibly looking confusing or
>>>> unexpected, something like ASSERT_OR_RETURN() (shouldn't it be
>>>> ASSERT_AND_RETURN()?) is imo less readable. In particular I dislike
>>>> the larger amount of uppercase text. But yes, I could accept this
>>>> as a compromise as it still seems better to me than the multi-line
>>>> constructs we currently use.
>>> 
>>> I see what you’re saying with AND/OR; I personally still prefer OR but wouldn’t argue to hard against AND if others preferred it.
>>> 
>>> As far as I’m concerned, the fact that we’re reducing lines of code isn’t a reason to use this at all.  As our CODING_STYLE says, ASSERT() is just a louder printk.  We would never consider writing PRINTK_AND_RETURN(), and we would never consider writing a macro like CONDRET(condition, retval) to replace
>>> 
>>> if (condition)
>>>   return retval;
>>> 
>>> The only justification for this kind of macro, in my opinion, is to avoid duplication errors; i.e. replacing your code segment with the following:
>>> 
>>> if (condition) {
>>>   ASSERT(!condition);
>>>   return foo;
>>> }
>>> 
>>> is undesirable because there’s too much risk that the conditions will drift or be inverted incorrectly. But having control statements like ‘return’ and ‘continue’ in a macro is also undesirable in my opinion; I’m personally not sure which I find most undesirable.
>>> 
>>> I guess one advantage of something like ASSERT_OR(condition, return foo); or ASSERT_OR(condition, continue); is that searching for “return” or “continue” will come up even if you’re doing a case-sensitive search.  But I’m still wary of unintended side effects.
>>> 
>>> Bertrand / Julien, any more thoughts?
>> 
>> I think that having macros which are magic like that one which includes a possible return and the fact that the macro is taking code as argument is making the code really hard to read and understand for someone not knowing this.
>> Even the code is longer right now, it is more readable and easy to understand which means less chance for errors so I do not think the macro will avoid errors but might in fact introduce some in the future.
>> 
>> So I am voting to keep the current macro as it is.
> 
> But you recall that there were two aspects to me wanting the switch?
> (Source) code size was only one. The other was that ASSERT_UNREACHABLE()
> doesn't show the original expression which has triggered the failure,
> unlike ASSERT() does.

Sorry I focused on the macro part after Julien asked me to comment from the Fusa point of view.

The usual expectation is that an ASSERT should never occur and is an help for the programmer to detect programming errors. Usually an assert is crashing the application with an information of where an assert was triggered.
In the current case, the assert is used as debug print and in production mode an error is returned if this is happening without any print. Isn’t this a debug print case instead of an assert ?

Bertrand

> 
> Jan
> 

On 16.02.2022 15:35, Bertrand Marquis wrote:
> Hi Jan,
> 
>> On 16 Feb 2022, at 14:03, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 16.02.2022 14:57, Bertrand Marquis wrote:
>>>> On 16 Feb 2022, at 12:23, George Dunlap <George.Dunlap@citrix.com> wrote:
>>>>> On Feb 16, 2022, at 11:42 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 16.02.2022 12:34, George Dunlap wrote:
>>>>>> 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.
>>>>>
>>>>> Hmm, while I see your point of things possibly looking confusing or
>>>>> unexpected, something like ASSERT_OR_RETURN() (shouldn't it be
>>>>> ASSERT_AND_RETURN()?) is imo less readable. In particular I dislike
>>>>> the larger amount of uppercase text. But yes, I could accept this
>>>>> as a compromise as it still seems better to me than the multi-line
>>>>> constructs we currently use.
>>>>
>>>> I see what you’re saying with AND/OR; I personally still prefer OR but wouldn’t argue to hard against AND if others preferred it.
>>>>
>>>> As far as I’m concerned, the fact that we’re reducing lines of code isn’t a reason to use this at all.  As our CODING_STYLE says, ASSERT() is just a louder printk.  We would never consider writing PRINTK_AND_RETURN(), and we would never consider writing a macro like CONDRET(condition, retval) to replace
>>>>
>>>> if (condition)
>>>>   return retval;
>>>>
>>>> The only justification for this kind of macro, in my opinion, is to avoid duplication errors; i.e. replacing your code segment with the following:
>>>>
>>>> if (condition) {
>>>>   ASSERT(!condition);
>>>>   return foo;
>>>> }
>>>>
>>>> is undesirable because there’s too much risk that the conditions will drift or be inverted incorrectly. But having control statements like ‘return’ and ‘continue’ in a macro is also undesirable in my opinion; I’m personally not sure which I find most undesirable.
>>>>
>>>> I guess one advantage of something like ASSERT_OR(condition, return foo); or ASSERT_OR(condition, continue); is that searching for “return” or “continue” will come up even if you’re doing a case-sensitive search.  But I’m still wary of unintended side effects.
>>>>
>>>> Bertrand / Julien, any more thoughts?
>>>
>>> I think that having macros which are magic like that one which includes a possible return and the fact that the macro is taking code as argument is making the code really hard to read and understand for someone not knowing this.
>>> Even the code is longer right now, it is more readable and easy to understand which means less chance for errors so I do not think the macro will avoid errors but might in fact introduce some in the future.
>>>
>>> So I am voting to keep the current macro as it is.
>>
>> But you recall that there were two aspects to me wanting the switch?
>> (Source) code size was only one. The other was that ASSERT_UNREACHABLE()
>> doesn't show the original expression which has triggered the failure,
>> unlike ASSERT() does.
> 
> Sorry I focused on the macro part after Julien asked me to comment from the Fusa point of view.
> 
> The usual expectation is that an ASSERT should never occur and is an help for the programmer to detect programming errors. Usually an assert is crashing the application with an information of where an assert was triggered.
> In the current case, the assert is used as debug print and in production mode an error is returned if this is happening without any print. Isn’t this a debug print case instead of an assert ?

Depends on the terminology you want to use: Our ASSERT() is in no way
different in this regard from the C standard's assert(). The message
logged is of course to aid the developers. But personally I'd call it
more than just a "debug print".

Jan

Hi Jan,

> On 16 Feb 2022, at 14:43, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 16.02.2022 15:35, Bertrand Marquis wrote:
>> Hi Jan,
>> 
>>> On 16 Feb 2022, at 14:03, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 16.02.2022 14:57, Bertrand Marquis wrote:
>>>>> On 16 Feb 2022, at 12:23, George Dunlap <George.Dunlap@citrix.com> wrote:
>>>>>> On Feb 16, 2022, at 11:42 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>> On 16.02.2022 12:34, George Dunlap wrote:
>>>>>>> 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.
>>>>>> 
>>>>>> Hmm, while I see your point of things possibly looking confusing or
>>>>>> unexpected, something like ASSERT_OR_RETURN() (shouldn't it be
>>>>>> ASSERT_AND_RETURN()?) is imo less readable. In particular I dislike
>>>>>> the larger amount of uppercase text. But yes, I could accept this
>>>>>> as a compromise as it still seems better to me than the multi-line
>>>>>> constructs we currently use.
>>>>> 
>>>>> I see what you’re saying with AND/OR; I personally still prefer OR but wouldn’t argue to hard against AND if others preferred it.
>>>>> 
>>>>> As far as I’m concerned, the fact that we’re reducing lines of code isn’t a reason to use this at all.  As our CODING_STYLE says, ASSERT() is just a louder printk.  We would never consider writing PRINTK_AND_RETURN(), and we would never consider writing a macro like CONDRET(condition, retval) to replace
>>>>> 
>>>>> if (condition)
>>>>>  return retval;
>>>>> 
>>>>> The only justification for this kind of macro, in my opinion, is to avoid duplication errors; i.e. replacing your code segment with the following:
>>>>> 
>>>>> if (condition) {
>>>>>  ASSERT(!condition);
>>>>>  return foo;
>>>>> }
>>>>> 
>>>>> is undesirable because there’s too much risk that the conditions will drift or be inverted incorrectly. But having control statements like ‘return’ and ‘continue’ in a macro is also undesirable in my opinion; I’m personally not sure which I find most undesirable.
>>>>> 
>>>>> I guess one advantage of something like ASSERT_OR(condition, return foo); or ASSERT_OR(condition, continue); is that searching for “return” or “continue” will come up even if you’re doing a case-sensitive search.  But I’m still wary of unintended side effects.
>>>>> 
>>>>> Bertrand / Julien, any more thoughts?
>>>> 
>>>> I think that having macros which are magic like that one which includes a possible return and the fact that the macro is taking code as argument is making the code really hard to read and understand for someone not knowing this.
>>>> Even the code is longer right now, it is more readable and easy to understand which means less chance for errors so I do not think the macro will avoid errors but might in fact introduce some in the future.
>>>> 
>>>> So I am voting to keep the current macro as it is.
>>> 
>>> But you recall that there were two aspects to me wanting the switch?
>>> (Source) code size was only one. The other was that ASSERT_UNREACHABLE()
>>> doesn't show the original expression which has triggered the failure,
>>> unlike ASSERT() does.
>> 
>> Sorry I focused on the macro part after Julien asked me to comment from the Fusa point of view.
>> 
>> The usual expectation is that an ASSERT should never occur and is an help for the programmer to detect programming errors. Usually an assert is crashing the application with an information of where an assert was triggered.
>> In the current case, the assert is used as debug print and in production mode an error is returned if this is happening without any print. Isn’t this a debug print case instead of an assert ?
> 
> Depends on the terminology you want to use: Our ASSERT() is in no way
> different in this regard from the C standard's assert(). The message
> logged is of course to aid the developers. But personally I'd call it
> more than just a "debug print".

But it will be if we change it. But I agree with you it is more than a debug print.

Bertrand

> 
> Jan

Hi George,

Sorry for the late answer.

On 16/02/2022 12:23, George Dunlap wrote:
> 
> 
>> On Feb 16, 2022, at 11:42 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 16.02.2022 12:34, George Dunlap wrote:
>>
>>> 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.
>>
>> Hmm, while I see your point of things possibly looking confusing or
>> unexpected, something like ASSERT_OR_RETURN() (shouldn't it be
>> ASSERT_AND_RETURN()?) is imo less readable. In particular I dislike
>> the larger amount of uppercase text. But yes, I could accept this
>> as a compromise as it still seems better to me than the multi-line
>> constructs we currently use.
> 
> I see what you’re saying with AND/OR; I personally still prefer OR but wouldn’t argue to hard against AND if others preferred it.
> 
> As far as I’m concerned, the fact that we’re reducing lines of code isn’t a reason to use this at all.  As our CODING_STYLE says, ASSERT() is just a louder printk.  We would never consider writing PRINTK_AND_RETURN(), and we would never consider writing a macro like CONDRET(condition, retval) to replace
> 
> if (condition)
>      return retval;
> 
> The only justification for this kind of macro, in my opinion, is to avoid duplication errors; i.e. replacing your code segment with the following:
> 
> if (condition) {
>      ASSERT(!condition);
>      return foo;
> }
> 
> is undesirable because there’s too much risk that the conditions will drift or be inverted incorrectly. But having control statements like ‘return’ and ‘continue’ in a macro is also undesirable in my opinion; I’m personally not sure which I find most undesirable.
> 
> I guess one advantage of something like ASSERT_OR(condition, return foo); or ASSERT_OR(condition, continue); is that searching for “return” or “continue” will come up even if you’re doing a case-sensitive search.  But I’m still wary of unintended side effects.
> 
> Bertrand / Julien, any more thoughts?
The discussion reminds me WARN_ONCE() in Linux. The macro returns a 
value so it can be used like:

if (WARN_ONCE(...))
   return error;

How about introducing a new macro that would return whether the check 
passed and if the check failed crashed in debug build?

I am not suggesting to modify ASSERT() because the compiler may decide 
to not ellide check in production build. Also, the name feels a little 
bit odd.

Cheers,

-- 
Julien Grall

Hi Jan,

> On 16 Feb 2022, at 09:31, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 16.02.2022 10:25, Bertrand Marquis wrote:
>> Hi Jan, Julien,
>> 
>>> On 15 Feb 2022, at 21:00, Julien Grall <julien@xen.org> 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@suse.com>
>>>> ---
>>>> 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.

Then having code as parameter for a macro is really not nice I think.

> 
>> 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.

Understood then my statement of Xen having an ASSERT different from any other known
assert still stands, this is no something I would vote for.
If you want to keep the code then maybe using ASSERT_ACTION or something like that 
and keep ASSERT being a standard assert would be a good idea.

Regards
Bertrand

> 
> Jan