Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.12.2020 14:19, Julien Grall wrote:
> On 15/12/2020 11:46, Jan Beulich wrote:
>> On 15.12.2020 12:26, Julien Grall wrote:
>>> --- a/xen/include/xen/lib.h
>>> +++ b/xen/include/xen/lib.h
>>> @@ -23,7 +23,13 @@
>>>   #include <asm/bug.h>
>>>   
>>>   #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
>>> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
>>> +#define WARN_ON(p)  ({                  \
>>> +    bool __ret_warn_on = (p);           \
>>
>> Please can you avoid leading underscores here?
> 
> I can.
> 
>>
>>> +                                        \
>>> +    if ( unlikely(__ret_warn_on) )      \
>>> +        WARN();                         \
>>> +    unlikely(__ret_warn_on);            \
>>> +})
>>
>> Is this latter unlikely() having any effect? So far I thought it
>> would need to be immediately inside a control construct or be an
>> operand to && or ||.
> 
> The unlikely() is directly taken from the Linux implementation.
> 
> My guess is the compiler is still able to use the information for the 
> branch prediction in the case of:
> 
> if ( WARN_ON(...) )

Maybe. Or maybe not. I don't suppose the Linux commit introducing
it clarifies this?

Jan