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

[Jürgen Groß <jgross@xxxxxxxx>]

On 18.12.20 08:54, Jan Beulich wrote:
> 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.
I have made a little experiment and found that the unlikely() at the
end of a macro is having effect.

The disassembly of

#define unlikely(x) __builtin_expect(!!(x), 0)

#define foo() ({        \
        int i = !time(NULL); \
        unlikely(i); })

#include "stdio.h"
#include "time.h"

int main() {
    if (foo())
        puts("a");
    return 0;
}

is the same as that of:

#define unlikely(x) __builtin_expect(!!(x), 0)

#include "stdio.h"
#include "time.h"

int main() {
    int i = !time(NULL);

    if (unlikely(i))
        puts("a");
    return 0;
}

while that of:

#include "stdio.h"
#include "time.h"

int main() {
    int i = !time(NULL);

    if (i)
        puts("a");
    return 0;
}

is different.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature