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

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 4 November 2024 13.19
> 
> 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?

Yes. It's used in the __rte_assume() macro below for GCC before version 13.

> I'm
> not sure about just adding macros just in case they might be needed in
> future.

IMHO, such hints might be useful for applications, and DPDK could use them in the future.
But if it came to a vote about adding unused/dead code, it would probably not be accepted.
Anyway, this macro is not unused, so no problem here.

> Having unreachable code seems a bit problematic to me generally
> anyway.

Agree.
The correct use of __rte_unreachable() is to provide information to the compiler/optimizer/sanitizer it cannot figure out by itself, and/or as information to the code reviewer.
Using it to prevent compiler warnings from unreachable code would be utterly wrong. I don't know if that is even possible.

> 
> > >  /**
> > >   * 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)

Note: I did not come up with this myself; this seems to be the "official" way of doing it with older GCC versions.

> > > +#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