diff mbox series

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

Message ID 20241025115223.1230680-2-mb@smartsharesystems.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series ethdev: support single queue per port |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Morten Brørup Oct. 25, 2024, 11:52 a.m. UTC 
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(+)

Comments

Morten Brørup Nov. 4, 2024, 11:40 a.m. UTC | #1 
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
> +
>  /**
>   * 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
> +
>  /**
>   * Disable AddressSanitizer on some code
>   */
> --
> 2.43.0
Bruce Richardson Nov. 4, 2024, 12:19 p.m. UTC | #2 
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
Morten Brørup Nov. 4, 2024, 12:53 p.m. UTC | #3 
> 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
Bruce Richardson Nov. 6, 2024, 11:48 a.m. UTC | #4 
On Fri, Oct 25, 2024 at 11:52:22AM +0000, Morten Brørup wrote:
> 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(+)
> 
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
diff mbox series

Patch

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
+
 /**
  * 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
+
 /**
  * Disable AddressSanitizer on some code
  */