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

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

On Thu, 17 Dec 2020 at 23:54, Stefano Stabellini <sstabellini@xxxxxxxxxx> 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.

Thanks! I was planning to answer earlier on with some data but got
preempted with some higher priority work.
The Linux commit message is not very helpful. I will do some testing
so I can convince Jan that compilers can be clever and make use of it.

Cheers,