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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.12.2020 00:54, Stefano Stabellini wrote:
> On Tue, 15 Dec 2020, Jan Beulich wrote:
>> 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?
> 
> I did a bit of digging but it looks like the unlikely has been there
> forever. I'd just keep it as is.

I'm afraid I don't view this as a reason to inherit code unchanged.
If it was introduced with a clear indication that compilers can
recognize it despite the somewhat unusual placement, then fine. But
likely() / unlikely() quite often get put in more or less blindly -
see the not uncommon unlikely(a && b) style of uses, which don't
typically have the intended effect and would instead need to be
unlikely(a) && unlikely(b) [assuming each condition alone is indeed
deemed unlikely], unless compilers have learned to guess/infer
what's meant between when I last looked at this and now.

Jan