Re: [PATCH 1/2] eal: add unreachable and precondition hints

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Nov 04, 2024 at 12:40:49PM +0100, Morten Brørup wrote:
> Ping for review.
> 
> > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > Sent: Friday, 25 October 2024 13.52
> > 
> > Added two new compiler/optimizer hints:
> > * The __rte_unreachable hint for use in points in code known never to
> > be
> > reached.
> > * The __rte_assume hint for providing information about preconditions
> > the
> > compiler/optimizer might be unable to figure out by itself.
> > 
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> >  lib/eal/include/rte_common.h | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/lib/eal/include/rte_common.h
> > b/lib/eal/include/rte_common.h
> > index c79f9ed319..2f143c87e4 100644
> > --- a/lib/eal/include/rte_common.h
> > +++ b/lib/eal/include/rte_common.h
> > @@ -366,6 +366,16 @@ static void
> > __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> >  #define __rte_noreturn __attribute__((noreturn))
> >  #endif
> > 
> > +/**
> > + * Hint point in program never reached
> > + */
> > +#if defined(RTE_TOOLCHAIN_GCC) || defined(RTE_TOOLCHAIN_CLANG)
> > +#define __rte_unreachable() __extension__(__builtin_unreachable())
> > +#else
> > +/* MSVC or ICC */
> > +#define __rte_unreachable() __assume(0)
> > +#endif
> > +

Is there already somewhere in the code where we might want to use this? I'm
not sure about just adding macros just in case they might be needed in
future. Having unreachable code seems a bit problematic to me generally
anyway.

> >  /**
> >   * Issue a warning in case the function's return value is ignored.
> >   *
> > @@ -423,6 +433,23 @@ static void
> > __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> >  #define __rte_cold __attribute__((cold))
> >  #endif
> > 
> > +/**
> > + * Hint precondition
> > + *
> > + * @warning Depending on the compiler, any code in ``condition`` might
> > be executed.
> > + * This currently only occurs with GCC prior to version 13.
> > + */
> > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 130000)
> > +#define __rte_assume(condition) __attribute__((assume(condition)))
> > +#elif defined(RTE_TOOLCHAIN_GCC)
> > +#define __rte_assume(condition) do { if (!(condition))
> > __rte_unreachable(); } while (0)
> > +#elif defined(RTE_TOOLCHAIN_CLANG)
> > +#define __rte_assume(condition)
> > __extension__(__builtin_assume(condition))
> > +#else
> > +/* MSVC or ICC */
> > +#define __rte_assume(condition) __assume(condition)
> > +#endif
> > +

This part seems ok, I see it used in the next patch, and also looks rather
useful to have.

/Bruce