[PATCH] remove some MSVC conditional compile to empty

[PATCH] remove some MSVC conditional compile to empty

[Tyler Retzlaff]

Introduce a new macro __rte_attribute intended for internal use that
may be used when defining other __rte_xxx macros that should expand
empty on MSVC.

Use the new macro to clean up conditional compile in rte_common.h

Tyler Retzlaff (1):
  eal: provide rte attribute macro for GCC attribute

 lib/eal/include/rte_common.h | 77 ++++++++++++--------------------------------
 1 file changed, 21 insertions(+), 56 deletions(-)

-- 
1.8.3.1

[PATCH] eal: provide rte attribute macro for GCC attribute

[Tyler Retzlaff]

Provide a new macro __rte_attribute(a) that when directly used
compiles to empty for MSVC and to __attribute__(a) when using GCC/LLVM.

Replace direct use of __attribute__ in __rte_xxx macros where there is
existing empty expansion of the macro for MSVC allowing removal of
repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/include/rte_common.h | 77 ++++++++++++--------------------------------
 1 file changed, 21 insertions(+), 56 deletions(-)

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index d7d6390..e582f99 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -24,6 +24,12 @@
 /* OS specific include */
 #include <rte_os.h>
 
+#ifdef RTE_TOOLCHAIN_MSVC
+#define __rte_attribute(a)
+#else
+#define __rte_attribute(a) __attribute__(a)
+#endif
+
 #ifndef RTE_TOOLCHAIN_MSVC
 #ifndef typeof
 #define typeof __typeof__
@@ -83,29 +89,16 @@
 /**
  * Force a structure to be packed
  */
-#ifdef RTE_TOOLCHAIN_MSVC
-#define __rte_packed
-#else
-#define __rte_packed __attribute__((__packed__))
-#endif
+#define __rte_packed __rte_attribute((__packed__))
 
 /**
  * Macro to mark a type that is not subject to type-based aliasing rules
  */
-#ifdef RTE_TOOLCHAIN_MSVC
-#define __rte_may_alias
-#else
-#define __rte_may_alias __attribute__((__may_alias__))
-#endif
+#define __rte_may_alias __rte_attribute((__may_alias__))
 
 /******* Macro to mark functions and fields scheduled for removal *****/
-#ifdef RTE_TOOLCHAIN_MSVC
-#define __rte_deprecated
-#define __rte_deprecated_msg(msg)
-#else
-#define __rte_deprecated	__attribute__((__deprecated__))
-#define __rte_deprecated_msg(msg)	__attribute__((__deprecated__(msg)))
-#endif
+#define __rte_deprecated	__rte_attribute((__deprecated__))
+#define __rte_deprecated_msg(msg)	__rte_attribute((__deprecated__(msg)))
 
 /**
  *  Macro to mark macros and defines scheduled for removal
@@ -121,27 +114,19 @@
 /**
  * Mark a function or variable to a weak reference.
  */
-#define __rte_weak __attribute__((__weak__))
+#define __rte_weak __rte_attribute((__weak__))
 
 /**
  * Force symbol to be generated even if it appears to be unused.
  */
-#ifdef RTE_TOOLCHAIN_MSVC
-#define __rte_used
-#else
-#define __rte_used __attribute__((used))
-#endif
+#define __rte_used __rte_attribute((used))
 
 /*********** Macros to eliminate unused variable warnings ********/
 
 /**
  * short definition to mark a function parameter unused
  */
-#ifdef RTE_TOOLCHAIN_MSVC
-#define __rte_unused
-#else
-#define __rte_unused __attribute__((__unused__))
-#endif
+#define __rte_unused __rte_attribute((__unused__))
 
 /**
  * Mark pointer as restricted with regard to pointer aliasing.
@@ -165,16 +150,12 @@
  * even if the underlying stdio implementation is ANSI-compliant,
  * so this must be overridden.
  */
-#ifdef RTE_TOOLCHAIN_MSVC
-#define __rte_format_printf(format_index, first_arg)
-#else
 #if RTE_CC_IS_GNU
 #define __rte_format_printf(format_index, first_arg) \
-	__attribute__((format(gnu_printf, format_index, first_arg)))
+	__rte_attribute((format(gnu_printf, format_index, first_arg)))
 #else
 #define __rte_format_printf(format_index, first_arg) \
-	__attribute__((format(printf, format_index, first_arg)))
-#endif
+	__rte_attribute((format(printf, format_index, first_arg)))
 #endif
 
 /**
@@ -298,11 +279,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
 /**
  * Hint never returning function
  */
-#ifdef RTE_TOOLCHAIN_MSVC
-#define __rte_noreturn
-#else
-#define __rte_noreturn __attribute__((noreturn))
-#endif
+#define __rte_noreturn __rte_attribute((noreturn))
 
 /**
  * Issue a warning in case the function's return value is ignored.
@@ -327,39 +304,27 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
  *  }
  * @endcode
  */
-#ifdef RTE_TOOLCHAIN_MSVC
-#define __rte_warn_unused_result
-#else
-#define __rte_warn_unused_result __attribute__((warn_unused_result))
-#endif
+#define __rte_warn_unused_result __rte_attribute((warn_unused_result))
 
 /**
  * Force a function to be inlined
  */
-#ifdef RTE_TOOLCHAIN_MSVC
-#define __rte_always_inline
-#else
-#define __rte_always_inline inline __attribute__((always_inline))
-#endif
+#define __rte_always_inline inline __rte_attribute((always_inline))
 
 /**
  * Force a function to be noinlined
  */
-#define __rte_noinline __attribute__((noinline))
+#define __rte_noinline __rte_attribute((noinline))
 
 /**
  * Hint function in the hot path
  */
-#define __rte_hot __attribute__((hot))
+#define __rte_hot __rte_attribute((hot))
 
 /**
  * Hint function in the cold path
  */
-#ifdef RTE_TOOLCHAIN_MSVC
-#define __rte_cold
-#else
-#define __rte_cold __attribute__((cold))
-#endif
+#define __rte_cold __rte_attribute((cold))
 
 /**
  * Disable AddressSanitizer on some code
-- 
1.8.3.1

Re: [PATCH] eal: provide rte attribute macro for GCC attribute

[Thomas Monjalon]

15/02/2024 23:20, Tyler Retzlaff:
> Provide a new macro __rte_attribute(a) that when directly used
> compiles to empty for MSVC and to __attribute__(a) when using GCC/LLVM.
> 
> Replace direct use of __attribute__ in __rte_xxx macros where there is
> existing empty expansion of the macro for MSVC allowing removal of
> repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.

I'm not sure it makes sense.
I prefer seeing clearly what is empty with MSVC.

RE: [PATCH] eal: provide rte attribute macro for GCC attribute

[Morten Brørup]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Sunday, 18 February 2024 13.24
> 
> 15/02/2024 23:20, Tyler Retzlaff:
> > Provide a new macro __rte_attribute(a) that when directly used
> > compiles to empty for MSVC and to __attribute__(a) when using
> GCC/LLVM.
> >
> > Replace direct use of __attribute__ in __rte_xxx macros where there
> is
> > existing empty expansion of the macro for MSVC allowing removal of
> > repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.
> 
> I'm not sure it makes sense.
> I prefer seeing clearly what is empty with MSVC.

This topic has previously been discussed in another context - adding external libraries [1].

Like you do here, I generally preferred #ifdefs in the code, but the majority preferred stubs "promoting improved code readability".

I might argue that Tyler is following that guidance here; and perhaps the decision should be reconsidered, now that we have a real-life example of how it affects code readability. ;-)

[1]: https://inbox.dpdk.org/dev/20240109141009.497807-1-jerinj@marvell.com/

Re: [PATCH] eal: provide rte attribute macro for GCC attribute

[Thomas Monjalon]

18/02/2024 13:53, Morten Brørup:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Sunday, 18 February 2024 13.24
> > 
> > 15/02/2024 23:20, Tyler Retzlaff:
> > > Provide a new macro __rte_attribute(a) that when directly used
> > > compiles to empty for MSVC and to __attribute__(a) when using
> > GCC/LLVM.
> > >
> > > Replace direct use of __attribute__ in __rte_xxx macros where there
> > is
> > > existing empty expansion of the macro for MSVC allowing removal of
> > > repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.
> > 
> > I'm not sure it makes sense.
> > I prefer seeing clearly what is empty with MSVC.
> 
> This topic has previously been discussed in another context - adding external libraries [1].
> 
> Like you do here, I generally preferred #ifdefs in the code, but the majority preferred stubs "promoting improved code readability".

Stubs may make sense in many places,
but here we are talking about rte_common.h
where we abstract differences between arch and compilers,
so it is the right place to be explicit with compilers support.

> I might argue that Tyler is following that guidance here; and perhaps the decision should be reconsidered, now that we have a real-life example of how it affects code readability. ;-)
> 
> [1]: https://inbox.dpdk.org/dev/20240109141009.497807-1-jerinj@marvell.com/

RE: [PATCH] eal: provide rte attribute macro for GCC attribute

[Morten Brørup]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Sunday, 18 February 2024 16.35
> 
> 18/02/2024 13:53, Morten Brørup:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Sunday, 18 February 2024 13.24
> > >
> > > 15/02/2024 23:20, Tyler Retzlaff:
> > > > Provide a new macro __rte_attribute(a) that when directly used
> > > > compiles to empty for MSVC and to __attribute__(a) when using
> > > GCC/LLVM.
> > > >
> > > > Replace direct use of __attribute__ in __rte_xxx macros where
> there
> > > is
> > > > existing empty expansion of the macro for MSVC allowing removal
> of
> > > > repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.
> > >
> > > I'm not sure it makes sense.
> > > I prefer seeing clearly what is empty with MSVC.
> >
> > This topic has previously been discussed in another context - adding
> external libraries [1].
> >
> > Like you do here, I generally preferred #ifdefs in the code, but the
> majority preferred stubs "promoting improved code readability".
> 
> Stubs may make sense in many places,
> but here we are talking about rte_common.h
> where we abstract differences between arch and compilers,
> so it is the right place to be explicit with compilers support.

Very strong point. I'm convinced.

Should the new rte_attribute() macro still be introduced for other uses of __attribute__(), e.g. the somewhat exotic attributes in eal/include/rte_lock_annotations.h?

The not-so-exotic attributes could have new macros added, e.g. __rte_const and __rte_pure.

> 
> > I might argue that Tyler is following that guidance here; and perhaps
> the decision should be reconsidered, now that we have a real-life
> example of how it affects code readability. ;-)
> >
> > [1]: https://inbox.dpdk.org/dev/20240109141009.497807-1-
> jerinj@marvell.com/
> 
> 

Re: [PATCH] eal: provide rte attribute macro for GCC attribute

[Thomas Monjalon]

18/02/2024 17:38, Morten Brørup:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Sunday, 18 February 2024 16.35
> > 
> > 18/02/2024 13:53, Morten Brørup:
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > Sent: Sunday, 18 February 2024 13.24
> > > >
> > > > 15/02/2024 23:20, Tyler Retzlaff:
> > > > > Provide a new macro __rte_attribute(a) that when directly used
> > > > > compiles to empty for MSVC and to __attribute__(a) when using
> > > > GCC/LLVM.
> > > > >
> > > > > Replace direct use of __attribute__ in __rte_xxx macros where
> > there
> > > > is
> > > > > existing empty expansion of the macro for MSVC allowing removal
> > of
> > > > > repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.
> > > >
> > > > I'm not sure it makes sense.
> > > > I prefer seeing clearly what is empty with MSVC.
> > >
> > > This topic has previously been discussed in another context - adding
> > external libraries [1].
> > >
> > > Like you do here, I generally preferred #ifdefs in the code, but the
> > majority preferred stubs "promoting improved code readability".
> > 
> > Stubs may make sense in many places,
> > but here we are talking about rte_common.h
> > where we abstract differences between arch and compilers,
> > so it is the right place to be explicit with compilers support.
> 
> Very strong point. I'm convinced.
> 
> Should the new rte_attribute() macro still be introduced for other uses of __attribute__(), e.g. the somewhat exotic attributes in eal/include/rte_lock_annotations.h?

They are all wrapped in a meaningful macro.

> The not-so-exotic attributes could have new macros added, e.g. __rte_const and __rte_pure.

Yes we need wrappers for all attributes.


> > > I might argue that Tyler is following that guidance here; and perhaps
> > the decision should be reconsidered, now that we have a real-life
> > example of how it affects code readability. ;-)
> > >
> > > [1]: https://inbox.dpdk.org/dev/20240109141009.497807-1-
> > jerinj@marvell.com/

Re: [PATCH] eal: provide rte attribute macro for GCC attribute

[Tyler Retzlaff]

On Sun, Feb 18, 2024 at 05:44:48PM +0100, Thomas Monjalon wrote:
> 18/02/2024 17:38, Morten Brørup:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Sunday, 18 February 2024 16.35
> > > 
> > > 18/02/2024 13:53, Morten Brørup:
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > Sent: Sunday, 18 February 2024 13.24
> > > > >
> > > > > 15/02/2024 23:20, Tyler Retzlaff:
> > > > > > Provide a new macro __rte_attribute(a) that when directly used
> > > > > > compiles to empty for MSVC and to __attribute__(a) when using
> > > > > GCC/LLVM.
> > > > > >
> > > > > > Replace direct use of __attribute__ in __rte_xxx macros where
> > > there
> > > > > is
> > > > > > existing empty expansion of the macro for MSVC allowing removal
> > > of
> > > > > > repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.
> > > > >
> > > > > I'm not sure it makes sense.
> > > > > I prefer seeing clearly what is empty with MSVC.
> > > >
> > > > This topic has previously been discussed in another context - adding
> > > external libraries [1].
> > > >
> > > > Like you do here, I generally preferred #ifdefs in the code, but the
> > > majority preferred stubs "promoting improved code readability".
> > > 
> > > Stubs may make sense in many places,
> > > but here we are talking about rte_common.h
> > > where we abstract differences between arch and compilers,
> > > so it is the right place to be explicit with compilers support.
> > 
> > Very strong point. I'm convinced.
> > 
> > Should the new rte_attribute() macro still be introduced for other uses of __attribute__(), e.g. the somewhat exotic attributes in eal/include/rte_lock_annotations.h?
> 
> They are all wrapped in a meaningful macro.
> 
> > The not-so-exotic attributes could have new macros added, e.g. __rte_const and __rte_pure.
> 
> Yes we need wrappers for all attributes.

These are my expectation of what is supposed to be done based on the
comments in checkpatches.sh. The intention would be that
__rte__attribute not be directly used outside of rte_common.h (as is
required for __attribute__).

Introducing __rte_attribute was an attempt by me to appease previous
complaints of there being a conditional MSVC check for many macros which
was triggered by finding bare use of __attribute__ elsewhere
contradicting what was commented in checkpatches.sh

> 
> 
> > > > I might argue that Tyler is following that guidance here; and perhaps
> > > the decision should be reconsidered, now that we have a real-life
> > > example of how it affects code readability. ;-)
> > > >
> > > > [1]: https://inbox.dpdk.org/dev/20240109141009.497807-1-
> > > jerinj@marvell.com/
> 
> 

Re: [PATCH] eal: provide rte attribute macro for GCC attribute

[Mattias Rönnblom]

On 2024-02-18 13:24, Thomas Monjalon wrote:
> 15/02/2024 23:20, Tyler Retzlaff:
>> Provide a new macro __rte_attribute(a) that when directly used
>> compiles to empty for MSVC and to __attribute__(a) when using GCC/LLVM.
>>
>> Replace direct use of __attribute__ in __rte_xxx macros where there is
>> existing empty expansion of the macro for MSVC allowing removal of
>> repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.
> 
> I'm not sure it makes sense.
> I prefer seeing clearly what is empty with MSVC.
> 
> 

Anything __rte_attribute() is empty on MSVC. You could rename it 
__rte_attribute_ignored_by_msvc() for clarity.

One could note that on the ignore list are things like "may_alias" and 
"packed", so whatever comes out of a MSVC build should not be expected 
to actually work.

Unless I'm missing something, for all the attributes that will have 
MSVC-propriety equivalent, the usage pattern would have to change, since 
the syntax is different in incompatible ways.

Wouldn't it be better to ask the MSVC team to add support GCC 
attributes? ICC and LLVM managed, so why not Microsoft. Then you would 
solve this issue for all Open Source projects, not only DPDK.

Re: [PATCH] eal: provide rte attribute macro for GCC attribute

[Thomas Monjalon]

18/02/2024 15:51, Mattias Rönnblom:
> On 2024-02-18 13:24, Thomas Monjalon wrote:
> > 15/02/2024 23:20, Tyler Retzlaff:
> >> Provide a new macro __rte_attribute(a) that when directly used
> >> compiles to empty for MSVC and to __attribute__(a) when using GCC/LLVM.
> >>
> >> Replace direct use of __attribute__ in __rte_xxx macros where there is
> >> existing empty expansion of the macro for MSVC allowing removal of
> >> repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.
> > 
> > I'm not sure it makes sense.
> > I prefer seeing clearly what is empty with MSVC.
> 
> Anything __rte_attribute() is empty on MSVC. You could rename it 
> __rte_attribute_ignored_by_msvc() for clarity.

Yes it would bring more clarity.
But I still prefer #ifdef which may work with more compilers.

> One could note that on the ignore list are things like "may_alias" and 
> "packed", so whatever comes out of a MSVC build should not be expected 
> to actually work.
> 
> Unless I'm missing something, for all the attributes that will have 
> MSVC-propriety equivalent, the usage pattern would have to change, since 
> the syntax is different in incompatible ways.
> 
> Wouldn't it be better to ask the MSVC team to add support GCC 
> attributes? ICC and LLVM managed, so why not Microsoft. Then you would 
> solve this issue for all Open Source projects, not only DPDK.

We can expect MSVC to improve.
That's another reason why I prefer to keep #ifdef to keep track easily.

Re: [PATCH] eal: provide rte attribute macro for GCC attribute

[Tyler Retzlaff]

On Sun, Feb 18, 2024 at 04:31:50PM +0100, Thomas Monjalon wrote:
> 18/02/2024 15:51, Mattias Rönnblom:
> > On 2024-02-18 13:24, Thomas Monjalon wrote:
> > > 15/02/2024 23:20, Tyler Retzlaff:
> > >> Provide a new macro __rte_attribute(a) that when directly used
> > >> compiles to empty for MSVC and to __attribute__(a) when using GCC/LLVM.
> > >>
> > >> Replace direct use of __attribute__ in __rte_xxx macros where there is
> > >> existing empty expansion of the macro for MSVC allowing removal of
> > >> repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.
> > > 
> > > I'm not sure it makes sense.
> > > I prefer seeing clearly what is empty with MSVC.
> > 
> > Anything __rte_attribute() is empty on MSVC. You could rename it 
> > __rte_attribute_ignored_by_msvc() for clarity.
> 
> Yes it would bring more clarity.
> But I still prefer #ifdef which may work with more compilers.
> 
> > One could note that on the ignore list are things like "may_alias" and 
> > "packed", so whatever comes out of a MSVC build should not be expected 
> > to actually work.
> > 
> > Unless I'm missing something, for all the attributes that will have 
> > MSVC-propriety equivalent, the usage pattern would have to change, since 
> > the syntax is different in incompatible ways.
> > 
> > Wouldn't it be better to ask the MSVC team to add support GCC 
> > attributes? ICC and LLVM managed, so why not Microsoft. Then you would 
> > solve this issue for all Open Source projects, not only DPDK.
> 
> We can expect MSVC to improve.
> That's another reason why I prefer to keep #ifdef to keep track easily.

MSVC is committed to provide functionality where something simply cannot
be done at all with their toolset and standard C. They will not make
changes to their toolset for functionality they already have.

Re: [PATCH] eal: provide rte attribute macro for GCC attribute

[Thomas Monjalon]

20/02/2024 19:06, Tyler Retzlaff:
> On Sun, Feb 18, 2024 at 04:31:50PM +0100, Thomas Monjalon wrote:
> > 18/02/2024 15:51, Mattias Rönnblom:
> > > On 2024-02-18 13:24, Thomas Monjalon wrote:
> > > > 15/02/2024 23:20, Tyler Retzlaff:
> > > >> Provide a new macro __rte_attribute(a) that when directly used
> > > >> compiles to empty for MSVC and to __attribute__(a) when using GCC/LLVM.
> > > >>
> > > >> Replace direct use of __attribute__ in __rte_xxx macros where there is
> > > >> existing empty expansion of the macro for MSVC allowing removal of
> > > >> repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.
> > > > 
> > > > I'm not sure it makes sense.
> > > > I prefer seeing clearly what is empty with MSVC.
> > > 
> > > Anything __rte_attribute() is empty on MSVC. You could rename it 
> > > __rte_attribute_ignored_by_msvc() for clarity.
> > 
> > Yes it would bring more clarity.
> > But I still prefer #ifdef which may work with more compilers.
> > 
> > > One could note that on the ignore list are things like "may_alias" and 
> > > "packed", so whatever comes out of a MSVC build should not be expected 
> > > to actually work.
> > > 
> > > Unless I'm missing something, for all the attributes that will have 
> > > MSVC-propriety equivalent, the usage pattern would have to change, since 
> > > the syntax is different in incompatible ways.
> > > 
> > > Wouldn't it be better to ask the MSVC team to add support GCC 
> > > attributes? ICC and LLVM managed, so why not Microsoft. Then you would 
> > > solve this issue for all Open Source projects, not only DPDK.
> > 
> > We can expect MSVC to improve.
> > That's another reason why I prefer to keep #ifdef to keep track easily.
> 
> MSVC is committed to provide functionality where something simply cannot
> be done at all with their toolset and standard C.

That makes sense.

> They will not make changes to their toolset for functionality they already have.

So you mean there is already a way to insert zero-size markers in a struct?

Re: [PATCH] remove some MSVC conditional compile to empty

[Tyler Retzlaff]

On Thu, Feb 15, 2024 at 02:20:17PM -0800, Tyler Retzlaff wrote:
> Introduce a new macro __rte_attribute intended for internal use that
> may be used when defining other __rte_xxx macros that should expand
> empty on MSVC.
> 
> Use the new macro to clean up conditional compile in rte_common.h
> 
> Tyler Retzlaff (1):
>   eal: provide rte attribute macro for GCC attribute
> 
>  lib/eal/include/rte_common.h | 77 ++++++++++++--------------------------------
>  1 file changed, 21 insertions(+), 56 deletions(-)
> 
> -- 
> 1.8.3.1

series was not popular so withdrawn due to objections.