[PATCH 0/2] provide toolchain abstracted __builtin_constant_p

[PATCH 0/2] provide toolchain abstracted __builtin_constant_p

[Tyler Retzlaff]

Decouple direct dependency on GCC built-in __builtin_constant_p provide
a new macro __rte_constant(e) that expands to the built-in for GCC and
to false for MSVC.

Tyler Retzlaff (2):
  eal: provide macro for GCC builtin constant intrinsic
  mempool: use rte constant macro instead of GCC builtin

 lib/eal/include/rte_common.h | 6 ++++++
 lib/mempool/rte_mempool.h    | 7 +++----
 2 files changed, 9 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] eal: provide macro for GCC builtin constant intrinsic

[Tyler Retzlaff]

MSVC does not have a __builtin_constant_p intrinsic so provide
__rte_constant(e) that expands false for MSVC and to the intrinsic for
GCC.

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

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 298a5c6..d520be6 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -44,6 +44,12 @@
 #endif
 #endif
 
+#ifdef RTE_TOOLCHAIN_MSVC
+#define __rte_constant(e) 0
+#else
+#define __rte_constant(e) __extension__(__builtin_constant_p(e))
+#endif
+
 /*
  * RTE_TOOLCHAIN_GCC is defined if the target is built with GCC,
  * while a host application (like pmdinfogen) may have another compiler.
-- 
1.8.3.1

RE: [PATCH 1/2] eal: provide macro for GCC builtin constant intrinsic

[Morten Brørup]

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Wednesday, 20 March 2024 22.34
> 
> MSVC does not have a __builtin_constant_p intrinsic so provide
> __rte_constant(e) that expands false for MSVC and to the intrinsic for
> GCC.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH 1/2] eal: provide macro for GCC builtin constant intrinsic

[Stephen Hemminger]

On Wed, 20 Mar 2024 14:33:35 -0700
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> +#ifdef RTE_TOOLCHAIN_MSVC
> +#define __rte_constant(e) 0
> +#else
> +#define __rte_constant(e) __extension__(__builtin_constant_p(e))
> +#endif
> +


I did some looking around and some other project have macros
for expressing constant expression vs constant.

Implementing this with some form of sizeof math is possible.
For example in linux/compiler.h

/*
 * This returns a constant expression while determining if an argument is
 * a constant expression, most importantly without evaluating the argument.
 * Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
 *
 * Details:
 * - sizeof() return an integer constant expression, and does not evaluate
 *   the value of its operand; it only examines the type of its operand.
 * - The results of comparing two integer constant expressions is also
 *   an integer constant expression.
 * - The first literal "8" isn't important. It could be any literal value.
 * - The second literal "8" is to avoid warnings about unaligned pointers;
 *   this could otherwise just be "1".
 * - (long)(x) is used to avoid warnings about 64-bit types on 32-bit
 *   architectures.
 * - The C Standard defines "null pointer constant", "(void *)0", as
 *   distinct from other void pointers.
 * - If (x) is an integer constant expression, then the "* 0l" resolves
 *   it into an integer constant expression of value 0. Since it is cast to
 *   "void *", this makes the second operand a null pointer constant.
 * - If (x) is not an integer constant expression, then the second operand
 *   resolves to a void pointer (but not a null pointer constant: the value
 *   is not an integer constant 0).
 * - The conditional operator's third operand, "(int *)8", is an object
 *   pointer (to type "int").
 * - The behavior (including the return type) of the conditional operator
 *   ("operand1 ? operand2 : operand3") depends on the kind of expressions
 *   given for the second and third operands. This is the central mechanism
 *   of the macro:
 *   - When one operand is a null pointer constant (i.e. when x is an integer
 *     constant expression) and the other is an object pointer (i.e. our
 *     third operand), the conditional operator returns the type of the
 *     object pointer operand (i.e. "int *). Here, within the sizeof(), we
 *     would then get:
 *       sizeof(*((int *)(...))  == sizeof(int)  == 4
 *   - When one operand is a void pointer (i.e. when x is not an integer
 *     constant expression) and the other is an object pointer (i.e. our
 *     third operand), the conditional operator returns a "void *" type.
 *     Here, within the sizeof(), we would then get:
 *       sizeof(*((void *)(...)) == sizeof(void) == 1
 * - The equality comparison to "sizeof(int)" therefore depends on (x):
 *     sizeof(int) == sizeof(int)     (x) was a constant expression
 *     sizeof(int) != sizeof(void)    (x) was not a constant expression
 */
#define __is_constexpr(x) \
	(sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))

RE: [PATCH 1/2] eal: provide macro for GCC builtin constant intrinsic

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, 1 April 2024 00.03
> 
> On Wed, 20 Mar 2024 14:33:35 -0700
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > +#ifdef RTE_TOOLCHAIN_MSVC
> > +#define __rte_constant(e) 0
> > +#else
> > +#define __rte_constant(e) __extension__(__builtin_constant_p(e))
> > +#endif
> > +
> 
> 
> I did some looking around and some other project have macros
> for expressing constant expression vs constant.
> 
> Implementing this with some form of sizeof math is possible.
> For example in linux/compiler.h
> 
> /*
>  * This returns a constant expression while determining if an argument
> is
>  * a constant expression, most importantly without evaluating the
> argument.
>  * Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
>  *
>  * Details:
>  * - sizeof() return an integer constant expression, and does not
> evaluate
>  *   the value of its operand; it only examines the type of its operand.
>  * - The results of comparing two integer constant expressions is also
>  *   an integer constant expression.
>  * - The first literal "8" isn't important. It could be any literal
> value.
>  * - The second literal "8" is to avoid warnings about unaligned
> pointers;
>  *   this could otherwise just be "1".
>  * - (long)(x) is used to avoid warnings about 64-bit types on 32-bit
>  *   architectures.
>  * - The C Standard defines "null pointer constant", "(void *)0", as
>  *   distinct from other void pointers.
>  * - If (x) is an integer constant expression, then the "* 0l" resolves
>  *   it into an integer constant expression of value 0. Since it is cast
> to
>  *   "void *", this makes the second operand a null pointer constant.
>  * - If (x) is not an integer constant expression, then the second
> operand
>  *   resolves to a void pointer (but not a null pointer constant: the
> value
>  *   is not an integer constant 0).
>  * - The conditional operator's third operand, "(int *)8", is an object
>  *   pointer (to type "int").
>  * - The behavior (including the return type) of the conditional
> operator
>  *   ("operand1 ? operand2 : operand3") depends on the kind of
> expressions
>  *   given for the second and third operands. This is the central
> mechanism
>  *   of the macro:
>  *   - When one operand is a null pointer constant (i.e. when x is an
> integer
>  *     constant expression) and the other is an object pointer (i.e. our
>  *     third operand), the conditional operator returns the type of the
>  *     object pointer operand (i.e. "int *). Here, within the sizeof(),
> we
>  *     would then get:
>  *       sizeof(*((int *)(...))  == sizeof(int)  == 4
>  *   - When one operand is a void pointer (i.e. when x is not an integer
>  *     constant expression) and the other is an object pointer (i.e. our
>  *     third operand), the conditional operator returns a "void *" type.
>  *     Here, within the sizeof(), we would then get:
>  *       sizeof(*((void *)(...)) == sizeof(void) == 1
>  * - The equality comparison to "sizeof(int)" therefore depends on (x):
>  *     sizeof(int) == sizeof(int)     (x) was a constant expression
>  *     sizeof(int) != sizeof(void)    (x) was not a constant expression
>  */
> #define __is_constexpr(x) \
> 	(sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int
> *)8)))

Nice!
If the author is willing to license it under the BSD license, we can copy it as is.

We might want to add a couple of build time checks to verify that it does what is expected; to catch any changes in compiler behavior.

RE: [PATCH 1/2] eal: provide macro for GCC builtin constant intrinsic

[Morten Brørup]

PING for Review/ACK.

Come on fellow reviewers, it's only 5 lines of code!

The mempool library cannot build with MSVC without this patch series.

Other patches are also being held back, waiting for this MSVC compatible DPDK macro for __builtin_constant_p().

The macro for MSVC can be improved as suggested by Stephen later.

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Monday, 1 April 2024 10.35
> 
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Monday, 1 April 2024 00.03
> >
> > On Wed, 20 Mar 2024 14:33:35 -0700
> > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> >
> > > +#ifdef RTE_TOOLCHAIN_MSVC
> > > +#define __rte_constant(e) 0
> > > +#else
> > > +#define __rte_constant(e) __extension__(__builtin_constant_p(e))
> > > +#endif
> > > +
> >
> >
> > I did some looking around and some other project have macros
> > for expressing constant expression vs constant.
> >
> > Implementing this with some form of sizeof math is possible.
> > For example in linux/compiler.h
> >
> > /*
> >  * This returns a constant expression while determining if an argument
> > is
> >  * a constant expression, most importantly without evaluating the
> > argument.
> >  * Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
> >  *
> >  * Details:
> >  * - sizeof() return an integer constant expression, and does not
> > evaluate
> >  *   the value of its operand; it only examines the type of its operand.
> >  * - The results of comparing two integer constant expressions is also
> >  *   an integer constant expression.
> >  * - The first literal "8" isn't important. It could be any literal
> > value.
> >  * - The second literal "8" is to avoid warnings about unaligned
> > pointers;
> >  *   this could otherwise just be "1".
> >  * - (long)(x) is used to avoid warnings about 64-bit types on 32-bit
> >  *   architectures.
> >  * - The C Standard defines "null pointer constant", "(void *)0", as
> >  *   distinct from other void pointers.
> >  * - If (x) is an integer constant expression, then the "* 0l" resolves
> >  *   it into an integer constant expression of value 0. Since it is cast
> > to
> >  *   "void *", this makes the second operand a null pointer constant.
> >  * - If (x) is not an integer constant expression, then the second
> > operand
> >  *   resolves to a void pointer (but not a null pointer constant: the
> > value
> >  *   is not an integer constant 0).
> >  * - The conditional operator's third operand, "(int *)8", is an object
> >  *   pointer (to type "int").
> >  * - The behavior (including the return type) of the conditional
> > operator
> >  *   ("operand1 ? operand2 : operand3") depends on the kind of
> > expressions
> >  *   given for the second and third operands. This is the central
> > mechanism
> >  *   of the macro:
> >  *   - When one operand is a null pointer constant (i.e. when x is an
> > integer
> >  *     constant expression) and the other is an object pointer (i.e. our
> >  *     third operand), the conditional operator returns the type of the
> >  *     object pointer operand (i.e. "int *). Here, within the sizeof(),
> > we
> >  *     would then get:
> >  *       sizeof(*((int *)(...))  == sizeof(int)  == 4
> >  *   - When one operand is a void pointer (i.e. when x is not an integer
> >  *     constant expression) and the other is an object pointer (i.e. our
> >  *     third operand), the conditional operator returns a "void *" type.
> >  *     Here, within the sizeof(), we would then get:
> >  *       sizeof(*((void *)(...)) == sizeof(void) == 1
> >  * - The equality comparison to "sizeof(int)" therefore depends on (x):
> >  *     sizeof(int) == sizeof(int)     (x) was a constant expression
> >  *     sizeof(int) != sizeof(void)    (x) was not a constant expression
> >  */
> > #define __is_constexpr(x) \
> > 	(sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int
> > *)8)))
> 
> Nice!
> If the author is willing to license it under the BSD license, we can copy it
> as is.
> 
> We might want to add a couple of build time checks to verify that it does what
> is expected; to catch any changes in compiler behavior.

Re: [PATCH 1/2] eal: provide macro for GCC builtin constant intrinsic

[Andrew Rybchenko]

On 4/1/24 11:34, Morten Brørup wrote:
>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>> Sent: Monday, 1 April 2024 00.03
>>
>> On Wed, 20 Mar 2024 14:33:35 -0700
>> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
>>
>>> +#ifdef RTE_TOOLCHAIN_MSVC
>>> +#define __rte_constant(e) 0
>>> +#else
>>> +#define __rte_constant(e) __extension__(__builtin_constant_p(e))
>>> +#endif
>>> +
>>
>>
>> I did some looking around and some other project have macros
>> for expressing constant expression vs constant.
>>
>> Implementing this with some form of sizeof math is possible.
>> For example in linux/compiler.h
>>
>> /*
>>   * This returns a constant expression while determining if an argument
>> is
>>   * a constant expression, most importantly without evaluating the
>> argument.
>>   * Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
>>   *
>>   * Details:
>>   * - sizeof() return an integer constant expression, and does not
>> evaluate
>>   *   the value of its operand; it only examines the type of its operand.
>>   * - The results of comparing two integer constant expressions is also
>>   *   an integer constant expression.
>>   * - The first literal "8" isn't important. It could be any literal
>> value.
>>   * - The second literal "8" is to avoid warnings about unaligned
>> pointers;
>>   *   this could otherwise just be "1".
>>   * - (long)(x) is used to avoid warnings about 64-bit types on 32-bit
>>   *   architectures.
>>   * - The C Standard defines "null pointer constant", "(void *)0", as
>>   *   distinct from other void pointers.
>>   * - If (x) is an integer constant expression, then the "* 0l" resolves
>>   *   it into an integer constant expression of value 0. Since it is cast
>> to
>>   *   "void *", this makes the second operand a null pointer constant.
>>   * - If (x) is not an integer constant expression, then the second
>> operand
>>   *   resolves to a void pointer (but not a null pointer constant: the
>> value
>>   *   is not an integer constant 0).
>>   * - The conditional operator's third operand, "(int *)8", is an object
>>   *   pointer (to type "int").
>>   * - The behavior (including the return type) of the conditional
>> operator
>>   *   ("operand1 ? operand2 : operand3") depends on the kind of
>> expressions
>>   *   given for the second and third operands. This is the central
>> mechanism
>>   *   of the macro:
>>   *   - When one operand is a null pointer constant (i.e. when x is an
>> integer
>>   *     constant expression) and the other is an object pointer (i.e. our
>>   *     third operand), the conditional operator returns the type of the
>>   *     object pointer operand (i.e. "int *). Here, within the sizeof(),
>> we
>>   *     would then get:
>>   *       sizeof(*((int *)(...))  == sizeof(int)  == 4
>>   *   - When one operand is a void pointer (i.e. when x is not an integer
>>   *     constant expression) and the other is an object pointer (i.e. our
>>   *     third operand), the conditional operator returns a "void *" type.
>>   *     Here, within the sizeof(), we would then get:
>>   *       sizeof(*((void *)(...)) == sizeof(void) == 1
>>   * - The equality comparison to "sizeof(int)" therefore depends on (x):
>>   *     sizeof(int) == sizeof(int)     (x) was a constant expression
>>   *     sizeof(int) != sizeof(void)    (x) was not a constant expression
>>   */
>> #define __is_constexpr(x) \
>> 	(sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int
>> *)8)))
> 
> Nice!
> If the author is willing to license it under the BSD license, we can copy it as is.
> 
> We might want to add a couple of build time checks to verify that it does what is expected; to catch any changes in compiler behavior.
> 

LGTM too, but meanwhile we can continue without it just to unblock build 
on MSVC

Re: [PATCH 1/2] eal: provide macro for GCC builtin constant intrinsic

[Bruce Richardson]

On Wed, Mar 20, 2024 at 02:33:35PM -0700, Tyler Retzlaff wrote:
> MSVC does not have a __builtin_constant_p intrinsic so provide
> __rte_constant(e) that expands false for MSVC and to the intrinsic for
> GCC.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/eal/include/rte_common.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index 298a5c6..d520be6 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -44,6 +44,12 @@
>  #endif
>  #endif
>  
> +#ifdef RTE_TOOLCHAIN_MSVC
> +#define __rte_constant(e) 0
> +#else
> +#define __rte_constant(e) __extension__(__builtin_constant_p(e))
> +#endif
> +

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH 1/2] eal: provide macro for GCC builtin constant intrinsic

[Andrew Rybchenko]

On 5/27/24 15:00, Bruce Richardson wrote:
> On Wed, Mar 20, 2024 at 02:33:35PM -0700, Tyler Retzlaff wrote:
>> MSVC does not have a __builtin_constant_p intrinsic so provide
>> __rte_constant(e) that expands false for MSVC and to the intrinsic for
>> GCC.
>>
>> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
>> ---
>>   lib/eal/include/rte_common.h | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
>> index 298a5c6..d520be6 100644
>> --- a/lib/eal/include/rte_common.h
>> +++ b/lib/eal/include/rte_common.h
>> @@ -44,6 +44,12 @@
>>   #endif
>>   #endif
>>   
>> +#ifdef RTE_TOOLCHAIN_MSVC
>> +#define __rte_constant(e) 0
>> +#else
>> +#define __rte_constant(e) __extension__(__builtin_constant_p(e))
>> +#endif
>> +
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

[PATCH 2/2] mempool: use rte constant macro instead of GCC builtin

[Tyler Retzlaff]

Use newly introduced __rte_constant(e) macro instead of directly using
__builtin_constant_p() allowing mempool to be built by MSVC.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/mempool/rte_mempool.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index 23fd5c8..a3564fb 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -1521,7 +1521,7 @@ struct rte_mempool_cache *
 	/* The cache is a stack, so copy will be in reverse order. */
 	cache_objs = &cache->objs[cache->len];
 
-	if (__extension__(__builtin_constant_p(n)) && n <= cache->len) {
+	if (__rte_constant(n) && n <= cache->len) {
 		/*
 		 * The request size is known at build time, and
 		 * the entire request can be satisfied from the cache,
@@ -1542,8 +1542,7 @@ struct rte_mempool_cache *
 	 * If the request size 'n' is known at build time, the above comparison
 	 * ensures that n > cache->len here, so omit RTE_MIN().
 	 */
-	len = __extension__(__builtin_constant_p(n)) ? cache->len :
-			RTE_MIN(n, cache->len);
+	len = __rte_constant(n) ? cache->len : RTE_MIN(n, cache->len);
 	cache->len -= len;
 	remaining = n - len;
 	for (index = 0; index < len; index++)
@@ -1554,7 +1553,7 @@ struct rte_mempool_cache *
 	 * where the entire request can be satisfied from the cache
 	 * has already been handled above, so omit handling it here.
 	 */
-	if (!__extension__(__builtin_constant_p(n)) && remaining == 0) {
+	if (!__rte_constant(n) && remaining == 0) {
 		/* The entire request is satisfied from the cache. */
 
 		RTE_MEMPOOL_CACHE_STAT_ADD(cache, get_success_bulk, 1);
-- 
1.8.3.1

RE: [PATCH 2/2] mempool: use rte constant macro instead of GCC builtin

[Morten Brørup]

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Wednesday, 20 March 2024 22.34
> 
> Use newly introduced __rte_constant(e) macro instead of directly using
> __builtin_constant_p() allowing mempool to be built by MSVC.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH 2/2] mempool: use rte constant macro instead of GCC builtin

[Andrew Rybchenko]

On 3/26/24 12:57, Morten Brørup wrote:
>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
>> Sent: Wednesday, 20 March 2024 22.34
>>
>> Use newly introduced __rte_constant(e) macro instead of directly using
>> __builtin_constant_p() allowing mempool to be built by MSVC.
>>
>> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
>> ---
> 
> Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
> 

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [PATCH 2/2] mempool: use rte constant macro instead of GCC builtin

[Thomas Monjalon]

20/03/2024 22:33, Tyler Retzlaff:
> Use newly introduced __rte_constant(e) macro instead of directly using
> __builtin_constant_p() allowing mempool to be built by MSVC.

Does it mean we should enable mempool build?
If yes, please send a v2.

Re: [PATCH 2/2] mempool: use rte constant macro instead of GCC builtin

[David Marchand]

On Wed, May 29, 2024 at 4:51 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 20/03/2024 22:33, Tyler Retzlaff:
> > Use newly introduced __rte_constant(e) macro instead of directly using
> > __builtin_constant_p() allowing mempool to be built by MSVC.
>
> Does it mean we should enable mempool build?
> If yes, please send a v2.

I guess now it is possible, as I merged some other patches on mempool
from Stephen that were for MSVC.
Tyler, can you send a v2 so it passes through the CI?

Thanks.

-- 
David Marchand

Re: [PATCH 2/2] mempool: use rte constant macro instead of GCC builtin

[Thomas Monjalon]

14/06/2024 16:32, David Marchand:
> On Wed, May 29, 2024 at 4:51 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 20/03/2024 22:33, Tyler Retzlaff:
> > > Use newly introduced __rte_constant(e) macro instead of directly using
> > > __builtin_constant_p() allowing mempool to be built by MSVC.
> >
> > Does it mean we should enable mempool build?
> > If yes, please send a v2.
> 
> I guess now it is possible, as I merged some other patches on mempool
> from Stephen that were for MSVC.
> Tyler, can you send a v2 so it passes through the CI?

I tried a retest last week and there is this failure on Ubuntu 24.04
that I don't manage to reproduce locally:

/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error: argument 2 null where non-null expected [-Werror=nonnull]
29 |   return __builtin___memcpy_chk (__dest, __src, __len,
|          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
30 |                                  __glibc_objsize0 (__dest));
|                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: note: in a call to built-in function '__builtin___memcpy_chk'
In function 'memcpy',
inlined from 'pcapng_add_option' at ../lib/pcapng/rte_pcapng.c:131:2,
inlined from 'rte_pcapng_write_stats' at ../lib/pcapng/rte_pcapng.c:371:9:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error: argument 2 null where non-null expected [-Werror=nonnull]
29 |   return __builtin___memcpy_chk (__dest, __src, __len,
|          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
30 |                                  __glibc_objsize0 (__dest));
|                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: note: in a call to built-in function '__builtin___memcpy_chk'

RE: [PATCH 2/2] mempool: use rte constant macro instead of GCC builtin

[Morten Brørup]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, 3 July 2024 15.17
> 
> 14/06/2024 16:32, David Marchand:
> > On Wed, May 29, 2024 at 4:51 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > > 20/03/2024 22:33, Tyler Retzlaff:
> > > > Use newly introduced __rte_constant(e) macro instead of directly using
> > > > __builtin_constant_p() allowing mempool to be built by MSVC.
> > >
> > > Does it mean we should enable mempool build?
> > > If yes, please send a v2.
> >
> > I guess now it is possible, as I merged some other patches on mempool
> > from Stephen that were for MSVC.
> > Tyler, can you send a v2 so it passes through the CI?
> 
> I tried a retest last week and there is this failure on Ubuntu 24.04
> that I don't manage to reproduce locally:
> 
> /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error: argument 2
> null where non-null expected [-Werror=nonnull]
> 29 |   return __builtin___memcpy_chk (__dest, __src, __len,
> |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 30 |                                  __glibc_objsize0 (__dest));
> |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
> /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: note: in a call
> to built-in function '__builtin___memcpy_chk'
> In function 'memcpy',
> inlined from 'pcapng_add_option' at ../lib/pcapng/rte_pcapng.c:131:2,

pcapng_add_option() in rte_pcapng.c has memcpy() on line 132 [1] (and has a fix for this error, by comparing len > 0 before calling memcpy()); older versions had memcpy() on line 131, so the CI must be building with an older version of rte_pcapng.c.

[1]: https://elixir.bootlin.com/dpdk/v24.07-rc1/source/lib/pcapng/rte_pcapng.c#L132
[2]: https://elixir.bootlin.com/dpdk/v24.03/source/lib/pcapng/rte_pcapng.c#L131

> inlined from 'rte_pcapng_write_stats' at ../lib/pcapng/rte_pcapng.c:371:9:
> /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error: argument 2
> null where non-null expected [-Werror=nonnull]
> 29 |   return __builtin___memcpy_chk (__dest, __src, __len,
> |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 30 |                                  __glibc_objsize0 (__dest));
> |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
> /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: note: in a call
> to built-in function '__builtin___memcpy_chk'
> 
> 

Re: [PATCH 2/2] mempool: use rte constant macro instead of GCC builtin

[David Marchand]

On Wed, Jul 3, 2024 at 3:49 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Wednesday, 3 July 2024 15.17
> >
> > 14/06/2024 16:32, David Marchand:
> > > On Wed, May 29, 2024 at 4:51 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > >
> > > > 20/03/2024 22:33, Tyler Retzlaff:
> > > > > Use newly introduced __rte_constant(e) macro instead of directly using
> > > > > __builtin_constant_p() allowing mempool to be built by MSVC.
> > > >
> > > > Does it mean we should enable mempool build?
> > > > If yes, please send a v2.
> > >
> > > I guess now it is possible, as I merged some other patches on mempool
> > > from Stephen that were for MSVC.
> > > Tyler, can you send a v2 so it passes through the CI?
> >
> > I tried a retest last week and there is this failure on Ubuntu 24.04
> > that I don't manage to reproduce locally:
> >
> > /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error: argument 2
> > null where non-null expected [-Werror=nonnull]
> > 29 |   return __builtin___memcpy_chk (__dest, __src, __len,
> > |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 30 |                                  __glibc_objsize0 (__dest));
> > |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: note: in a call
> > to built-in function '__builtin___memcpy_chk'
> > In function 'memcpy',
> > inlined from 'pcapng_add_option' at ../lib/pcapng/rte_pcapng.c:131:2,
>
> pcapng_add_option() in rte_pcapng.c has memcpy() on line 132 [1] (and has a fix for this error, by comparing len > 0 before calling memcpy()); older versions had memcpy() on line 131, so the CI must be building with an older version of rte_pcapng.c.
>
> [1]: https://elixir.bootlin.com/dpdk/v24.07-rc1/source/lib/pcapng/rte_pcapng.c#L132
> [2]: https://elixir.bootlin.com/dpdk/v24.03/source/lib/pcapng/rte_pcapng.c#L131

This is likely the reason.
Looking at the report:

http://mails.dpdk.org/archives/test-report/2024-June/717951.html

_Testing issues RETEST #1_

Submitter: Tyler Retzlaff <roretzla at linux.microsoft.com>
Date: Wednesday, March 20 2024 21:33:36
DPDK git baseline: Repo:dpdk
  Branch: master
  CommitID:80ecef6d1f71fcebc0a51d7cabc51f73ee142ff2


$ git describe --contains 80ecef6d1f71fcebc0a51d7cabc51f73ee142ff2
v24.03-rc3^0


From the discussions on the retest mechanism, I understand we need to
ask for a rebase.
I sent a new retest. Let's see...


-- 
David Marchand

Re: [PATCH 2/2] mempool: use rte constant macro instead of GCC builtin

[Patrick Robb]

On Wed, Jul 3, 2024 at 10:22 AM David Marchand
<david.marchand@redhat.com> wrote:
>
>
>
> From the discussions on the retest mechanism, I understand we need to
> ask for a rebase.
> I sent a new retest. Let's see...
>

Hi,

That makes sense that we need to re-apply on the latest mainline and
retest. But, the "rebase" feature for rechecks which you are referring
to is still in development. So, unfortunately your request won't work
with the automated system - it's just going to recheck from the DPDK
artifact created in March when Tyler originally submitted the series.

For now, I can manually trigger a job in our CI platform which will
re-apply Tyler's patch to the up to date tree, and rerun all testing.

Thanks!

Re: [PATCH 0/2] provide toolchain abstracted __builtin_constant_p

[Thomas Monjalon]

Recheck-request: iol-compile-amd64-testing, iol-unit-amd64-testing, iol-intel-Functional, iol-compile-arm64-testing, iol-unit-arm64-testing, github-robot

Re: [PATCH 0/2] provide toolchain abstracted __builtin_constant_p

[David Marchand]

On Wed, Mar 20, 2024 at 10:33 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> Decouple direct dependency on GCC built-in __builtin_constant_p provide
> a new macro __rte_constant(e) that expands to the built-in for GCC and
> to false for MSVC.
>
> Tyler Retzlaff (2):
>   eal: provide macro for GCC builtin constant intrinsic
>   mempool: use rte constant macro instead of GCC builtin
>
>  lib/eal/include/rte_common.h | 6 ++++++
>  lib/mempool/rte_mempool.h    | 7 +++----
>  2 files changed, 9 insertions(+), 4 deletions(-)

Recheck-request: rebase=main, iol-compile-amd64-testing,
iol-unit-amd64-testing, iol-intel-Functional,
iol-compile-arm64-testing, iol-unit-arm64-testing


-- 
David Marchand

Re: [PATCH 0/2] provide toolchain abstracted __builtin_constant_p

[Thomas Monjalon]

20/03/2024 22:33, Tyler Retzlaff:
> Decouple direct dependency on GCC built-in __builtin_constant_p provide
> a new macro __rte_constant(e) that expands to the built-in for GCC and
> to false for MSVC.
> 
> Tyler Retzlaff (2):
>   eal: provide macro for GCC builtin constant intrinsic
>   mempool: use rte constant macro instead of GCC builtin

We had some doubts with CI.
It should be solved on the top of the main branch, let's see.

Applied, thanks.

Re: [PATCH 0/2] provide toolchain abstracted __builtin_constant_p

[Patrick Robb]

On Wed, Jul 3, 2024 at 12:14 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
>
> We had some doubts with CI.
> It should be solved on the top of the main branch, let's see.
>
> Applied, thanks.
>
https://patchwork.dpdk.org/project/dpdk/patch/1710970416-27841-3-git-send-email-roretzla@linux.microsoft.com/

Looks like David's suspicion was correct. It passes with a re-apply on
main and retest.

Cheers.