[PATCH] eal: add nonnull and access function attributes

[PATCH] eal: add nonnull and access function attributes

[Morten Brørup]

Add "nonnull" function attribute to help the compiler detect a NULL
pointer being passed to a function not accepting NULL pointers as an
argument at build time.

Add "access" function attribute to tell the optimizer how a function
accesses its pointer arguments.

Add these attributes to the rte_memcpy() function, as the first in
hopefully many to come.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/eal/arm/include/rte_memcpy_32.h  |  6 ++++++
 lib/eal/arm/include/rte_memcpy_64.h  |  6 ++++++
 lib/eal/include/generic/rte_memcpy.h |  5 +++++
 lib/eal/include/rte_common.h         | 26 ++++++++++++++++++++++++++
 lib/eal/ppc/include/rte_memcpy.h     |  3 +++
 lib/eal/x86/include/rte_memcpy.h     |  6 ++++++
 6 files changed, 52 insertions(+)

diff --git a/lib/eal/arm/include/rte_memcpy_32.h b/lib/eal/arm/include/rte_memcpy_32.h
index fb3245b59c..ba4f050a5d 100644
--- a/lib/eal/arm/include/rte_memcpy_32.h
+++ b/lib/eal/arm/include/rte_memcpy_32.h
@@ -125,6 +125,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy((dst), (src), (n)) :          \
 	rte_memcpy_func((dst), (src), (n)); })
 
+__rte_nonnull(1, 2)
+__rte_access(write_only, 1, 3)
+__rte_access(read_only, 2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)
 {
@@ -290,6 +293,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy(dst, src, 256);
 }
 
+__rte_nonnull(1, 2)
+__rte_access(write_only, 1, 3)
+__rte_access(read_only, 2, 3)
 static inline void *
 rte_memcpy(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/arm/include/rte_memcpy_64.h b/lib/eal/arm/include/rte_memcpy_64.h
index 85ad587bd3..5403e30db5 100644
--- a/lib/eal/arm/include/rte_memcpy_64.h
+++ b/lib/eal/arm/include/rte_memcpy_64.h
@@ -282,6 +282,9 @@ void rte_memcpy_ge64(uint8_t *dst, const uint8_t *src, size_t n)
 }
 
 #if RTE_CACHE_LINE_SIZE >= 128
+__rte_nonnull(1, 2)
+__rte_access(write_only, 1, 3)
+__rte_access(read_only, 2, 3)
 static __rte_always_inline
 void *rte_memcpy(void *dst, const void *src, size_t n)
 {
@@ -303,6 +306,9 @@ void *rte_memcpy(void *dst, const void *src, size_t n)
 }
 
 #else
+__rte_nonnull(1, 2)
+__rte_access(write_only, 1, 3)
+__rte_access(read_only, 2, 3)
 static __rte_always_inline
 void *rte_memcpy(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/include/generic/rte_memcpy.h b/lib/eal/include/generic/rte_memcpy.h
index e7f0f8eaa9..86fd20884c 100644
--- a/lib/eal/include/generic/rte_memcpy.h
+++ b/lib/eal/include/generic/rte_memcpy.h
@@ -11,6 +11,8 @@
  * Functions for vectorised implementation of memcpy().
  */
 
+#include <rte_common.h>
+
 /**
  * Copy 16 bytes from one location to another using optimised
  * instructions. The locations should not overlap.
@@ -108,6 +110,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src);
  * @return
  *   Pointer to the destination data.
  */
+__rte_nonnull(1, 2)
+__rte_access(write_only, 1, 3)
+__rte_access(read_only, 2, 3)
 static void *
 rte_memcpy(void *dst, const void *src, size_t n);
 
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 15765b408d..c2bd6074b1 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -149,6 +149,32 @@ typedef uint16_t unaligned_uint16_t;
 	__attribute__((format(printf, format_index, first_arg)))
 #endif
 
+/**
+ * Check pointer arguments at compile-time.
+ *
+ * @param ...
+ *    Comma separated list of parameter indexes of pointer arguments.
+ */
+#define __rte_nonnull(...) \
+	__attribute__((nonnull(__VA_ARGS__)))
+
+/**
+ * Tells compiler about the access mode of a pointer argument.
+ *
+ * @param access_mode
+ *    Access mode: read_only, read_write, write_only, or none.
+ * @param ref_index
+ *    Parameter index of pointer argument.
+ * @param size_index (optional)
+ *    Parameter index of size argument.
+ */
+#if defined(RTE_CC_IS_GNU) && (GCC_VERSION >= 100400)
+#define __rte_access(access_mode, ...) \
+	__attribute__((access(access_mode, __VA_ARGS__)))
+#else
+#define __rte_access(access_mode, ...)
+#endif
+
 /**
  * Tells compiler that the function returns a value that points to
  * memory, where the size is given by the one or two arguments.
diff --git a/lib/eal/ppc/include/rte_memcpy.h b/lib/eal/ppc/include/rte_memcpy.h
index 6f388c0234..d2c3234d5d 100644
--- a/lib/eal/ppc/include/rte_memcpy.h
+++ b/lib/eal/ppc/include/rte_memcpy.h
@@ -84,6 +84,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy((dst), (src), (n)) :          \
 	rte_memcpy_func((dst), (src), (n)); })
 
+__rte_nonnull(1, 2)
+__rte_access(write_only, 1, 3)
+__rte_access(read_only, 2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/x86/include/rte_memcpy.h b/lib/eal/x86/include/rte_memcpy.h
index d4d7a5cfc8..397461115b 100644
--- a/lib/eal/x86/include/rte_memcpy.h
+++ b/lib/eal/x86/include/rte_memcpy.h
@@ -42,6 +42,9 @@ extern "C" {
  * @return
  *   Pointer to the destination data.
  */
+__rte_nonnull(1, 2)
+__rte_access(write_only, 1, 3)
+__rte_access(read_only, 2, 3)
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n);
 
@@ -859,6 +862,9 @@ rte_memcpy_aligned(void *dst, const void *src, size_t n)
 	return ret;
 }
 
+__rte_nonnull(1, 2)
+__rte_access(write_only, 1, 3)
+__rte_access(read_only, 2, 3)
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n)
 {
-- 
2.17.1

Re: [PATCH] eal: add nonnull and access function attributes

[Tyler Retzlaff]

On Fri, Dec 02, 2022 at 04:34:32PM +0100, Morten Brørup wrote:
> Add "nonnull" function attribute to help the compiler detect a NULL
> pointer being passed to a function not accepting NULL pointers as an
> argument at build time.
> 
> Add "access" function attribute to tell the optimizer how a function
> accesses its pointer arguments.
> 
> Add these attributes to the rte_memcpy() function, as the first in
> hopefully many to come.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---

thanks for providing the non-gcc expansion to empty.

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

[PATCH v2] eal: add nonnull and access function attributes

[Morten Brørup]

Add "nonnull" function attribute to help the compiler detect a NULL
pointer being passed to a function not accepting NULL pointers as an
argument at build time.

Add "access" function attribute to tell the compiler how a function
accesses its pointer arguments.

Add these attributes to the rte_memcpy() function, as the first in
hopefully many to come.

v2:
* Only define "nonnull" for GCC and CLANG.
* Append _param/_params to prepare for possible future attributes
  attached directly to the individual parameters, like __rte_unused.
* Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints about
  GCC_VERSION being undefined.
* Try to fix Doxygen compliants.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/arm/include/rte_memcpy_32.h |  8 ++++++++
 lib/eal/arm/include/rte_memcpy_64.h |  6 ++++++
 lib/eal/include/rte_common.h        | 29 +++++++++++++++++++++++++++++
 lib/eal/ppc/include/rte_memcpy.h    |  3 +++
 lib/eal/x86/include/rte_memcpy.h    |  6 ++++++
 5 files changed, 52 insertions(+)

diff --git a/lib/eal/arm/include/rte_memcpy_32.h b/lib/eal/arm/include/rte_memcpy_32.h
index fb3245b59c..f454c06eca 100644
--- a/lib/eal/arm/include/rte_memcpy_32.h
+++ b/lib/eal/arm/include/rte_memcpy_32.h
@@ -14,6 +14,8 @@ extern "C" {
 
 #include "generic/rte_memcpy.h"
 
+#include <rte_common.h>
+
 #ifdef RTE_ARCH_ARM_NEON_MEMCPY
 
 #ifndef __ARM_NEON
@@ -125,6 +127,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy((dst), (src), (n)) :          \
 	rte_memcpy_func((dst), (src), (n)); })
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)
 {
@@ -290,6 +295,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy(dst, src, 256);
 }
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static inline void *
 rte_memcpy(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/arm/include/rte_memcpy_64.h b/lib/eal/arm/include/rte_memcpy_64.h
index 85ad587bd3..55cbe07733 100644
--- a/lib/eal/arm/include/rte_memcpy_64.h
+++ b/lib/eal/arm/include/rte_memcpy_64.h
@@ -282,6 +282,9 @@ void rte_memcpy_ge64(uint8_t *dst, const uint8_t *src, size_t n)
 }
 
 #if RTE_CACHE_LINE_SIZE >= 128
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline
 void *rte_memcpy(void *dst, const void *src, size_t n)
 {
@@ -303,6 +306,9 @@ void *rte_memcpy(void *dst, const void *src, size_t n)
 }
 
 #else
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline
 void *rte_memcpy(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 15765b408d..6e4011aa85 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -149,6 +149,35 @@ typedef uint16_t unaligned_uint16_t;
 	__attribute__((format(printf, format_index, first_arg)))
 #endif
 
+/**
+ * Check pointer arguments at compile-time.
+ *
+ * @param ...
+ *    Comma separated list of parameter indexes of pointer arguments.
+ */
+#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
+#define __rte_nonnull_params(...) \
+	__attribute__((nonnull(__VA_ARGS__)))
+#else
+#define __rte_nonnull_params(...)
+#endif
+
+/**
+ * Tells compiler about the access mode of a pointer argument.
+ *
+ * @param access_mode
+ *    Access mode: read_only, read_write, write_only, or none.
+ * @param ...
+ *    Parameter index of pointer argument.
+ *    Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_access_param(access_mode, ...) \
+	__attribute__((access(access_mode, __VA_ARGS__)))
+#else
+#define __rte_access_param(access_mode, ...)
+#endif
+
 /**
  * Tells compiler that the function returns a value that points to
  * memory, where the size is given by the one or two arguments.
diff --git a/lib/eal/ppc/include/rte_memcpy.h b/lib/eal/ppc/include/rte_memcpy.h
index 6f388c0234..59a5d86948 100644
--- a/lib/eal/ppc/include/rte_memcpy.h
+++ b/lib/eal/ppc/include/rte_memcpy.h
@@ -84,6 +84,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy((dst), (src), (n)) :          \
 	rte_memcpy_func((dst), (src), (n)); })
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/x86/include/rte_memcpy.h b/lib/eal/x86/include/rte_memcpy.h
index d4d7a5cfc8..2e7161e4e7 100644
--- a/lib/eal/x86/include/rte_memcpy.h
+++ b/lib/eal/x86/include/rte_memcpy.h
@@ -42,6 +42,9 @@ extern "C" {
  * @return
  *   Pointer to the destination data.
  */
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n);
 
@@ -859,6 +862,9 @@ rte_memcpy_aligned(void *dst, const void *src, size_t n)
 	return ret;
 }
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n)
 {
-- 
2.17.1

RE: [PATCH v2] eal: add nonnull and access function attributes

[Ruifeng Wang]

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Saturday, December 3, 2022 10:23 PM
> To: dev@dpdk.org; roretzla@linux.microsoft.com
> Cc: Ruifeng Wang <Ruifeng.Wang@arm.com>; zhoumin@loongson.cn; drc@linux.vnet.ibm.com;
> kda@semihalf.com; bruce.richardson@intel.com; konstantin.v.ananyev@yandex.ru; Morten
> Brørup <mb@smartsharesystems.com>
> Subject: [PATCH v2] eal: add nonnull and access function attributes
> 
> Add "nonnull" function attribute to help the compiler detect a NULL pointer being passed
> to a function not accepting NULL pointers as an argument at build time.
> 
> Add "access" function attribute to tell the compiler how a function accesses its pointer
> arguments.
> 
> Add these attributes to the rte_memcpy() function, as the first in hopefully many to come.
> 
> v2:
> * Only define "nonnull" for GCC and CLANG.
> * Append _param/_params to prepare for possible future attributes
>   attached directly to the individual parameters, like __rte_unused.
> * Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints about
>   GCC_VERSION being undefined.
> * Try to fix Doxygen compliants.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/eal/arm/include/rte_memcpy_32.h |  8 ++++++++  lib/eal/arm/include/rte_memcpy_64.h |
> 6 ++++++
>  lib/eal/include/rte_common.h        | 29 +++++++++++++++++++++++++++++
>  lib/eal/ppc/include/rte_memcpy.h    |  3 +++
>  lib/eal/x86/include/rte_memcpy.h    |  6 ++++++
>  5 files changed, 52 insertions(+)
> 
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

Thanks and regards.

RE: [PATCH v2] eal: add nonnull and access function attributes

[Morten Brørup]

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Saturday, 3 December 2022 15.23
> 
> Add "nonnull" function attribute to help the compiler detect a NULL
> pointer being passed to a function not accepting NULL pointers as an
> argument at build time.
> 
> Add "access" function attribute to tell the compiler how a function
> accesses its pointer arguments.
> 
> Add these attributes to the rte_memcpy() function, as the first in
> hopefully many to come.

Notes about Warnings/Errors reported in Patchwork [1]:

* The checkpatch warning is about the attributes being defined. This is the purpose of the patch, and I don't know how to do it without getting a checkpatch warning.

* The errors are previously uncaught bugs elsewhere, now being caught by GCC versions >= 10.4.0 due to these new attributes. I have reported these as [Bug 1146]. It shows some actual usefulness of the attributes in this patch, and provides a good example of why we should decorate many more functions with them.

[1]: https://patchwork.dpdk.org/project/dpdk/patch/20221203142244.17135-1-mb@smartsharesystems.com/
[Bug 1146]: https://bugs.dpdk.org/show_bug.cgi?id=1146


Example of previously uncaught bug:

../drivers/net/bnx2x/bnx2x_vfpf.c: In function ‘bnx2x_check_bull’:
../drivers/net/bnx2x/bnx2x_vfpf.c:57:17: error: ‘rte_memcpy’ writing 4 bytes into a region of size 2 overflows the destination [-Werror=stringop-overflow=]
   57 |                 rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../drivers/net/bnx2x/bnx2x.h:29,
                 from ../drivers/net/bnx2x/bnx2x_vfpf.c:8:
../drivers/net/bnx2x/bnx2x_vfpf.h:297:18: note: destination object ‘vlan’ of size 2
  297 |         uint16_t vlan;
      |                  ^~~~
../drivers/net/bnx2x/bnx2x_vfpf.c:57:17: error: ‘rte_memcpy’ reading 4 bytes from a region of size 2 [-Werror=stringop-overread]
   57 |                 rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/net/bnx2x/bnx2x_vfpf.h:297:18: note: source object ‘vlan’ of size 2
  297 |         uint16_t vlan;
      |                  ^~~~
In file included from ../lib/mempool/rte_mempool.h:48,
                 from ../lib/mbuf/rte_mbuf.h:38,
                 from ../lib/net/rte_ether.h:22,
                 from ../lib/ethdev/rte_eth_ctrl.h:10,
                 from ../lib/ethdev/rte_ethdev.h:1419,
                 from ../lib/ethdev/ethdev_driver.h:24,
                 from ../drivers/net/bnx2x/bnx2x_ethdev.h:34,
                 from ../drivers/net/bnx2x/bnx2x.h:23:
../lib/eal/x86/include/rte_memcpy.h:869:1: note: in a call to function ‘rte_memcpy’ declared with attribute ‘access (read_only, 2, 3)’
  869 | rte_memcpy(void *dst, const void *src, size_t n)
      | ^~~~~~~~~~

[PATCH v3 1/2] eal: add nonnull and access function attributes

[Morten Brørup]

Add "nonnull" function attribute to help the compiler detect a NULL
pointer being passed to a function not accepting NULL pointers as an
argument at build time.

Add "access" function attribute to tell the compiler how a function
accesses its pointer arguments.

Add these attributes to the rte_memcpy() function, as the first in
hopefully many to come.

v3:
* No changes.
v2:
* Only define "nonnull" for GCC and CLANG.
* Append _param/_params to prepare for possible future attributes
  attached directly to the individual parameters, like __rte_unused.
* Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints about
  GCC_VERSION being undefined.
* Try to fix Doxygen compliants.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 lib/eal/arm/include/rte_memcpy_32.h |  8 ++++++++
 lib/eal/arm/include/rte_memcpy_64.h |  6 ++++++
 lib/eal/include/rte_common.h        | 29 +++++++++++++++++++++++++++++
 lib/eal/ppc/include/rte_memcpy.h    |  3 +++
 lib/eal/x86/include/rte_memcpy.h    |  6 ++++++
 5 files changed, 52 insertions(+)

diff --git a/lib/eal/arm/include/rte_memcpy_32.h b/lib/eal/arm/include/rte_memcpy_32.h
index fb3245b59c..f454c06eca 100644
--- a/lib/eal/arm/include/rte_memcpy_32.h
+++ b/lib/eal/arm/include/rte_memcpy_32.h
@@ -14,6 +14,8 @@ extern "C" {
 
 #include "generic/rte_memcpy.h"
 
+#include <rte_common.h>
+
 #ifdef RTE_ARCH_ARM_NEON_MEMCPY
 
 #ifndef __ARM_NEON
@@ -125,6 +127,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy((dst), (src), (n)) :          \
 	rte_memcpy_func((dst), (src), (n)); })
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)
 {
@@ -290,6 +295,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy(dst, src, 256);
 }
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static inline void *
 rte_memcpy(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/arm/include/rte_memcpy_64.h b/lib/eal/arm/include/rte_memcpy_64.h
index 85ad587bd3..55cbe07733 100644
--- a/lib/eal/arm/include/rte_memcpy_64.h
+++ b/lib/eal/arm/include/rte_memcpy_64.h
@@ -282,6 +282,9 @@ void rte_memcpy_ge64(uint8_t *dst, const uint8_t *src, size_t n)
 }
 
 #if RTE_CACHE_LINE_SIZE >= 128
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline
 void *rte_memcpy(void *dst, const void *src, size_t n)
 {
@@ -303,6 +306,9 @@ void *rte_memcpy(void *dst, const void *src, size_t n)
 }
 
 #else
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline
 void *rte_memcpy(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 15765b408d..6e4011aa85 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -149,6 +149,35 @@ typedef uint16_t unaligned_uint16_t;
 	__attribute__((format(printf, format_index, first_arg)))
 #endif
 
+/**
+ * Check pointer arguments at compile-time.
+ *
+ * @param ...
+ *    Comma separated list of parameter indexes of pointer arguments.
+ */
+#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
+#define __rte_nonnull_params(...) \
+	__attribute__((nonnull(__VA_ARGS__)))
+#else
+#define __rte_nonnull_params(...)
+#endif
+
+/**
+ * Tells compiler about the access mode of a pointer argument.
+ *
+ * @param access_mode
+ *    Access mode: read_only, read_write, write_only, or none.
+ * @param ...
+ *    Parameter index of pointer argument.
+ *    Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_access_param(access_mode, ...) \
+	__attribute__((access(access_mode, __VA_ARGS__)))
+#else
+#define __rte_access_param(access_mode, ...)
+#endif
+
 /**
  * Tells compiler that the function returns a value that points to
  * memory, where the size is given by the one or two arguments.
diff --git a/lib/eal/ppc/include/rte_memcpy.h b/lib/eal/ppc/include/rte_memcpy.h
index 6f388c0234..59a5d86948 100644
--- a/lib/eal/ppc/include/rte_memcpy.h
+++ b/lib/eal/ppc/include/rte_memcpy.h
@@ -84,6 +84,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy((dst), (src), (n)) :          \
 	rte_memcpy_func((dst), (src), (n)); })
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/x86/include/rte_memcpy.h b/lib/eal/x86/include/rte_memcpy.h
index d4d7a5cfc8..2e7161e4e7 100644
--- a/lib/eal/x86/include/rte_memcpy.h
+++ b/lib/eal/x86/include/rte_memcpy.h
@@ -42,6 +42,9 @@ extern "C" {
  * @return
  *   Pointer to the destination data.
  */
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n);
 
@@ -859,6 +862,9 @@ rte_memcpy_aligned(void *dst, const void *src, size_t n)
 	return ret;
 }
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n)
 {
-- 
2.17.1

[PATCH v3 2/2] net/bnx2x: fix warnings about rte_memcopy lengths

[Morten Brørup]

Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
VLAN ID, so only copy 2 byte, not 4. The target structure has padding
after the field, so copying 2 byte too many is effectively harmless.
There is no need to backport this patch.

Added type casts where copying arrays to the offset of a first field in a
structure holding multiple fields, to avoid compiler warnings.

Bugzilla ID: 1146

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

v3:
* First patch in series.
---
 drivers/net/bnx2x/bnx2x_stats.c | 6 +++---
 drivers/net/bnx2x/bnx2x_vfpf.c  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x_stats.c b/drivers/net/bnx2x/bnx2x_stats.c
index c07b01510a..a5a7aa3b40 100644
--- a/drivers/net/bnx2x/bnx2x_stats.c
+++ b/drivers/net/bnx2x/bnx2x_stats.c
@@ -819,7 +819,7 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
 
     rte_memcpy(old, new, sizeof(struct nig_stats));
 
-    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
+    rte_memcpy((struct mac_stx *)&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
 	   sizeof(struct mac_stx));
     estats->brb_drop_hi = pstats->brb_drop_hi;
     estats->brb_drop_lo = pstats->brb_drop_lo;
@@ -1492,9 +1492,9 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
 		REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
 	if (!CHIP_IS_E3(sc)) {
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
+				(uint32_t *)&(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
+				(uint32_t *)&(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
 	}
 
 	/* function stats */
diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfpf.c
index 63953c2979..87631c76ca 100644
--- a/drivers/net/bnx2x/bnx2x_vfpf.c
+++ b/drivers/net/bnx2x/bnx2x_vfpf.c
@@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
 	if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac, sc->old_bulletin.mac, ETH_ALEN))
 		rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
 	if (valid_bitmap & (1 << VLAN_VALID))
-		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
+		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, sizeof(bull->vlan));
 
 	sc->old_bulletin = *bull;
 
-- 
2.17.1

[PATCH v4 1/2] eal: add nonnull and access function attributes

[Morten Brørup]

Add "nonnull" function attribute to help the compiler detect a NULL
pointer being passed to a function not accepting NULL pointers as an
argument at build time.

Add "access" function attribute to tell the compiler how a function
accesses its pointer arguments.

Add these attributes to the rte_memcpy() function, as the first in
hopefully many to come.

v4:
* No changes.
v3:
* No changes.
v2:
* Only define "nonnull" for GCC and CLANG.
* Append _param/_params to prepare for possible future attributes
  attached directly to the individual parameters, like __rte_unused.
* Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints about
  GCC_VERSION being undefined.
* Try to fix Doxygen compliants.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 lib/eal/arm/include/rte_memcpy_32.h |  8 ++++++++
 lib/eal/arm/include/rte_memcpy_64.h |  6 ++++++
 lib/eal/include/rte_common.h        | 29 +++++++++++++++++++++++++++++
 lib/eal/ppc/include/rte_memcpy.h    |  3 +++
 lib/eal/x86/include/rte_memcpy.h    |  6 ++++++
 5 files changed, 52 insertions(+)

diff --git a/lib/eal/arm/include/rte_memcpy_32.h b/lib/eal/arm/include/rte_memcpy_32.h
index fb3245b59c..f454c06eca 100644
--- a/lib/eal/arm/include/rte_memcpy_32.h
+++ b/lib/eal/arm/include/rte_memcpy_32.h
@@ -14,6 +14,8 @@ extern "C" {
 
 #include "generic/rte_memcpy.h"
 
+#include <rte_common.h>
+
 #ifdef RTE_ARCH_ARM_NEON_MEMCPY
 
 #ifndef __ARM_NEON
@@ -125,6 +127,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy((dst), (src), (n)) :          \
 	rte_memcpy_func((dst), (src), (n)); })
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)
 {
@@ -290,6 +295,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy(dst, src, 256);
 }
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static inline void *
 rte_memcpy(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/arm/include/rte_memcpy_64.h b/lib/eal/arm/include/rte_memcpy_64.h
index 85ad587bd3..55cbe07733 100644
--- a/lib/eal/arm/include/rte_memcpy_64.h
+++ b/lib/eal/arm/include/rte_memcpy_64.h
@@ -282,6 +282,9 @@ void rte_memcpy_ge64(uint8_t *dst, const uint8_t *src, size_t n)
 }
 
 #if RTE_CACHE_LINE_SIZE >= 128
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline
 void *rte_memcpy(void *dst, const void *src, size_t n)
 {
@@ -303,6 +306,9 @@ void *rte_memcpy(void *dst, const void *src, size_t n)
 }
 
 #else
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline
 void *rte_memcpy(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 15765b408d..6e4011aa85 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -149,6 +149,35 @@ typedef uint16_t unaligned_uint16_t;
 	__attribute__((format(printf, format_index, first_arg)))
 #endif
 
+/**
+ * Check pointer arguments at compile-time.
+ *
+ * @param ...
+ *    Comma separated list of parameter indexes of pointer arguments.
+ */
+#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
+#define __rte_nonnull_params(...) \
+	__attribute__((nonnull(__VA_ARGS__)))
+#else
+#define __rte_nonnull_params(...)
+#endif
+
+/**
+ * Tells compiler about the access mode of a pointer argument.
+ *
+ * @param access_mode
+ *    Access mode: read_only, read_write, write_only, or none.
+ * @param ...
+ *    Parameter index of pointer argument.
+ *    Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_access_param(access_mode, ...) \
+	__attribute__((access(access_mode, __VA_ARGS__)))
+#else
+#define __rte_access_param(access_mode, ...)
+#endif
+
 /**
  * Tells compiler that the function returns a value that points to
  * memory, where the size is given by the one or two arguments.
diff --git a/lib/eal/ppc/include/rte_memcpy.h b/lib/eal/ppc/include/rte_memcpy.h
index 6f388c0234..59a5d86948 100644
--- a/lib/eal/ppc/include/rte_memcpy.h
+++ b/lib/eal/ppc/include/rte_memcpy.h
@@ -84,6 +84,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy((dst), (src), (n)) :          \
 	rte_memcpy_func((dst), (src), (n)); })
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/x86/include/rte_memcpy.h b/lib/eal/x86/include/rte_memcpy.h
index d4d7a5cfc8..2e7161e4e7 100644
--- a/lib/eal/x86/include/rte_memcpy.h
+++ b/lib/eal/x86/include/rte_memcpy.h
@@ -42,6 +42,9 @@ extern "C" {
  * @return
  *   Pointer to the destination data.
  */
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n);
 
@@ -859,6 +862,9 @@ rte_memcpy_aligned(void *dst, const void *src, size_t n)
 	return ret;
 }
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n)
 {
-- 
2.17.1

[PATCH v4 2/2] net/bnx2x: fix warnings about rte_memcpy lengths

[Morten Brørup]

Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
VLAN ID, so only copy 2 byte, not 4. The target structure has padding
after the field, so copying 2 byte too many is effectively harmless.
There is no need to backport this patch.

Added type casts where copying arrays to the offset of a first field in a
structure holding multiple fields, to avoid compiler warnings.

Bugzilla ID: 1146

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

v4:
* Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
v3:
* First patch in series.
---
 drivers/net/bnx2x/bnx2x_stats.c | 11 +++++++----
 drivers/net/bnx2x/bnx2x_vfpf.c  |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x_stats.c b/drivers/net/bnx2x/bnx2x_stats.c
index c07b01510a..1c44504663 100644
--- a/drivers/net/bnx2x/bnx2x_stats.c
+++ b/drivers/net/bnx2x/bnx2x_stats.c
@@ -819,8 +819,9 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
 
     rte_memcpy(old, new, sizeof(struct nig_stats));
 
-    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
-	   sizeof(struct mac_stx));
+    rte_memcpy(RTE_PTR_ADD(estats,
+	   offsetof(struct bnx2x_eth_stats, rx_stat_ifhcinbadoctets_hi)),
+	   &(pstats->mac_stx[1]), sizeof(struct mac_stx));
     estats->brb_drop_hi = pstats->brb_drop_hi;
     estats->brb_drop_lo = pstats->brb_drop_lo;
 
@@ -1492,9 +1493,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
 		REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
 	if (!CHIP_IS_E3(sc)) {
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
+				RTE_PTR_ADD(&(sc->port.old_nig_stats),
+				offsetof(struct nig_stats, egress_mac_pkt0_lo)), 2);
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
+				RTE_PTR_ADD(&(sc->port.old_nig_stats),
+				offsetof(struct nig_stats, egress_mac_pkt1_lo)), 2);
 	}
 
 	/* function stats */
diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfpf.c
index 63953c2979..87631c76ca 100644
--- a/drivers/net/bnx2x/bnx2x_vfpf.c
+++ b/drivers/net/bnx2x/bnx2x_vfpf.c
@@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
 	if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac, sc->old_bulletin.mac, ETH_ALEN))
 		rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
 	if (valid_bitmap & (1 << VLAN_VALID))
-		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
+		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, sizeof(bull->vlan));
 
 	sc->old_bulletin = *bull;
 
-- 
2.17.1

[PATCH v5 1/4] eal: add nonnull and access function attributes

[Morten Brørup]

Add "nonnull" function attribute to help the compiler detect a NULL
pointer being passed to a function not accepting NULL pointers as an
argument at build time.

Add "access" function attribute to tell the compiler how a function
accesses its pointer arguments.

Add these attributes to the rte_memcpy() function, as the first in
hopefully many to come.

v5:
* No changes.
v4:
* No changes.
v3:
* No changes.
v2:
* Only define "nonnull" for GCC and CLANG.
* Append _param/_params to prepare for possible future attributes
  attached directly to the individual parameters, like __rte_unused.
* Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints about
  GCC_VERSION being undefined.
* Try to fix Doxygen compliants.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 lib/eal/arm/include/rte_memcpy_32.h |  8 ++++++++
 lib/eal/arm/include/rte_memcpy_64.h |  6 ++++++
 lib/eal/include/rte_common.h        | 29 +++++++++++++++++++++++++++++
 lib/eal/ppc/include/rte_memcpy.h    |  3 +++
 lib/eal/x86/include/rte_memcpy.h    |  6 ++++++
 5 files changed, 52 insertions(+)

diff --git a/lib/eal/arm/include/rte_memcpy_32.h b/lib/eal/arm/include/rte_memcpy_32.h
index fb3245b59c..f454c06eca 100644
--- a/lib/eal/arm/include/rte_memcpy_32.h
+++ b/lib/eal/arm/include/rte_memcpy_32.h
@@ -14,6 +14,8 @@ extern "C" {
 
 #include "generic/rte_memcpy.h"
 
+#include <rte_common.h>
+
 #ifdef RTE_ARCH_ARM_NEON_MEMCPY
 
 #ifndef __ARM_NEON
@@ -125,6 +127,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy((dst), (src), (n)) :          \
 	rte_memcpy_func((dst), (src), (n)); })
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)
 {
@@ -290,6 +295,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy(dst, src, 256);
 }
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static inline void *
 rte_memcpy(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/arm/include/rte_memcpy_64.h b/lib/eal/arm/include/rte_memcpy_64.h
index 85ad587bd3..55cbe07733 100644
--- a/lib/eal/arm/include/rte_memcpy_64.h
+++ b/lib/eal/arm/include/rte_memcpy_64.h
@@ -282,6 +282,9 @@ void rte_memcpy_ge64(uint8_t *dst, const uint8_t *src, size_t n)
 }
 
 #if RTE_CACHE_LINE_SIZE >= 128
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline
 void *rte_memcpy(void *dst, const void *src, size_t n)
 {
@@ -303,6 +306,9 @@ void *rte_memcpy(void *dst, const void *src, size_t n)
 }
 
 #else
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline
 void *rte_memcpy(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 15765b408d..6e4011aa85 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -149,6 +149,35 @@ typedef uint16_t unaligned_uint16_t;
 	__attribute__((format(printf, format_index, first_arg)))
 #endif
 
+/**
+ * Check pointer arguments at compile-time.
+ *
+ * @param ...
+ *    Comma separated list of parameter indexes of pointer arguments.
+ */
+#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
+#define __rte_nonnull_params(...) \
+	__attribute__((nonnull(__VA_ARGS__)))
+#else
+#define __rte_nonnull_params(...)
+#endif
+
+/**
+ * Tells compiler about the access mode of a pointer argument.
+ *
+ * @param access_mode
+ *    Access mode: read_only, read_write, write_only, or none.
+ * @param ...
+ *    Parameter index of pointer argument.
+ *    Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_access_param(access_mode, ...) \
+	__attribute__((access(access_mode, __VA_ARGS__)))
+#else
+#define __rte_access_param(access_mode, ...)
+#endif
+
 /**
  * Tells compiler that the function returns a value that points to
  * memory, where the size is given by the one or two arguments.
diff --git a/lib/eal/ppc/include/rte_memcpy.h b/lib/eal/ppc/include/rte_memcpy.h
index 6f388c0234..59a5d86948 100644
--- a/lib/eal/ppc/include/rte_memcpy.h
+++ b/lib/eal/ppc/include/rte_memcpy.h
@@ -84,6 +84,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy((dst), (src), (n)) :          \
 	rte_memcpy_func((dst), (src), (n)); })
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/x86/include/rte_memcpy.h b/lib/eal/x86/include/rte_memcpy.h
index d4d7a5cfc8..2e7161e4e7 100644
--- a/lib/eal/x86/include/rte_memcpy.h
+++ b/lib/eal/x86/include/rte_memcpy.h
@@ -42,6 +42,9 @@ extern "C" {
  * @return
  *   Pointer to the destination data.
  */
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n);
 
@@ -859,6 +862,9 @@ rte_memcpy_aligned(void *dst, const void *src, size_t n)
 	return ret;
 }
 
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n)
 {
-- 
2.17.1

[PATCH v5 2/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Morten Brørup]

Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
VLAN ID, so only copy 2 byte, not 4. The target structure has padding
after the field, so copying 2 byte too many is effectively harmless.
There is no need to backport this patch.

Use RTE_PTR_ADD where copying arrays to the offset of a first field in a
structure holding multiple fields, to avoid compiler warnings.

Bugzilla ID: 1146

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

v5:
* No changes.
v4:
* Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
v3:
* First patch in series.
---
 drivers/net/bnx2x/bnx2x_stats.c | 11 +++++++----
 drivers/net/bnx2x/bnx2x_vfpf.c  |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x_stats.c b/drivers/net/bnx2x/bnx2x_stats.c
index c07b01510a..1c44504663 100644
--- a/drivers/net/bnx2x/bnx2x_stats.c
+++ b/drivers/net/bnx2x/bnx2x_stats.c
@@ -819,8 +819,9 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
 
     rte_memcpy(old, new, sizeof(struct nig_stats));
 
-    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
-	   sizeof(struct mac_stx));
+    rte_memcpy(RTE_PTR_ADD(estats,
+	   offsetof(struct bnx2x_eth_stats, rx_stat_ifhcinbadoctets_hi)),
+	   &(pstats->mac_stx[1]), sizeof(struct mac_stx));
     estats->brb_drop_hi = pstats->brb_drop_hi;
     estats->brb_drop_lo = pstats->brb_drop_lo;
 
@@ -1492,9 +1493,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
 		REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
 	if (!CHIP_IS_E3(sc)) {
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
+				RTE_PTR_ADD(&(sc->port.old_nig_stats),
+				offsetof(struct nig_stats, egress_mac_pkt0_lo)), 2);
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
+				RTE_PTR_ADD(&(sc->port.old_nig_stats),
+				offsetof(struct nig_stats, egress_mac_pkt1_lo)), 2);
 	}
 
 	/* function stats */
diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfpf.c
index 63953c2979..87631c76ca 100644
--- a/drivers/net/bnx2x/bnx2x_vfpf.c
+++ b/drivers/net/bnx2x/bnx2x_vfpf.c
@@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
 	if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac, sc->old_bulletin.mac, ETH_ALEN))
 		rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
 	if (valid_bitmap & (1 << VLAN_VALID))
-		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
+		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, sizeof(bull->vlan));
 
 	sc->old_bulletin = *bull;
 
-- 
2.17.1

[PATCH v5 3/4] event/dlb2: remove superfluous rte_memcpy

[Morten Brørup]

Copying with the same src and dst address has no effect; removed to
avoid compiler warning.

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

v5:
* First patch in series.
---
 drivers/event/dlb2/dlb2.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
index 60c5cd4804..03d32c779f 100644
--- a/drivers/event/dlb2/dlb2.c
+++ b/drivers/event/dlb2/dlb2.c
@@ -215,7 +215,6 @@ static int
 dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
 {
 	struct dlb2_hw_dev *handle = &dlb2->qm_instance;
-	struct dlb2_hw_resource_info *dlb2_info = &handle->info;
 	int num_ldb_ports;
 	int ret;
 
@@ -277,8 +276,6 @@ dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
 	handle->info.hw_rsrc_max.reorder_window_size =
 		dlb2->hw_rsrc_query_results.num_hist_list_entries;
 
-	rte_memcpy(dlb2_info, &handle->info.hw_rsrc_max, sizeof(*dlb2_info));
-
 	return 0;
 }
 
-- 
2.17.1

[PATCH v5 4/4] net/mlx5: fix warning about rte_memcpy length

[Morten Brørup]

Use RTE_PTR_ADD where copying to the offset of a field in a structure
holding multiple fields, to avoid compiler warnings.

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

v5:
* First patch in series.
---
 drivers/net/mlx5/mlx5_flow_dv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 62c38b87a1..dd9f5fda1a 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -5662,7 +5662,7 @@ flow_dv_modify_create_cb(void *tool_ctx, void *cb_ctx)
 				   "cannot allocate resource memory");
 		return NULL;
 	}
-	rte_memcpy(&entry->ft_type,
+	rte_memcpy(RTE_PTR_ADD(entry, offsetof(typeof(*entry), ft_type)),
 		   RTE_PTR_ADD(ref, offsetof(typeof(*ref), ft_type)),
 		   key_len + data_len);
 	if (entry->ft_type == MLX5DV_FLOW_TABLE_TYPE_FDB)
-- 
2.17.1

Re: [PATCH v5 2/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Stanisław Kardach]

[-- Attachment #1: Type: text/plain, Size: 3315 bytes --]

On Wed, Dec 28, 2022, 16:10 Morten Brørup <mb@smartsharesystems.com> wrote:

> Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
> VLAN ID, so only copy 2 byte, not 4. The target structure has padding
> after the field, so copying 2 byte too many is effectively harmless.
>
It is a small nitpick but why use rte_memcpy for a 2 byte / half-word copy?
Shouldn't assignment with casts be enough?

> There is no need to backport this patch.
>
> Use RTE_PTR_ADD where copying arrays to the offset of a first field in a
> structure holding multiple fields, to avoid compiler warnings.
>
> Bugzilla ID: 1146
>
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
>
> v5:
> * No changes.
> v4:
> * Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
> v3:
> * First patch in series.
> ---
>  drivers/net/bnx2x/bnx2x_stats.c | 11 +++++++----
>  drivers/net/bnx2x/bnx2x_vfpf.c  |  2 +-
>  2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/bnx2x/bnx2x_stats.c
> b/drivers/net/bnx2x/bnx2x_stats.c
> index c07b01510a..1c44504663 100644
> --- a/drivers/net/bnx2x/bnx2x_stats.c
> +++ b/drivers/net/bnx2x/bnx2x_stats.c
> @@ -819,8 +819,9 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
>
>      rte_memcpy(old, new, sizeof(struct nig_stats));
>
> -    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi),
> &(pstats->mac_stx[1]),
> -          sizeof(struct mac_stx));
> +    rte_memcpy(RTE_PTR_ADD(estats,
> +          offsetof(struct bnx2x_eth_stats, rx_stat_ifhcinbadoctets_hi)),
> +          &(pstats->mac_stx[1]), sizeof(struct mac_stx));
>      estats->brb_drop_hi = pstats->brb_drop_hi;
>      estats->brb_drop_lo = pstats->brb_drop_lo;
>
> @@ -1492,9 +1493,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
>                 REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
>         if (!CHIP_IS_E3(sc)) {
>                 REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
> -
>  &(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
> +                               RTE_PTR_ADD(&(sc->port.old_nig_stats),
> +                               offsetof(struct nig_stats,
> egress_mac_pkt0_lo)), 2);
>                 REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
> -
>  &(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
> +                               RTE_PTR_ADD(&(sc->port.old_nig_stats),
> +                               offsetof(struct nig_stats,
> egress_mac_pkt1_lo)), 2);
>         }
>
>         /* function stats */
> diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c
> b/drivers/net/bnx2x/bnx2x_vfpf.c
> index 63953c2979..87631c76ca 100644
> --- a/drivers/net/bnx2x/bnx2x_vfpf.c
> +++ b/drivers/net/bnx2x/bnx2x_vfpf.c
> @@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
>         if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac,
> sc->old_bulletin.mac, ETH_ALEN))
>                 rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
>         if (valid_bitmap & (1 << VLAN_VALID))
> -               rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan,
> RTE_VLAN_HLEN);
> +               rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan,
> sizeof(bull->vlan));
>
>         sc->old_bulletin = *bull;
>
> --
> 2.17.1
>
>

[-- Attachment #2: Type: text/html, Size: 4379 bytes --]

RE: [PATCH v5 2/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Morten Brørup]

From: Stanisław Kardach [mailto:kda@semihalf.com] 
Sent: Wednesday, 28 December 2022 17.14
> On Wed, Dec 28, 2022, 16:10 Morten Brørup <mb@smartsharesystems.com> wrote:
> > Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
> > VLAN ID, so only copy 2 byte, not 4. The target structure has padding
> > after the field, so copying 2 byte too many is effectively harmless.
> It is a small nitpick but why use rte_memcpy for a 2 byte / half-word copy? Shouldn't assignment with casts be enough?

Absolutely. It would also have prevented the bug to begin with.
But in order to keep the changes minimal, I kept the rte_memcpy().

> > There is no need to backport this patch.
> > 
> > Use RTE_PTR_ADD where copying arrays to the offset of a first field in a
> > structure holding multiple fields, to avoid compiler warnings.
> > 
> > Bugzilla ID: 1146
> > 
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > 
> > v5:
> > * No changes.
> > v4:
> > * Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
> > v3:
> > * First patch in series.
> > ---
> >  drivers/net/bnx2x/bnx2x_stats.c | 11 +++++++----
> >  drivers/net/bnx2x/bnx2x_vfpf.c  |  2 +-
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/bnx2x/bnx2x_stats.c b/drivers/net/bnx2x/bnx2x_stats.c
> > index c07b01510a..1c44504663 100644
> > --- a/drivers/net/bnx2x/bnx2x_stats.c
> > +++ b/drivers/net/bnx2x/bnx2x_stats.c
> > @@ -819,8 +819,9 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
> > 
> >      rte_memcpy(old, new, sizeof(struct nig_stats));
> > 
> > -    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
> > -          sizeof(struct mac_stx));
> > +    rte_memcpy(RTE_PTR_ADD(estats,
> > +          offsetof(struct bnx2x_eth_stats, rx_stat_ifhcinbadoctets_hi)),
> > +          &(pstats->mac_stx[1]), sizeof(struct mac_stx));
> >      estats->brb_drop_hi = pstats->brb_drop_hi;
> >      estats->brb_drop_lo = pstats->brb_drop_lo;
> > 
> > @@ -1492,9 +1493,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
> >                 REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
> >         if (!CHIP_IS_E3(sc)) {
> >                 REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
> > -                               &(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
> > +                               RTE_PTR_ADD(&(sc->port.old_nig_stats),
> > +                               offsetof(struct nig_stats, egress_mac_pkt0_lo)), 2);
> >                 REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
> > -                               &(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
> > +                               RTE_PTR_ADD(&(sc->port.old_nig_stats),
> > +                               offsetof(struct nig_stats, egress_mac_pkt1_lo)), 2);
> >         }
> > 
> >         /* function stats */
> > diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfpf.c
> > index 63953c2979..87631c76ca 100644
> > --- a/drivers/net/bnx2x/bnx2x_vfpf.c
> > +++ b/drivers/net/bnx2x/bnx2x_vfpf.c
> > @@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
> >         if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac, sc->old_bulletin.mac, ETH_ALEN))
> >                 rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
> >         if (valid_bitmap & (1 << VLAN_VALID))
> > -               rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
> > +               rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, sizeof(bull->vlan));

As Stanisław mentioned, this might as well be:

        if (valid_bitmap & (1 << VLAN_VALID))
-               rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
+               bull->vlan = sc->old_bulletin.vlan;

> > 
> >         sc->old_bulletin = *bull;
> > 
> > -- 
> > 2.17.1

Re: [PATCH v5 3/4] event/dlb2: remove superfluous rte_memcpy

[Stephen Hemminger]

On Wed, 28 Dec 2022 16:10:18 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:

> Copying with the same src and dst address has no effect; removed to
> avoid compiler warning.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> 
> v5:
> * First patch in series.
> ---
>  drivers/event/dlb2/dlb2.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
> index 60c5cd4804..03d32c779f 100644
> --- a/drivers/event/dlb2/dlb2.c
> +++ b/drivers/event/dlb2/dlb2.c
> @@ -215,7 +215,6 @@ static int
>  dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
>  {
>  	struct dlb2_hw_dev *handle = &dlb2->qm_instance;
> -	struct dlb2_hw_resource_info *dlb2_info = &handle->info;
>  	int num_ldb_ports;
>  	int ret;
>  
> @@ -277,8 +276,6 @@ dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
>  	handle->info.hw_rsrc_max.reorder_window_size =
>  		dlb2->hw_rsrc_query_results.num_hist_list_entries;
>  
> -	rte_memcpy(dlb2_info, &handle->info.hw_rsrc_max, sizeof(*dlb2_info));
> -
>  	return 0;
>  }

Please add Fixes and CC author.
Fixes: e7c9971a857a ("event/dlb2: add probe-time hardware init")
Cc: timothy.mcdaniel@intel.com


Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH v5 2/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Stephen Hemminger]

On Wed, 28 Dec 2022 17:38:56 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:

> From: Stanisław Kardach [mailto:kda@semihalf.com] 
> Sent: Wednesday, 28 December 2022 17.14
> > On Wed, Dec 28, 2022, 16:10 Morten Brørup <mb@smartsharesystems.com> wrote:  
> > > Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
> > > VLAN ID, so only copy 2 byte, not 4. The target structure has padding
> > > after the field, so copying 2 byte too many is effectively harmless.  
> > It is a small nitpick but why use rte_memcpy for a 2 byte / half-word copy? Shouldn't assignment with casts be enough?  
> 
> Absolutely. It would also have prevented the bug to begin with.
> But in order to keep the changes minimal, I kept the rte_memcpy().

For small fixed values compiler can optimize memcpy into one instruction.
Not so with current rte_memcpy

RE: [PATCH v5 2/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, 28 December 2022 18.03
> 
> On Wed, 28 Dec 2022 17:38:56 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > From: Stanisław Kardach [mailto:kda@semihalf.com]
> > Sent: Wednesday, 28 December 2022 17.14
> > > On Wed, Dec 28, 2022, 16:10 Morten Brørup
> <mb@smartsharesystems.com> wrote:
> > > > Bugfix: The vlan in the bulletin does not contain a VLAN header,
> only the
> > > > VLAN ID, so only copy 2 byte, not 4. The target structure has
> padding
> > > > after the field, so copying 2 byte too many is effectively
> harmless.
> > > It is a small nitpick but why use rte_memcpy for a 2 byte / half-
> word copy? Shouldn't assignment with casts be enough?
> >
> > Absolutely. It would also have prevented the bug to begin with.
> > But in order to keep the changes minimal, I kept the rte_memcpy().
> 
> For small fixed values compiler can optimize memcpy into one
> instruction.
> Not so with current rte_memcpy

Good point, Stephen.

I took another look at it, and the two byte rte_memcpy() is only used in a slow path function, so no need to optimize further.

If the maintainers disagree, and want to optimize anyway, here's a proposal:

/* check the mac address and VLAN and allocate memory if valid */
if (valid_bitmap & (1 << MAC_ADDR_VALID) && !rte_is_same_ether_addr(
		(const struct rte_ether_addr *)&bull->mac,
		(const struct rte_ether_addr *)&sc->old_bulletin.mac))
	rte_ether_addr_copy((const struct rte_ether_addr *)&bull->mac,
			(struct rte_ether_addr *)&sc->link_params.mac_addr);
if (valid_bitmap & (1 << VLAN_VALID))
	bull->vlan = sc->old_bulletin.vlan;

Re: [PATCH v5 2/4] net/bnx2x: fix warnings about rte_memcpy lengths

[David Marchand]

Hello bnx2x maintainers,

On Wed, Dec 28, 2022 at 6:37 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Wednesday, 28 December 2022 18.03
> >
> > On Wed, 28 Dec 2022 17:38:56 +0100
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > > From: Stanisław Kardach [mailto:kda@semihalf.com]
> > > Sent: Wednesday, 28 December 2022 17.14
> > > > On Wed, Dec 28, 2022, 16:10 Morten Brørup
> > <mb@smartsharesystems.com> wrote:
> > > > > Bugfix: The vlan in the bulletin does not contain a VLAN header,
> > only the
> > > > > VLAN ID, so only copy 2 byte, not 4. The target structure has
> > padding
> > > > > after the field, so copying 2 byte too many is effectively
> > harmless.
> > > > It is a small nitpick but why use rte_memcpy for a 2 byte / half-
> > word copy? Shouldn't assignment with casts be enough?
> > >
> > > Absolutely. It would also have prevented the bug to begin with.
> > > But in order to keep the changes minimal, I kept the rte_memcpy().
> >
> > For small fixed values compiler can optimize memcpy into one
> > instruction.
> > Not so with current rte_memcpy
>
> Good point, Stephen.
>
> I took another look at it, and the two byte rte_memcpy() is only used in a slow path function, so no need to optimize further.
>
> If the maintainers disagree, and want to optimize anyway, here's a proposal:
>
> /* check the mac address and VLAN and allocate memory if valid */
> if (valid_bitmap & (1 << MAC_ADDR_VALID) && !rte_is_same_ether_addr(
>                 (const struct rte_ether_addr *)&bull->mac,
>                 (const struct rte_ether_addr *)&sc->old_bulletin.mac))
>         rte_ether_addr_copy((const struct rte_ether_addr *)&bull->mac,
>                         (struct rte_ether_addr *)&sc->link_params.mac_addr);
> if (valid_bitmap & (1 << VLAN_VALID))
>         bull->vlan = sc->old_bulletin.vlan;
>

Please review/advise.
Thanks.


-- 
David Marchand

Re: [PATCH v5 1/4] eal: add nonnull and access function attributes

[Thomas Monjalon]

28/12/2022 16:10, Morten Brørup:
> Add "nonnull" function attribute to help the compiler detect a NULL
> pointer being passed to a function not accepting NULL pointers as an
> argument at build time.
> 
> Add "access" function attribute to tell the compiler how a function
> accesses its pointer arguments.

Why access specification is needed?
Isn't it enough to have the const keyword?
I'm afraid we are going to make the code ugly to read with such attribute.

Re: [PATCH v5 1/4] eal: add nonnull and access function attributes

[David Marchand]

On Wed, Dec 28, 2022 at 4:10 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> Add "nonnull" function attribute to help the compiler detect a NULL
> pointer being passed to a function not accepting NULL pointers as an
> argument at build time.
>
> Add "access" function attribute to tell the compiler how a function
> accesses its pointer arguments.
>
> Add these attributes to the rte_memcpy() function, as the first in
> hopefully many to come.
>

Compilation is broken starting first patch, so patches must be
reordered to have the fixes first.


> v5:
> * No changes.
> v4:
> * No changes.
> v3:
> * No changes.
> v2:
> * Only define "nonnull" for GCC and CLANG.
> * Append _param/_params to prepare for possible future attributes
>   attached directly to the individual parameters, like __rte_unused.
> * Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints about
>   GCC_VERSION being undefined.
> * Try to fix Doxygen compliants.

Patch history should be put as annotations (i.e. after --- but before
patch content).


>
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

[snip]


> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index 15765b408d..6e4011aa85 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -149,6 +149,35 @@ typedef uint16_t unaligned_uint16_t;
>         __attribute__((format(printf, format_index, first_arg)))
>  #endif
>
> +/**
> + * Check pointer arguments at compile-time.
> + *
> + * @param ...
> + *    Comma separated list of parameter indexes of pointer arguments.
> + */
> +#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
> +#define __rte_nonnull_params(...) \
> +       __attribute__((nonnull(__VA_ARGS__)))
> +#else
> +#define __rte_nonnull_params(...)
> +#endif
> +
> +/**
> + * Tells compiler about the access mode of a pointer argument.
> + *
> + * @param access_mode
> + *    Access mode: read_only, read_write, write_only, or none.
> + * @param ...
> + *    Parameter index of pointer argument.
> + *    Optionally followeded by comma and parameter index of size argument.
> + */
> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
> +#define __rte_access_param(access_mode, ...) \
> +       __attribute__((access(access_mode, __VA_ARGS__)))
> +#else
> +#define __rte_access_param(access_mode, ...)
> +#endif
> +

This is tightly bound to gcc syntax.
With dedicated macros (which I find easier to read too), we can hope
to adapt to other compilers if some of them add support for this kind
of code cookies.
__rte_read_only_params(indexes...)
__rte_write_only_params(indexes...)
__rte_no_access_params(indexes...)


-- 
David Marchand

RE: [PATCH v5 1/4] eal: add nonnull and access function attributes

[Morten Brørup]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, 9 January 2023 12.09
> 
> 28/12/2022 16:10, Morten Brørup:
> > Add "nonnull" function attribute to help the compiler detect a NULL
> > pointer being passed to a function not accepting NULL pointers as an
> > argument at build time.
> >
> > Add "access" function attribute to tell the compiler how a function
> > accesses its pointer arguments.
> 
> Why access specification is needed?
> Isn't it enough to have the const keyword?

No, it the const keyword is not enough. The read_only access attribute implies a stronger guarantee than the const qualifier which, when cast away from a pointer, does not prevent the pointed-to object from being modified [1].

[1]: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

These attributes help finding bugs at build time (which I consider rather important). Proof: The other patches in the series are bugs revealed by using this attribute - bugs that were completely undetected without these attributes.

I guess they might also somehow help the optimizer, similar to "const" and "restrict". I noticed that we define __rte_weak, but not __rte_pure, so perhaps I should add that too?

> I'm afraid we are going to make the code ugly to read with such
> attribute.

The same can be said about ASAN. Ugly, but helpful.

In the extreme, the same could be said about the "const" and "restrict" keywords... The more decoration on the functions, the uglier the code. :-)


If you prefer, I could split the __rte_access_param(access_mode, ...) up into one macro per access mode, so this:

+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)

would be shortened to this instead:

+__rte_params_nonnull(1, 2)
+__rte_param_writeonly(1, 3)
+__rte_param_readonly(2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)

or something else.

But I don't think it's the improvement you are hoping for. In essence, it is only a change of the names of the macros.

Please note: I prefer keeping the word "params" in the _nonnull macro, so we are prepared for defining an __rte_nonnull macro (without parameters) to be used with function parameters like the C22 NonNull keyword. Keeping "param" in the name(s) of the access macro(s) would ensure similar future proofing. That, and the similarity with the __rte_nonnull_params() name was the reason for the access macro name to include "param".

RE: [PATCH v5 1/4] eal: add nonnull and access function attributes

[Morten Brørup]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Monday, 9 January 2023 12.22
> attributes
> 
> On Wed, Dec 28, 2022 at 4:10 PM Morten Brørup
> <mb@smartsharesystems.com> wrote:
> >
> > Add "nonnull" function attribute to help the compiler detect a NULL
> > pointer being passed to a function not accepting NULL pointers as an
> > argument at build time.
> >
> > Add "access" function attribute to tell the compiler how a function
> > accesses its pointer arguments.
> >
> > Add these attributes to the rte_memcpy() function, as the first in
> > hopefully many to come.
> >
> 
> Compilation is broken starting first patch, so patches must be
> reordered to have the fixes first.

Will do.

> 
> 
> > v5:
> > * No changes.
> > v4:
> > * No changes.
> > v3:
> > * No changes.
> > v2:
> > * Only define "nonnull" for GCC and CLANG.
> > * Append _param/_params to prepare for possible future attributes
> >   attached directly to the individual parameters, like __rte_unused.
> > * Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints
> about
> >   GCC_VERSION being undefined.
> > * Try to fix Doxygen compliants.
> 
> Patch history should be put as annotations (i.e. after --- but before
> patch content).

Will do.

[...]

> This is tightly bound to gcc syntax.
> With dedicated macros (which I find easier to read too), we can hope
> to adapt to other compilers if some of them add support for this kind
> of code cookies.
> __rte_read_only_params(indexes...)
> __rte_write_only_params(indexes...)
> __rte_no_access_params(indexes...)

I agree. Splitting the generic access macro up into dedicated access macros is probably better.

[PATCH v6 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Morten Brørup]

Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
VLAN ID, so only copy 2 byte, not 4. The target structure has padding
after the field, so copying 2 byte too many is effectively harmless.
There is no need to backport this patch.

Use RTE_PTR_ADD where copying arrays to the offset of a first field in a
structure holding multiple fields, to avoid compiler warnings with
decorated rte_memcpy.

Bugzilla ID: 1146

Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core")
Cc: stephen@networkplumber.org
Cc: rmody@marvell.com
Cc: shshaikh@marvell.com

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v6:
* Add Fixes to patch description.
* Fix checkpatch warnings.
v5:
* No changes.
v4:
* Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
v3:
* First patch in series.
---
 drivers/net/bnx2x/bnx2x_stats.c | 11 +++++++----
 drivers/net/bnx2x/bnx2x_vfpf.c  |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x_stats.c b/drivers/net/bnx2x/bnx2x_stats.c
index c07b01510a..bc4a8b8e71 100644
--- a/drivers/net/bnx2x/bnx2x_stats.c
+++ b/drivers/net/bnx2x/bnx2x_stats.c
@@ -819,8 +819,9 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
 
     rte_memcpy(old, new, sizeof(struct nig_stats));
 
-    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
-	   sizeof(struct mac_stx));
+	rte_memcpy(RTE_PTR_ADD(estats,
+			offsetof(struct bnx2x_eth_stats, rx_stat_ifhcinbadoctets_hi)),
+			&pstats->mac_stx[1], sizeof(struct mac_stx));
     estats->brb_drop_hi = pstats->brb_drop_hi;
     estats->brb_drop_lo = pstats->brb_drop_lo;
 
@@ -1492,9 +1493,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
 		REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
 	if (!CHIP_IS_E3(sc)) {
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
+				RTE_PTR_ADD(&sc->port.old_nig_stats,
+				offsetof(struct nig_stats, egress_mac_pkt0_lo)), 2);
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
+				RTE_PTR_ADD(&sc->port.old_nig_stats,
+				offsetof(struct nig_stats, egress_mac_pkt1_lo)), 2);
 	}
 
 	/* function stats */
diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfpf.c
index 63953c2979..87631c76ca 100644
--- a/drivers/net/bnx2x/bnx2x_vfpf.c
+++ b/drivers/net/bnx2x/bnx2x_vfpf.c
@@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
 	if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac, sc->old_bulletin.mac, ETH_ALEN))
 		rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
 	if (valid_bitmap & (1 << VLAN_VALID))
-		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
+		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, sizeof(bull->vlan));
 
 	sc->old_bulletin = *bull;
 
-- 
2.17.1

[PATCH v6 2/4] event/dlb2: remove superfluous rte_memcpy

[Morten Brørup]

Copying with the same src and dst address has no effect; removed to
avoid compiler warning with decorated rte_memcpy.

Fixes: e7c9971a857a ("event/dlb2: add probe-time hardware init")
Cc: timothy.mcdaniel@intel.com

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
---
v6:
* Add Fixes to patch description. (Stephen)
v5:
* First patch in series.
---
 drivers/event/dlb2/dlb2.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
index 60c5cd4804..03d32c779f 100644
--- a/drivers/event/dlb2/dlb2.c
+++ b/drivers/event/dlb2/dlb2.c
@@ -215,7 +215,6 @@ static int
 dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
 {
 	struct dlb2_hw_dev *handle = &dlb2->qm_instance;
-	struct dlb2_hw_resource_info *dlb2_info = &handle->info;
 	int num_ldb_ports;
 	int ret;
 
@@ -277,8 +276,6 @@ dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
 	handle->info.hw_rsrc_max.reorder_window_size =
 		dlb2->hw_rsrc_query_results.num_hist_list_entries;
 
-	rte_memcpy(dlb2_info, &handle->info.hw_rsrc_max, sizeof(*dlb2_info));
-
 	return 0;
 }
 
-- 
2.17.1

[PATCH v6 3/4] net/mlx5: fix warning about rte_memcpy length

[Morten Brørup]

Use RTE_PTR_ADD where copying to the offset of a field in a structure
holding multiple fields, to avoid compiler warnings with decorated
rte_memcpy.

Fixes: 16a7dbc4f69006cc1c96ca2a2c6d3e3c51a2ff50 ("net/mlx5: make flow modify action list thread safe")
Cc: xuemingl@nvidia.com
Cc: matan@nvidia.com
Cc: viacheslavo@nvidia.com

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v6:
* Add Fixes to patch description.
v5:
* First patch in series.
---
 drivers/net/mlx5/mlx5_flow_dv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 62c38b87a1..dd9f5fda1a 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -5662,7 +5662,7 @@ flow_dv_modify_create_cb(void *tool_ctx, void *cb_ctx)
 				   "cannot allocate resource memory");
 		return NULL;
 	}
-	rte_memcpy(&entry->ft_type,
+	rte_memcpy(RTE_PTR_ADD(entry, offsetof(typeof(*entry), ft_type)),
 		   RTE_PTR_ADD(ref, offsetof(typeof(*ref), ft_type)),
 		   key_len + data_len);
 	if (entry->ft_type == MLX5DV_FLOW_TABLE_TYPE_FDB)
-- 
2.17.1

[PATCH v6 4/4] eal: add nonnull and access function attributes

[Morten Brørup]

Add nonnull function attribute to help the compiler detect a NULL
pointer being passed to a function not accepting NULL pointers as an
argument at build time.

Add access function attributes to tell the compiler how a function
accesses memory pointed to by its pointer arguments.

Add these attributes to the rte_memcpy() function, as the first in
hopefully many to come.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
v6:
* Make this the last patch in the series, putting the fixes first.
  (David)
* Split the generic access macro into dedicated macros per access mode.
  (David)
v5:
* No changes.
v4:
* No changes.
v3:
* No changes.
v2:
* Only define "nonnull" for GCC and CLANG.
* Append _param/_params to prepare for possible future attributes
  attached directly to the individual parameters, like __rte_unused.
* Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints about
  GCC_VERSION being undefined.
* Try to fix Doxygen compliants.
---
 lib/eal/arm/include/rte_memcpy_32.h |  8 +++
 lib/eal/arm/include/rte_memcpy_64.h |  6 +++
 lib/eal/include/rte_common.h        | 76 +++++++++++++++++++++++++++++
 lib/eal/ppc/include/rte_memcpy.h    |  3 ++
 lib/eal/x86/include/rte_memcpy.h    |  6 +++
 5 files changed, 99 insertions(+)

diff --git a/lib/eal/arm/include/rte_memcpy_32.h b/lib/eal/arm/include/rte_memcpy_32.h
index fb3245b59c..a625a91951 100644
--- a/lib/eal/arm/include/rte_memcpy_32.h
+++ b/lib/eal/arm/include/rte_memcpy_32.h
@@ -14,6 +14,8 @@ extern "C" {
 
 #include "generic/rte_memcpy.h"
 
+#include <rte_common.h>
+
 #ifdef RTE_ARCH_ARM_NEON_MEMCPY
 
 #ifndef __ARM_NEON
@@ -125,6 +127,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy((dst), (src), (n)) :          \
 	rte_memcpy_func((dst), (src), (n)); })
 
+__rte_nonnull_params(1, 2)
+__rte_write_only_param(1, 3)
+__rte_read_only_param(2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)
 {
@@ -290,6 +295,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy(dst, src, 256);
 }
 
+__rte_nonnull_params(1, 2)
+__rte_write_only_param(1, 3)
+__rte_read_only_param(2, 3)
 static inline void *
 rte_memcpy(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/arm/include/rte_memcpy_64.h b/lib/eal/arm/include/rte_memcpy_64.h
index 85ad587bd3..0c86237cc9 100644
--- a/lib/eal/arm/include/rte_memcpy_64.h
+++ b/lib/eal/arm/include/rte_memcpy_64.h
@@ -282,6 +282,9 @@ void rte_memcpy_ge64(uint8_t *dst, const uint8_t *src, size_t n)
 }
 
 #if RTE_CACHE_LINE_SIZE >= 128
+__rte_nonnull_params(1, 2)
+__rte_write_only_param(1, 3)
+__rte_read_only_param(2, 3)
 static __rte_always_inline
 void *rte_memcpy(void *dst, const void *src, size_t n)
 {
@@ -303,6 +306,9 @@ void *rte_memcpy(void *dst, const void *src, size_t n)
 }
 
 #else
+__rte_nonnull_params(1, 2)
+__rte_write_only_param(1, 3)
+__rte_read_only_param(2, 3)
 static __rte_always_inline
 void *rte_memcpy(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 15765b408d..9ee4d4ff6a 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -149,6 +149,82 @@ typedef uint16_t unaligned_uint16_t;
 	__attribute__((format(printf, format_index, first_arg)))
 #endif
 
+/**
+ * Tells compiler that the pointer arguments must be non-null.
+ *
+ * @param ...
+ *    Comma separated list of parameter indexes of pointer arguments.
+ */
+#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
+#define __rte_nonnull_params(...) \
+	__attribute__((nonnull(__VA_ARGS__)))
+#else
+#define __rte_nonnull_params(...)
+#endif
+
+/**
+ * Tells compiler that the memory pointed to by a pointer argument
+*  is only read, not written to, by the function.
+*  It might not be accessed at all.
+ *
+ * @param ...
+ *    Parameter index of pointer argument.
+ *    Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_read_only_param(...) \
+	__attribute__((access(read_only, __VA_ARGS__)))
+#else
+#define __rte_read_only_param(...)
+#endif
+
+/**
+ * Tells compiler that the memory pointed to by a pointer argument
+*  is only written to, not read, by the function.
+*  It might not be accessed at all.
+ *
+ * @param ...
+ *    Parameter index of pointer argument.
+ *    Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_write_only_param(...) \
+	__attribute__((access(write_only, __VA_ARGS__)))
+#else
+#define __rte_write_only_param(...)
+#endif
+
+/**
+ * Tells compiler that the memory pointed to by a pointer argument
+*  is both read and written to by the function.
+*  It might not be read, written to, or accessed at all.
+ *
+ * @param ...
+ *    Parameter index of pointer argument.
+ *    Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_read_write_param(...) \
+	__attribute__((access(read_write, __VA_ARGS__)))
+#else
+#define __rte_read_write_param(...)
+#endif
+
+/**
+ * Tells compiler that the memory pointed to by a pointer argument
+*  is not accessed by the function.
+ *
+ * @param ...
+ *    Parameter index of pointer argument.
+ *    Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_no_access_param(...) \
+	__attribute__((access(none, __VA_ARGS__)))
+#else
+#define __rte_no_access_param(...)
+#endif
+
 /**
  * Tells compiler that the function returns a value that points to
  * memory, where the size is given by the one or two arguments.
diff --git a/lib/eal/ppc/include/rte_memcpy.h b/lib/eal/ppc/include/rte_memcpy.h
index 6f388c0234..c36869a414 100644
--- a/lib/eal/ppc/include/rte_memcpy.h
+++ b/lib/eal/ppc/include/rte_memcpy.h
@@ -84,6 +84,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy((dst), (src), (n)) :          \
 	rte_memcpy_func((dst), (src), (n)); })
 
+__rte_nonnull_params(1, 2)
+__rte_write_only_param(1, 3)
+__rte_read_only_param(2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/x86/include/rte_memcpy.h b/lib/eal/x86/include/rte_memcpy.h
index d4d7a5cfc8..ee543aa37d 100644
--- a/lib/eal/x86/include/rte_memcpy.h
+++ b/lib/eal/x86/include/rte_memcpy.h
@@ -42,6 +42,9 @@ extern "C" {
  * @return
  *   Pointer to the destination data.
  */
+__rte_nonnull_params(1, 2)
+__rte_write_only_param(1, 3)
+__rte_read_only_param(2, 3)
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n);
 
@@ -859,6 +862,9 @@ rte_memcpy_aligned(void *dst, const void *src, size_t n)
 	return ret;
 }
 
+__rte_nonnull_params(1, 2)
+__rte_write_only_param(1, 3)
+__rte_read_only_param(2, 3)
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n)
 {
-- 
2.17.1

RE: [PATCH v5 1/4] eal: add nonnull and access function attributes

[Morten Brørup]

> From: Morten Brørup
> Sent: Monday, 9 January 2023 13.28
> 
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Monday, 9 January 2023 12.22
> > attributes
> >
> > On Wed, Dec 28, 2022 at 4:10 PM Morten Brørup
> > <mb@smartsharesystems.com> wrote:
> > >
> > > Add "nonnull" function attribute to help the compiler detect a NULL
> > > pointer being passed to a function not accepting NULL pointers as
> an
> > > argument at build time.
> > >
> > > Add "access" function attribute to tell the compiler how a function
> > > accesses its pointer arguments.
> > >
> > > Add these attributes to the rte_memcpy() function, as the first in
> > > hopefully many to come.
> > >
> >
> > Compilation is broken starting first patch, so patches must be
> > reordered to have the fixes first.
> 
> Will do.
> 
> >
> >
> > > v5:
> > > * No changes.
> > > v4:
> > > * No changes.
> > > v3:
> > > * No changes.
> > > v2:
> > > * Only define "nonnull" for GCC and CLANG.
> > > * Append _param/_params to prepare for possible future attributes
> > >   attached directly to the individual parameters, like
> __rte_unused.
> > > * Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints
> > about
> > >   GCC_VERSION being undefined.
> > > * Try to fix Doxygen compliants.
> >
> > Patch history should be put as annotations (i.e. after --- but before
> > patch content).
> 
> Will do.
> 
> [...]
> 
> > This is tightly bound to gcc syntax.
> > With dedicated macros (which I find easier to read too), we can hope
> > to adapt to other compilers if some of them add support for this kind
> > of code cookies.
> > __rte_read_only_params(indexes...)
> > __rte_write_only_params(indexes...)
> > __rte_no_access_params(indexes...)
> 
> I agree. Splitting the generic access macro up into dedicated access
> macros is probably better.

Version 6 of this patch series is here:
https://patchwork.dpdk.org/project/dpdk/list/?series=26560

Please ignore the name of the series.

-Morten

[PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Morten Brørup]

Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
VLAN ID, so only copy 2 byte, not 4. The target structure has padding
after the field, so copying 2 byte too many is effectively harmless.
There is no need to backport this patch.

Use RTE_PTR_ADD where copying arrays to the offset of a first field in a
structure holding multiple fields, to avoid compiler warnings with
decorated rte_memcpy.

Bugzilla ID: 1146

Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core")
Cc: stephen@networkplumber.org
Cc: rmody@marvell.com
Cc: shshaikh@marvell.com

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v7:
* No changes.
v6:
* Add Fixes to patch description.
* Fix checkpatch warnings.
v5:
* No changes.
v4:
* Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
v3:
* First patch in series.
---
 drivers/net/bnx2x/bnx2x_stats.c | 11 +++++++----
 drivers/net/bnx2x/bnx2x_vfpf.c  |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x_stats.c b/drivers/net/bnx2x/bnx2x_stats.c
index c07b01510a..bc4a8b8e71 100644
--- a/drivers/net/bnx2x/bnx2x_stats.c
+++ b/drivers/net/bnx2x/bnx2x_stats.c
@@ -819,8 +819,9 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
 
     rte_memcpy(old, new, sizeof(struct nig_stats));
 
-    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
-	   sizeof(struct mac_stx));
+	rte_memcpy(RTE_PTR_ADD(estats,
+			offsetof(struct bnx2x_eth_stats, rx_stat_ifhcinbadoctets_hi)),
+			&pstats->mac_stx[1], sizeof(struct mac_stx));
     estats->brb_drop_hi = pstats->brb_drop_hi;
     estats->brb_drop_lo = pstats->brb_drop_lo;
 
@@ -1492,9 +1493,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
 		REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
 	if (!CHIP_IS_E3(sc)) {
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
+				RTE_PTR_ADD(&sc->port.old_nig_stats,
+				offsetof(struct nig_stats, egress_mac_pkt0_lo)), 2);
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
+				RTE_PTR_ADD(&sc->port.old_nig_stats,
+				offsetof(struct nig_stats, egress_mac_pkt1_lo)), 2);
 	}
 
 	/* function stats */
diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfpf.c
index 63953c2979..87631c76ca 100644
--- a/drivers/net/bnx2x/bnx2x_vfpf.c
+++ b/drivers/net/bnx2x/bnx2x_vfpf.c
@@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
 	if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac, sc->old_bulletin.mac, ETH_ALEN))
 		rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
 	if (valid_bitmap & (1 << VLAN_VALID))
-		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
+		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, sizeof(bull->vlan));
 
 	sc->old_bulletin = *bull;
 
-- 
2.17.1

[PATCH v7 2/4] event/dlb2: remove superfluous rte_memcpy

[Morten Brørup]

Copying with the same src and dst address has no effect; removed to
avoid compiler warning with decorated rte_memcpy.

Fixes: e7c9971a857a ("event/dlb2: add probe-time hardware init")
Cc: timothy.mcdaniel@intel.com

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
---
v7:
* No changes.
v6:
* Add Fixes to patch description. (Stephen)
v5:
* First patch in series.
---
 drivers/event/dlb2/dlb2.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
index 60c5cd4804..03d32c779f 100644
--- a/drivers/event/dlb2/dlb2.c
+++ b/drivers/event/dlb2/dlb2.c
@@ -215,7 +215,6 @@ static int
 dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
 {
 	struct dlb2_hw_dev *handle = &dlb2->qm_instance;
-	struct dlb2_hw_resource_info *dlb2_info = &handle->info;
 	int num_ldb_ports;
 	int ret;
 
@@ -277,8 +276,6 @@ dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
 	handle->info.hw_rsrc_max.reorder_window_size =
 		dlb2->hw_rsrc_query_results.num_hist_list_entries;
 
-	rte_memcpy(dlb2_info, &handle->info.hw_rsrc_max, sizeof(*dlb2_info));
-
 	return 0;
 }
 
-- 
2.17.1

[PATCH v7 3/4] net/mlx5: fix warning about rte_memcpy length

[Morten Brørup]

Use RTE_PTR_ADD where copying to the offset of a field in a structure
holding multiple fields, to avoid compiler warnings with decorated
rte_memcpy.

Fixes: 16a7dbc4f69006cc1c96ca2a2c6d3e3c51a2ff50 ("net/mlx5: make flow modify action list thread safe")
Cc: xuemingl@nvidia.com
Cc: matan@nvidia.com
Cc: viacheslavo@nvidia.com

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v7:
* No changes.
v6:
* Add Fixes to patch description.
v5:
* First patch in series.
---
 drivers/net/mlx5/mlx5_flow_dv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 62c38b87a1..dd9f5fda1a 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -5662,7 +5662,7 @@ flow_dv_modify_create_cb(void *tool_ctx, void *cb_ctx)
 				   "cannot allocate resource memory");
 		return NULL;
 	}
-	rte_memcpy(&entry->ft_type,
+	rte_memcpy(RTE_PTR_ADD(entry, offsetof(typeof(*entry), ft_type)),
 		   RTE_PTR_ADD(ref, offsetof(typeof(*ref), ft_type)),
 		   key_len + data_len);
 	if (entry->ft_type == MLX5DV_FLOW_TABLE_TYPE_FDB)
-- 
2.17.1

[PATCH v7 4/4] eal: add nonnull and access function attributes

[Morten Brørup]

Add nonnull function attribute to help the compiler detect a NULL
pointer being passed to a function not accepting NULL pointers as an
argument at build time.

Add access function attributes to tell the compiler how a function
accesses memory pointed to by its pointer arguments.

Add these attributes to the rte_memcpy() function, as the first in
hopefully many to come.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
v7:
* Fix indentation. (checkpatch)
v6:
* Make this the last patch in the series, putting the fixes first.
  (David)
* Split the generic access macro into dedicated macros per access mode.
  (David)
v5:
* No changes.
v4:
* No changes.
v3:
* No changes.
v2:
* Only define "nonnull" for GCC and CLANG.
* Append _param/_params to prepare for possible future attributes
  attached directly to the individual parameters, like __rte_unused.
* Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints about
  GCC_VERSION being undefined.
* Try to fix Doxygen compliants.
---
 lib/eal/arm/include/rte_memcpy_32.h |  8 +++
 lib/eal/arm/include/rte_memcpy_64.h |  6 +++
 lib/eal/include/rte_common.h        | 76 +++++++++++++++++++++++++++++
 lib/eal/ppc/include/rte_memcpy.h    |  3 ++
 lib/eal/x86/include/rte_memcpy.h    |  6 +++
 5 files changed, 99 insertions(+)

diff --git a/lib/eal/arm/include/rte_memcpy_32.h b/lib/eal/arm/include/rte_memcpy_32.h
index fb3245b59c..a625a91951 100644
--- a/lib/eal/arm/include/rte_memcpy_32.h
+++ b/lib/eal/arm/include/rte_memcpy_32.h
@@ -14,6 +14,8 @@ extern "C" {
 
 #include "generic/rte_memcpy.h"
 
+#include <rte_common.h>
+
 #ifdef RTE_ARCH_ARM_NEON_MEMCPY
 
 #ifndef __ARM_NEON
@@ -125,6 +127,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy((dst), (src), (n)) :          \
 	rte_memcpy_func((dst), (src), (n)); })
 
+__rte_nonnull_params(1, 2)
+__rte_write_only_param(1, 3)
+__rte_read_only_param(2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)
 {
@@ -290,6 +295,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy(dst, src, 256);
 }
 
+__rte_nonnull_params(1, 2)
+__rte_write_only_param(1, 3)
+__rte_read_only_param(2, 3)
 static inline void *
 rte_memcpy(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/arm/include/rte_memcpy_64.h b/lib/eal/arm/include/rte_memcpy_64.h
index 85ad587bd3..0c86237cc9 100644
--- a/lib/eal/arm/include/rte_memcpy_64.h
+++ b/lib/eal/arm/include/rte_memcpy_64.h
@@ -282,6 +282,9 @@ void rte_memcpy_ge64(uint8_t *dst, const uint8_t *src, size_t n)
 }
 
 #if RTE_CACHE_LINE_SIZE >= 128
+__rte_nonnull_params(1, 2)
+__rte_write_only_param(1, 3)
+__rte_read_only_param(2, 3)
 static __rte_always_inline
 void *rte_memcpy(void *dst, const void *src, size_t n)
 {
@@ -303,6 +306,9 @@ void *rte_memcpy(void *dst, const void *src, size_t n)
 }
 
 #else
+__rte_nonnull_params(1, 2)
+__rte_write_only_param(1, 3)
+__rte_read_only_param(2, 3)
 static __rte_always_inline
 void *rte_memcpy(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 15765b408d..c9cd2c7496 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -149,6 +149,82 @@ typedef uint16_t unaligned_uint16_t;
 	__attribute__((format(printf, format_index, first_arg)))
 #endif
 
+/**
+ * Tells compiler that the pointer arguments must be non-null.
+ *
+ * @param ...
+ *    Comma separated list of parameter indexes of pointer arguments.
+ */
+#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
+#define __rte_nonnull_params(...) \
+	__attribute__((nonnull(__VA_ARGS__)))
+#else
+#define __rte_nonnull_params(...)
+#endif
+
+/**
+ * Tells compiler that the memory pointed to by a pointer argument
+ * is only read, not written to, by the function.
+ * It might not be accessed at all.
+ *
+ * @param ...
+ *    Parameter index of pointer argument.
+ *    Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_read_only_param(...) \
+	__attribute__((access(read_only, __VA_ARGS__)))
+#else
+#define __rte_read_only_param(...)
+#endif
+
+/**
+ * Tells compiler that the memory pointed to by a pointer argument
+ * is only written to, not read, by the function.
+ * It might not be accessed at all.
+ *
+ * @param ...
+ *    Parameter index of pointer argument.
+ *    Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_write_only_param(...) \
+	__attribute__((access(write_only, __VA_ARGS__)))
+#else
+#define __rte_write_only_param(...)
+#endif
+
+/**
+ * Tells compiler that the memory pointed to by a pointer argument
+ * is both read and written to by the function.
+ * It might not be read, written to, or accessed at all.
+ *
+ * @param ...
+ *    Parameter index of pointer argument.
+ *    Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_read_write_param(...) \
+	__attribute__((access(read_write, __VA_ARGS__)))
+#else
+#define __rte_read_write_param(...)
+#endif
+
+/**
+ * Tells compiler that the memory pointed to by a pointer argument
+ * is not accessed by the function.
+ *
+ * @param ...
+ *    Parameter index of pointer argument.
+ *    Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_no_access_param(...) \
+	__attribute__((access(none, __VA_ARGS__)))
+#else
+#define __rte_no_access_param(...)
+#endif
+
 /**
  * Tells compiler that the function returns a value that points to
  * memory, where the size is given by the one or two arguments.
diff --git a/lib/eal/ppc/include/rte_memcpy.h b/lib/eal/ppc/include/rte_memcpy.h
index 6f388c0234..c36869a414 100644
--- a/lib/eal/ppc/include/rte_memcpy.h
+++ b/lib/eal/ppc/include/rte_memcpy.h
@@ -84,6 +84,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 	memcpy((dst), (src), (n)) :          \
 	rte_memcpy_func((dst), (src), (n)); })
 
+__rte_nonnull_params(1, 2)
+__rte_write_only_param(1, 3)
+__rte_read_only_param(2, 3)
 static inline void *
 rte_memcpy_func(void *dst, const void *src, size_t n)
 {
diff --git a/lib/eal/x86/include/rte_memcpy.h b/lib/eal/x86/include/rte_memcpy.h
index d4d7a5cfc8..ee543aa37d 100644
--- a/lib/eal/x86/include/rte_memcpy.h
+++ b/lib/eal/x86/include/rte_memcpy.h
@@ -42,6 +42,9 @@ extern "C" {
  * @return
  *   Pointer to the destination data.
  */
+__rte_nonnull_params(1, 2)
+__rte_write_only_param(1, 3)
+__rte_read_only_param(2, 3)
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n);
 
@@ -859,6 +862,9 @@ rte_memcpy_aligned(void *dst, const void *src, size_t n)
 	return ret;
 }
 
+__rte_nonnull_params(1, 2)
+__rte_write_only_param(1, 3)
+__rte_read_only_param(2, 3)
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n)
 {
-- 
2.17.1

Re: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Ferruh Yigit]

On 1/16/2023 1:07 PM, Morten Brørup wrote:
> Add nonnull function attribute to help the compiler detect a NULL
> pointer being passed to a function not accepting NULL pointers as an
> argument at build time.
> 
> Add access function attributes to tell the compiler how a function
> accesses memory pointed to by its pointer arguments.
> 
> Add these attributes to the rte_memcpy() function, as the first in
> hopefully many to come.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> v7:
> * Fix indentation. (checkpatch)
> v6:
> * Make this the last patch in the series, putting the fixes first.
>   (David)
> * Split the generic access macro into dedicated macros per access mode.
>   (David)
> v5:
> * No changes.
> v4:
> * No changes.
> v3:
> * No changes.
> v2:
> * Only define "nonnull" for GCC and CLANG.
> * Append _param/_params to prepare for possible future attributes
>   attached directly to the individual parameters, like __rte_unused.
> * Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints about
>   GCC_VERSION being undefined.
> * Try to fix Doxygen compliants.
> ---
>  lib/eal/arm/include/rte_memcpy_32.h |  8 +++
>  lib/eal/arm/include/rte_memcpy_64.h |  6 +++
>  lib/eal/include/rte_common.h        | 76 +++++++++++++++++++++++++++++
>  lib/eal/ppc/include/rte_memcpy.h    |  3 ++
>  lib/eal/x86/include/rte_memcpy.h    |  6 +++
>  5 files changed, 99 insertions(+)
> 
> diff --git a/lib/eal/arm/include/rte_memcpy_32.h b/lib/eal/arm/include/rte_memcpy_32.h
> index fb3245b59c..a625a91951 100644
> --- a/lib/eal/arm/include/rte_memcpy_32.h
> +++ b/lib/eal/arm/include/rte_memcpy_32.h
> @@ -14,6 +14,8 @@ extern "C" {
>  
>  #include "generic/rte_memcpy.h"
>  
> +#include <rte_common.h>
> +
>  #ifdef RTE_ARCH_ARM_NEON_MEMCPY
>  
>  #ifndef __ARM_NEON
> @@ -125,6 +127,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
>  	memcpy((dst), (src), (n)) :          \
>  	rte_memcpy_func((dst), (src), (n)); })
>  
> +__rte_nonnull_params(1, 2)
> +__rte_write_only_param(1, 3)
> +__rte_read_only_param(2, 3)
>  static inline void *
>  rte_memcpy_func(void *dst, const void *src, size_t n)
>  {
> @@ -290,6 +295,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
>  	memcpy(dst, src, 256);
>  }
>  
> +__rte_nonnull_params(1, 2)
> +__rte_write_only_param(1, 3)
> +__rte_read_only_param(2, 3)
>  static inline void *
>  rte_memcpy(void *dst, const void *src, size_t n)
>  {
> diff --git a/lib/eal/arm/include/rte_memcpy_64.h b/lib/eal/arm/include/rte_memcpy_64.h
> index 85ad587bd3..0c86237cc9 100644
> --- a/lib/eal/arm/include/rte_memcpy_64.h
> +++ b/lib/eal/arm/include/rte_memcpy_64.h
> @@ -282,6 +282,9 @@ void rte_memcpy_ge64(uint8_t *dst, const uint8_t *src, size_t n)
>  }
>  
>  #if RTE_CACHE_LINE_SIZE >= 128
> +__rte_nonnull_params(1, 2)
> +__rte_write_only_param(1, 3)
> +__rte_read_only_param(2, 3)
>  static __rte_always_inline
>  void *rte_memcpy(void *dst, const void *src, size_t n)
>  {
> @@ -303,6 +306,9 @@ void *rte_memcpy(void *dst, const void *src, size_t n)
>  }
>  
>  #else
> +__rte_nonnull_params(1, 2)
> +__rte_write_only_param(1, 3)
> +__rte_read_only_param(2, 3)
>  static __rte_always_inline
>  void *rte_memcpy(void *dst, const void *src, size_t n)
>  {
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index 15765b408d..c9cd2c7496 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -149,6 +149,82 @@ typedef uint16_t unaligned_uint16_t;
>  	__attribute__((format(printf, format_index, first_arg)))
>  #endif
>  
> +/**
> + * Tells compiler that the pointer arguments must be non-null.
> + *
> + * @param ...
> + *    Comma separated list of parameter indexes of pointer arguments.
> + */
> +#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
> +#define __rte_nonnull_params(...) \
> +	__attribute__((nonnull(__VA_ARGS__)))
> +#else
> +#define __rte_nonnull_params(...)
> +#endif
> +

What do you think to have a namespace for macros like '__rte_param_xxx',
so name macros as:
__rte_param_nonull
__rte_param_read_only
__rte_param_write_only

No strong opinion, it just feels tidier this way

> +/**
> + * Tells compiler that the memory pointed to by a pointer argument
> + * is only read, not written to, by the function.
> + * It might not be accessed at all.
> + *
> + * @param ...
> + *    Parameter index of pointer argument.
> + *    Optionally followeded by comma and parameter index of size argument.

s/followeded/followed/

multiple occurrences below

> + */
> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
> +#define __rte_read_only_param(...) \
> +	__attribute__((access(read_only, __VA_ARGS__)))
> +#else
> +#define __rte_read_only_param(...)
> +#endif
> +
> +/**
> + * Tells compiler that the memory pointed to by a pointer argument
> + * is only written to, not read, by the function.
> + * It might not be accessed at all.
> + *
> + * @param ...
> + *    Parameter index of pointer argument.
> + *    Optionally followeded by comma and parameter index of size argument.
> + */
> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
> +#define __rte_write_only_param(...) \
> +	__attribute__((access(write_only, __VA_ARGS__)))
> +#else
> +#define __rte_write_only_param(...)
> +#endif
> +
> +/**
> + * Tells compiler that the memory pointed to by a pointer argument
> + * is both read and written to by the function.
> + * It might not be read, written to, or accessed at all.
> + *

What I understand is difference between 'read_write' access mode and
'nonnull' attribute is,
'read_write' access mode additionally checks that variable is
initialized before used in this function.
Should we document this detail, although it is gcc manual I think it
helps to clarify here.

> + * @param ...
> + *    Parameter index of pointer argument.
> + *    Optionally followeded by comma and parameter index of size argument.
> + */
> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
> +#define __rte_read_write_param(...) \
> +	__attribute__((access(read_write, __VA_ARGS__)))
> +#else
> +#define __rte_read_write_param(...)
> +#endif
> +
> +/**
> + * Tells compiler that the memory pointed to by a pointer argument
> + * is not accessed by the function.
> + *
> + * @param ...
> + *    Parameter index of pointer argument.
> + *    Optionally followeded by comma and parameter index of size argument.
> + */
> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
> +#define __rte_no_access_param(...) \
> +	__attribute__((access(none, __VA_ARGS__)))
> +#else
> +#define __rte_no_access_param(...)
> +#endif
> +
>  /**
>   * Tells compiler that the function returns a value that points to
>   * memory, where the size is given by the one or two arguments.
> diff --git a/lib/eal/ppc/include/rte_memcpy.h b/lib/eal/ppc/include/rte_memcpy.h
> index 6f388c0234..c36869a414 100644
> --- a/lib/eal/ppc/include/rte_memcpy.h
> +++ b/lib/eal/ppc/include/rte_memcpy.h
> @@ -84,6 +84,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
>  	memcpy((dst), (src), (n)) :          \
>  	rte_memcpy_func((dst), (src), (n)); })
>  
> +__rte_nonnull_params(1, 2)
> +__rte_write_only_param(1, 3)
> +__rte_read_only_param(2, 3)
>  static inline void *
>  rte_memcpy_func(void *dst, const void *src, size_t n)
>  {
> diff --git a/lib/eal/x86/include/rte_memcpy.h b/lib/eal/x86/include/rte_memcpy.h
> index d4d7a5cfc8..ee543aa37d 100644
> --- a/lib/eal/x86/include/rte_memcpy.h
> +++ b/lib/eal/x86/include/rte_memcpy.h
> @@ -42,6 +42,9 @@ extern "C" {
>   * @return
>   *   Pointer to the destination data.
>   */
> +__rte_nonnull_params(1, 2)
> +__rte_write_only_param(1, 3)
> +__rte_read_only_param(2, 3)
>  static __rte_always_inline void *
>  rte_memcpy(void *dst, const void *src, size_t n);
>  
> @@ -859,6 +862,9 @@ rte_memcpy_aligned(void *dst, const void *src, size_t n)
>  	return ret;
>  }
>  
> +__rte_nonnull_params(1, 2)
> +__rte_write_only_param(1, 3)
> +__rte_read_only_param(2, 3)
>  static __rte_always_inline void *
>  rte_memcpy(void *dst, const void *src, size_t n)
>  {

RE: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Morten Brørup]

> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> Sent: Monday, 16 January 2023 18.02
> 
> On 1/16/2023 1:07 PM, Morten Brørup wrote:
> > Add nonnull function attribute to help the compiler detect a NULL
> > pointer being passed to a function not accepting NULL pointers as an
> > argument at build time.
> >
> > Add access function attributes to tell the compiler how a function
> > accesses memory pointed to by its pointer arguments.
> >
> > Add these attributes to the rte_memcpy() function, as the first in
> > hopefully many to come.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---

[...]

> > +/**
> > + * Tells compiler that the pointer arguments must be non-null.
> > + *
> > + * @param ...
> > + *    Comma separated list of parameter indexes of pointer
> arguments.
> > + */
> > +#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
> > +#define __rte_nonnull_params(...) \
> > +	__attribute__((nonnull(__VA_ARGS__)))
> > +#else
> > +#define __rte_nonnull_params(...)
> > +#endif
> > +
> 
> What do you think to have a namespace for macros like
> '__rte_param_xxx',
> so name macros as:
> __rte_param_nonull
> __rte_param_read_only
> __rte_param_write_only
> 
> No strong opinion, it just feels tidier this way

Being a proponent of the world_country_city naming scheme myself, I would usually agree with this proposal.

However, in the future, we might add macros without _param for use along with the function parameters, e.g.:

int foo(int bar __rte_nonnull __rte_read_only);

So I decided for this order in the names (treating nonnull/access_mode as "country" and param/params as "city"), also somewhat looking at the __rte_deprecated and __rte_deprecated_msg(msg) macros.

I have no strong preference either, so if anyone does, please speak up.

Slightly related, also note this:

The nonnull macro is plural (_params), because it can take multiple pointer parameter indexes.
The access mode macros are singular (_param), because they only take one pointer parameter index, and the optional size parameter index.

I considered splitting up the access mode macros even more, making two variants of each, e.g. __rte_read_only_param(ptr_index) and __rte_read_only_param_size(ptr_index, size_index), but concluded that it would be excruciatingly verbose. The only purpose would be to reduce the risk of using them incorrectly. I decided against it, thinking that any developer clever enough to use these macros is also clever enough to understand how to use them (or at least read their parameter descriptions to learn how).

> 
> > +/**
> > + * Tells compiler that the memory pointed to by a pointer argument
> > + * is only read, not written to, by the function.
> > + * It might not be accessed at all.
> > + *
> > + * @param ...
> > + *    Parameter index of pointer argument.
> > + *    Optionally followeded by comma and parameter index of size
> argument.
> 
> s/followeded/followed/
> 
> multiple occurrences below

Good catch. Will fix this repeated typo in the next version.

> 
> > + */
> > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
> > +#define __rte_read_only_param(...) \
> > +	__attribute__((access(read_only, __VA_ARGS__)))
> > +#else
> > +#define __rte_read_only_param(...)
> > +#endif
> > +
> > +/**
> > + * Tells compiler that the memory pointed to by a pointer argument
> > + * is only written to, not read, by the function.
> > + * It might not be accessed at all.
> > + *
> > + * @param ...
> > + *    Parameter index of pointer argument.
> > + *    Optionally followeded by comma and parameter index of size
> argument.
> > + */
> > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
> > +#define __rte_write_only_param(...) \
> > +	__attribute__((access(write_only, __VA_ARGS__)))
> > +#else
> > +#define __rte_write_only_param(...)
> > +#endif
> > +
> > +/**
> > + * Tells compiler that the memory pointed to by a pointer argument
> > + * is both read and written to by the function.
> > + * It might not be read, written to, or accessed at all.
> > + *
> 
> What I understand is difference between 'read_write' access mode and
> 'nonnull' attribute is,
> 'read_write' access mode additionally checks that variable is
> initialized before used in this function.
> Should we document this detail, although it is gcc manual I think it
> helps to clarify here.

This specific behavior might be compiler dependent, so I prefer not to add it to the description.

In the same context, I added "It might not be accessed at all." (and similar) to the macro descriptions for clarification. This leaves some flexibility for the compiler. It also reflects the behavior of GCC, which inspired me to add it. Just like __attribute__((__unused__)) only means that a variable might be unused, it does not mean that it is definitely unused.

> 
> > + * @param ...
> > + *    Parameter index of pointer argument.
> > + *    Optionally followeded by comma and parameter index of size
> argument.
> > + */
> > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
> > +#define __rte_read_write_param(...) \
> > +	__attribute__((access(read_write, __VA_ARGS__)))
> > +#else
> > +#define __rte_read_write_param(...)
> > +#endif
> > +
> > +/**
> > + * Tells compiler that the memory pointed to by a pointer argument
> > + * is not accessed by the function.
> > + *
> > + * @param ...
> > + *    Parameter index of pointer argument.
> > + *    Optionally followeded by comma and parameter index of size
> argument.
> > + */
> > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
> > +#define __rte_no_access_param(...) \
> > +	__attribute__((access(none, __VA_ARGS__)))
> > +#else
> > +#define __rte_no_access_param(...)
> > +#endif
> > +

Re: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Tyler Retzlaff]

On Tue, Jan 17, 2023 at 09:19:22AM +0100, Morten Brørup wrote:
> > From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> > Sent: Monday, 16 January 2023 18.02
> > 
> > On 1/16/2023 1:07 PM, Morten Brørup wrote:
> > > Add nonnull function attribute to help the compiler detect a NULL
> > > pointer being passed to a function not accepting NULL pointers as an
> > > argument at build time.
> > >
> > > Add access function attributes to tell the compiler how a function
> > > accesses memory pointed to by its pointer arguments.
> > >
> > > Add these attributes to the rte_memcpy() function, as the first in
> > > hopefully many to come.
> > >
> > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > ---
> 
> [...]
> 
> > > +/**
> > > + * Tells compiler that the pointer arguments must be non-null.
> > > + *
> > > + * @param ...
> > > + *    Comma separated list of parameter indexes of pointer
> > arguments.
> > > + */
> > > +#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
> > > +#define __rte_nonnull_params(...) \
> > > +	__attribute__((nonnull(__VA_ARGS__)))
> > > +#else
> > > +#define __rte_nonnull_params(...)
> > > +#endif
> > > +
> > 
> > What do you think to have a namespace for macros like
> > '__rte_param_xxx',
> > so name macros as:
> > __rte_param_nonull
> > __rte_param_read_only
> > __rte_param_write_only
> > 
> > No strong opinion, it just feels tidier this way
> 
> Being a proponent of the world_country_city naming scheme myself, I would usually agree with this proposal.
> 
> However, in the future, we might add macros without _param for use along with the function parameters, e.g.:
> 
> int foo(int bar __rte_nonnull __rte_read_only);
> 
> So I decided for this order in the names (treating nonnull/access_mode as "country" and param/params as "city"), also somewhat looking at the __rte_deprecated and __rte_deprecated_msg(msg) macros.
> 
> I have no strong preference either, so if anyone does, please speak up.
> 
> Slightly related, also note this:
> 
> The nonnull macro is plural (_params), because it can take multiple pointer parameter indexes.
> The access mode macros are singular (_param), because they only take one pointer parameter index, and the optional size parameter index.
> 
> I considered splitting up the access mode macros even more, making two variants of each, e.g. __rte_read_only_param(ptr_index) and __rte_read_only_param_size(ptr_index, size_index), but concluded that it would be excruciatingly verbose. The only purpose would be to reduce the risk of using them incorrectly. I decided against it, thinking that any developer clever enough to use these macros is also clever enough to understand how to use them (or at least read their parameter descriptions to learn how).
> 

microsoft also has a tool & annotation vehicle for this type of stuff.
this discussion has caused me to wonder what happens if we would like to
add additional annotations for other tools. just load on the annotations
and expand them empty conditionally?

https://learn.microsoft.com/en-us/cpp/code-quality/using-sal-annotations-to-reduce-c-cpp-code-defects?view=msvc-170

anyway, just a thought. no serious response required here.

RE: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Morten Brørup]

+To: Thomas & David, you probably have some opinions on this too!

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Tuesday, 17 January 2023 22.17
> 
> On Tue, Jan 17, 2023 at 09:19:22AM +0100, Morten Brørup wrote:
> > > From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> > > Sent: Monday, 16 January 2023 18.02
> > >
> > > On 1/16/2023 1:07 PM, Morten Brørup wrote:
> > > > Add nonnull function attribute to help the compiler detect a NULL
> > > > pointer being passed to a function not accepting NULL pointers as
> an
> > > > argument at build time.
> > > >
> > > > Add access function attributes to tell the compiler how a
> function
> > > > accesses memory pointed to by its pointer arguments.
> > > >
> > > > Add these attributes to the rte_memcpy() function, as the first
> in
> > > > hopefully many to come.
> > > >
> > > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > ---
> >
> > [...]
> >
> > > > +/**
> > > > + * Tells compiler that the pointer arguments must be non-null.
> > > > + *
> > > > + * @param ...
> > > > + *    Comma separated list of parameter indexes of pointer
> > > arguments.
> > > > + */
> > > > +#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
> > > > +#define __rte_nonnull_params(...) \
> > > > +	__attribute__((nonnull(__VA_ARGS__)))
> > > > +#else
> > > > +#define __rte_nonnull_params(...)
> > > > +#endif
> > > > +
> > >
> > > What do you think to have a namespace for macros like
> > > '__rte_param_xxx',
> > > so name macros as:
> > > __rte_param_nonull
> > > __rte_param_read_only
> > > __rte_param_write_only
> > >
> > > No strong opinion, it just feels tidier this way
> >
> > Being a proponent of the world_country_city naming scheme myself, I
> would usually agree with this proposal.
> >
> > However, in the future, we might add macros without _param for use
> along with the function parameters, e.g.:
> >
> > int foo(int bar __rte_nonnull __rte_read_only);
> >
> > So I decided for this order in the names (treating
> nonnull/access_mode as "country" and param/params as "city"), also
> somewhat looking at the __rte_deprecated and __rte_deprecated_msg(msg)
> macros.
> >
> > I have no strong preference either, so if anyone does, please speak
> up.
> >
> > Slightly related, also note this:
> >
> > The nonnull macro is plural (_params), because it can take multiple
> pointer parameter indexes.
> > The access mode macros are singular (_param), because they only take
> one pointer parameter index, and the optional size parameter index.
> >
> > I considered splitting up the access mode macros even more, making
> two variants of each, e.g. __rte_read_only_param(ptr_index) and
> __rte_read_only_param_size(ptr_index, size_index), but concluded that
> it would be excruciatingly verbose. The only purpose would be to reduce
> the risk of using them incorrectly. I decided against it, thinking that
> any developer clever enough to use these macros is also clever enough
> to understand how to use them (or at least read their parameter
> descriptions to learn how).
> >
> 
> microsoft also has a tool & annotation vehicle for this type of stuff.
> this discussion has caused me to wonder what happens if we would like
> to
> add additional annotations for other tools. just load on the
> annotations
> and expand them empty conditionally?
> 
> https://learn.microsoft.com/en-us/cpp/code-quality/using-sal-
> annotations-to-reduce-c-cpp-code-defects?view=msvc-170
> 
> anyway, just a thought. no serious response required here.

Excellent input, Tyler!

If we want DPDK to be considered truly cross-platform, and not treat non-Linux/non-GCC as second class citizens, we need to discuss this.

Microsoft's Source Code Annotation Language (SAL) seems very good, based on its finer granularity than GCC's attributes (which in comparison seem added as an afterthought, not cleanly structured like SAL). I have only skimmed the documentation, but that is my immediate impression of it.

SAL uses a completely different syntax than GCC attributes, and Microsoft happens to also use memcpy() as an example in the documentation referred to:

void * memcpy(
   _Out_writes_bytes_all_(count) void *dest,
   _In_reads_bytes_(count) const void *src,
   size_t count
);

Going back to how we can handle this in DPDK, we can either:

1. Not annotate the functions at all, and miss out on finding the errors for us.

2. Invent our own language (or find something existing) for function headers, and use a parser to convert them to compiler specific C/C++ headers when building the code.

3a. Keep loading on attributes, with empty macros for unsupported compilers.

3b. Keep loading on attributes, with empty macros for unsupported compilers. But limit ourselves to GCC/Clang style attributes.

3c. Keep loading on attributes, with empty macros for unsupported compilers. But limit ourselves to Microsoft SAL style attributes.

3d. Keep loading on attributes, with empty macros for unsupported compilers. But limit ourselves to the most relevant attributes, using performance and/or bug detection as criteria when considering relevance.

I am strongly against both 1 and 2.

If bug detection is the primary driver, we could stick with either 3b or 3c (i.e. only target one specific build environment) and rely on the DPDK CI for detecting bugs. But then application developers would not benefit, because they don't run their code through the DPDK CI. So I am also against this.

I think 3d (keep loading on attributes, but only the most relevant ones) is the best choice.

GCC/Clang style attributes are already supported as macros prefixed by __rte, so let's not change the way we do that.

Regarding the Microsoft SAL, I suppose Microsoft already chose annotation names to avoid collisions, so we could consider using those exact names (i.e. without __rte prefix), and define empty macros for non-Microsoft compilers. This would allow using Microsoft SAL annotations directly in the DPDK code.

Re: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Stephen Hemminger]

On Wed, 18 Jan 2023 09:31:42 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:

> > > So I decided for this order in the names (treating  
> > nonnull/access_mode as "country" and param/params as "city"), also
> > somewhat looking at the __rte_deprecated and __rte_deprecated_msg(msg)
> > macros.  
> > >
> > > I have no strong preference either, so if anyone does, please speak  
> > up.  
> > >
> > > Slightly related, also note this:
> > >
> > > The nonnull macro is plural (_params), because it can take multiple  
> > pointer parameter indexes.  
> > > The access mode macros are singular (_param), because they only take  
> > one pointer parameter index, and the optional size parameter index.  
> > >
> > > I considered splitting up the access mode macros even more, making  
> > two variants of each, e.g. __rte_read_only_param(ptr_index) and
> > __rte_read_only_param_size(ptr_index, size_index), but concluded that
> > it would be excruciatingly verbose. The only purpose would be to reduce
> > the risk of using them incorrectly. I decided against it, thinking that
> > any developer clever enough to use these macros is also clever enough
> > to understand how to use them (or at least read their parameter
> > descriptions to learn how).  
> > >  
> > 
> > microsoft also has a tool & annotation vehicle for this type of stuff.
> > this discussion has caused me to wonder what happens if we would like
> > to
> > add additional annotations for other tools. just load on the
> > annotations
> > and expand them empty conditionally?
> > 
> > https://learn.microsoft.com/en-us/cpp/code-quality/using-sal-
> > annotations-to-reduce-c-cpp-code-defects?view=msvc-170
> > 
> > anyway, just a thought. no serious response required here.  
> 
> Excellent input, Tyler!
> 
> If we want DPDK to be considered truly cross-platform, and not treat non-Linux/non-GCC as second class citizens, we need to discuss this.
> 
> Microsoft's Source Code Annotation Language (SAL) seems very good, based on its finer granularity than GCC's attributes (which in comparison seem added as an afterthought, not cleanly structured like SAL). I have only skimmed the documentation, but that is my immediate impression of it.
> 
> SAL uses a completely different syntax than GCC attributes, and Microsoft happens to also use memcpy() as an example in the documentation referred to:
> 
> void * memcpy(
>    _Out_writes_bytes_all_(count) void *dest,
>    _In_reads_bytes_(count) const void *src,
>    size_t count
> );
> 
> Going back to how we can handle this in DPDK, we can either:
> 
> 1. Not annotate the functions at all, and miss out on finding the errors for us.
> 
> 2. Invent our own language (or find something existing) for function headers, and use a parser to convert them to compiler specific C/C++ headers when building the code.
> 
> 3a. Keep loading on attributes, with empty macros for unsupported compilers.
> 
> 3b. Keep loading on attributes, with empty macros for unsupported compilers. But limit ourselves to GCC/Clang style attributes.
> 
> 3c. Keep loading on attributes, with empty macros for unsupported compilers. But limit ourselves to Microsoft SAL style attributes.
> 
> 3d. Keep loading on attributes, with empty macros for unsupported compilers. But limit ourselves to the most relevant attributes, using performance and/or bug detection as criteria when considering relevance.
> 
> I am strongly against both 1 and 2.
> 
> If bug detection is the primary driver, we could stick with either 3b or 3c (i.e. only target one specific build environment) and rely on the DPDK CI for detecting bugs. But then application developers would not benefit, because they don't run their code through the DPDK CI. So I am also against this.
> 
> I think 3d (keep loading on attributes, but only the most relevant ones) is the best choice.
> 
> GCC/Clang style attributes are already supported as macros prefixed by __rte, so let's not change the way we do that.
> 
> Regarding the Microsoft SAL, I suppose Microsoft already chose annotation names to avoid collisions, so we could consider using those exact names (i.e. without __rte prefix), and define empty macros for non-Microsoft compilers. This would allow using Microsoft SAL annotations directly in the DPDK code.

Looks like SAL was developed outside of all the other compilers, and probably pre-dates it.
Having had to deal with it, my impression was it that it became a nuisance on a large code base.
The value starts to drop off fast. And any annotation is only as good as the automated tooling
that supports it.  Doing more annotation than the CI system uses is worthless.

It would be good if there was a common set support by VS, Gcc, and Clang with DPDK macros
for that. We already have annotations for format and allocations.

Re: [PATCH v7 4/4] eal: add nonnull and access function attributes

[David Marchand]

On Wed, Jan 18, 2023 at 9:31 AM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> +To: Thomas & David, you probably have some opinions on this too!
>
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Tuesday, 17 January 2023 22.17
> >
> > On Tue, Jan 17, 2023 at 09:19:22AM +0100, Morten Brørup wrote:
> > > > From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> > > > Sent: Monday, 16 January 2023 18.02
> > > >
> > > > On 1/16/2023 1:07 PM, Morten Brørup wrote:
> > > > > Add nonnull function attribute to help the compiler detect a NULL
> > > > > pointer being passed to a function not accepting NULL pointers as
> > an
> > > > > argument at build time.
> > > > >
> > > > > Add access function attributes to tell the compiler how a
> > function
> > > > > accesses memory pointed to by its pointer arguments.
> > > > >
> > > > > Add these attributes to the rte_memcpy() function, as the first
> > in
> > > > > hopefully many to come.
> > > > >
> > > > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > > > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > > ---
> > >
> > > [...]
> > >
> > > > > +/**
> > > > > + * Tells compiler that the pointer arguments must be non-null.
> > > > > + *
> > > > > + * @param ...
> > > > > + *    Comma separated list of parameter indexes of pointer
> > > > arguments.
> > > > > + */
> > > > > +#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
> > > > > +#define __rte_nonnull_params(...) \
> > > > > +       __attribute__((nonnull(__VA_ARGS__)))
> > > > > +#else
> > > > > +#define __rte_nonnull_params(...)
> > > > > +#endif
> > > > > +
> > > >
> > > > What do you think to have a namespace for macros like
> > > > '__rte_param_xxx',
> > > > so name macros as:
> > > > __rte_param_nonull
> > > > __rte_param_read_only
> > > > __rte_param_write_only
> > > >
> > > > No strong opinion, it just feels tidier this way
> > >
> > > Being a proponent of the world_country_city naming scheme myself, I
> > would usually agree with this proposal.
> > >
> > > However, in the future, we might add macros without _param for use
> > along with the function parameters, e.g.:
> > >
> > > int foo(int bar __rte_nonnull __rte_read_only);
> > >
> > > So I decided for this order in the names (treating
> > nonnull/access_mode as "country" and param/params as "city"), also
> > somewhat looking at the __rte_deprecated and __rte_deprecated_msg(msg)
> > macros.
> > >
> > > I have no strong preference either, so if anyone does, please speak
> > up.
> > >
> > > Slightly related, also note this:
> > >
> > > The nonnull macro is plural (_params), because it can take multiple
> > pointer parameter indexes.
> > > The access mode macros are singular (_param), because they only take
> > one pointer parameter index, and the optional size parameter index.
> > >
> > > I considered splitting up the access mode macros even more, making
> > two variants of each, e.g. __rte_read_only_param(ptr_index) and
> > __rte_read_only_param_size(ptr_index, size_index), but concluded that
> > it would be excruciatingly verbose. The only purpose would be to reduce
> > the risk of using them incorrectly. I decided against it, thinking that
> > any developer clever enough to use these macros is also clever enough
> > to understand how to use them (or at least read their parameter
> > descriptions to learn how).
> > >
> >
> > microsoft also has a tool & annotation vehicle for this type of stuff.
> > this discussion has caused me to wonder what happens if we would like
> > to
> > add additional annotations for other tools. just load on the
> > annotations
> > and expand them empty conditionally?
> >
> > https://learn.microsoft.com/en-us/cpp/code-quality/using-sal-
> > annotations-to-reduce-c-cpp-code-defects?view=msvc-170
> >
> > anyway, just a thought. no serious response required here.
>
> Excellent input, Tyler!
>
> If we want DPDK to be considered truly cross-platform, and not treat non-Linux/non-GCC as second class citizens, we need to discuss this.
>
> Microsoft's Source Code Annotation Language (SAL) seems very good, based on its finer granularity than GCC's attributes (which in comparison seem added as an afterthought, not cleanly structured like SAL). I have only skimmed the documentation, but that is my immediate impression of it.
>
> SAL uses a completely different syntax than GCC attributes, and Microsoft happens to also use memcpy() as an example in the documentation referred to:
>
> void * memcpy(
>    _Out_writes_bytes_all_(count) void *dest,
>    _In_reads_bytes_(count) const void *src,
>    size_t count
> );
>
> Going back to how we can handle this in DPDK, we can either:
>
> 1. Not annotate the functions at all, and miss out on finding the errors for us.

Seeing how clang safety checks helped me catch bugs, that would be a pity.

>
> 2. Invent our own language (or find something existing) for function headers, and use a parser to convert them to compiler specific C/C++ headers when building the code.

Argh, no.

>
> 3a. Keep loading on attributes, with empty macros for unsupported compilers.
>
> 3b. Keep loading on attributes, with empty macros for unsupported compilers. But limit ourselves to GCC/Clang style attributes.
>
> 3c. Keep loading on attributes, with empty macros for unsupported compilers. But limit ourselves to Microsoft SAL style attributes.
>
> 3d. Keep loading on attributes, with empty macros for unsupported compilers. But limit ourselves to the most relevant attributes, using performance and/or bug detection as criteria when considering relevance.
>
> I am strongly against both 1 and 2.
>
> If bug detection is the primary driver, we could stick with either 3b or 3c (i.e. only target one specific build environment) and rely on the DPDK CI for detecting bugs. But then application developers would not benefit, because they don't run their code through the DPDK CI. So I am also against this.
>
> I think 3d (keep loading on attributes, but only the most relevant ones) is the best choice.
>
> GCC/Clang style attributes are already supported as macros prefixed by __rte, so let's not change the way we do that.
>
> Regarding the Microsoft SAL, I suppose Microsoft already chose annotation names to avoid collisions, so we could consider using those exact names (i.e. without __rte prefix), and define empty macros for non-Microsoft compilers. This would allow using Microsoft SAL annotations directly in the DPDK code.
>

I have a bit of trouble understanding the difference between 3a and
3d.. 3a would be about accepting any annotation?

3d is the best option as it is not changing anything to what we were
doing so far: we evaluate the pros and cons of each annotations/tools,
case per case.


-- 
David Marchand

RE: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Morten Brørup]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Tuesday, 31 January 2023 12.15
> 
> On Wed, Jan 18, 2023 at 9:31 AM Morten Brørup
> <mb@smartsharesystems.com> wrote:
> >
> > +To: Thomas & David, you probably have some opinions on this too!
> >
> > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > Sent: Tuesday, 17 January 2023 22.17
> > >
> > > On Tue, Jan 17, 2023 at 09:19:22AM +0100, Morten Brørup wrote:
> > > > > From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> > > > > Sent: Monday, 16 January 2023 18.02
> > > > >
> > > > > On 1/16/2023 1:07 PM, Morten Brørup wrote:
> > > > > > Add nonnull function attribute to help the compiler detect a
> NULL
> > > > > > pointer being passed to a function not accepting NULL
> pointers as
> > > an
> > > > > > argument at build time.
> > > > > >
> > > > > > Add access function attributes to tell the compiler how a
> > > function
> > > > > > accesses memory pointed to by its pointer arguments.
> > > > > >
> > > > > > Add these attributes to the rte_memcpy() function, as the
> first
> > > in
> > > > > > hopefully many to come.
> > > > > >
> > > > > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > > > > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > > > ---
> > > >
> > > > [...]
> > > >
> > > > > > +/**
> > > > > > + * Tells compiler that the pointer arguments must be non-
> null.
> > > > > > + *
> > > > > > + * @param ...
> > > > > > + *    Comma separated list of parameter indexes of pointer
> > > > > arguments.
> > > > > > + */
> > > > > > +#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
> > > > > > +#define __rte_nonnull_params(...) \
> > > > > > +       __attribute__((nonnull(__VA_ARGS__)))
> > > > > > +#else
> > > > > > +#define __rte_nonnull_params(...)
> > > > > > +#endif
> > > > > > +
> > > > >
> > > > > What do you think to have a namespace for macros like
> > > > > '__rte_param_xxx',
> > > > > so name macros as:
> > > > > __rte_param_nonull
> > > > > __rte_param_read_only
> > > > > __rte_param_write_only
> > > > >
> > > > > No strong opinion, it just feels tidier this way
> > > >
> > > > Being a proponent of the world_country_city naming scheme myself,
> I
> > > would usually agree with this proposal.
> > > >
> > > > However, in the future, we might add macros without _param for
> use
> > > along with the function parameters, e.g.:
> > > >
> > > > int foo(int bar __rte_nonnull __rte_read_only);
> > > >
> > > > So I decided for this order in the names (treating
> > > nonnull/access_mode as "country" and param/params as "city"), also
> > > somewhat looking at the __rte_deprecated and
> __rte_deprecated_msg(msg)
> > > macros.
> > > >
> > > > I have no strong preference either, so if anyone does, please
> speak
> > > up.
> > > >
> > > > Slightly related, also note this:
> > > >
> > > > The nonnull macro is plural (_params), because it can take
> multiple
> > > pointer parameter indexes.
> > > > The access mode macros are singular (_param), because they only
> take
> > > one pointer parameter index, and the optional size parameter index.
> > > >
> > > > I considered splitting up the access mode macros even more,
> making
> > > two variants of each, e.g. __rte_read_only_param(ptr_index) and
> > > __rte_read_only_param_size(ptr_index, size_index), but concluded
> that
> > > it would be excruciatingly verbose. The only purpose would be to
> reduce
> > > the risk of using them incorrectly. I decided against it, thinking
> that
> > > any developer clever enough to use these macros is also clever
> enough
> > > to understand how to use them (or at least read their parameter
> > > descriptions to learn how).
> > > >
> > >
> > > microsoft also has a tool & annotation vehicle for this type of
> stuff.
> > > this discussion has caused me to wonder what happens if we would
> like
> > > to
> > > add additional annotations for other tools. just load on the
> > > annotations
> > > and expand them empty conditionally?
> > >
> > > https://learn.microsoft.com/en-us/cpp/code-quality/using-sal-
> > > annotations-to-reduce-c-cpp-code-defects?view=msvc-170
> > >
> > > anyway, just a thought. no serious response required here.
> >
> > Excellent input, Tyler!
> >
> > If we want DPDK to be considered truly cross-platform, and not treat
> non-Linux/non-GCC as second class citizens, we need to discuss this.
> >
> > Microsoft's Source Code Annotation Language (SAL) seems very good,
> based on its finer granularity than GCC's attributes (which in
> comparison seem added as an afterthought, not cleanly structured like
> SAL). I have only skimmed the documentation, but that is my immediate
> impression of it.
> >
> > SAL uses a completely different syntax than GCC attributes, and
> Microsoft happens to also use memcpy() as an example in the
> documentation referred to:
> >
> > void * memcpy(
> >    _Out_writes_bytes_all_(count) void *dest,
> >    _In_reads_bytes_(count) const void *src,
> >    size_t count
> > );

Stephen had bad experiences with SAL, so let's just consider the SAL memcpy() example a reference only, showing how the syntax of annotations can differ very much between build systems.

> >
> > Going back to how we can handle this in DPDK, we can either:
> >
> > 1. Not annotate the functions at all, and miss out on finding the
> errors for us.
> 
> Seeing how clang safety checks helped me catch bugs, that would be a
> pity.
> 
> >
> > 2. Invent our own language (or find something existing) for function
> headers, and use a parser to convert them to compiler specific C/C++
> headers when building the code.
> 
> Argh, no.
> 
> >
> > 3a. Keep loading on attributes, with empty macros for unsupported
> compilers.
> >
> > 3b. Keep loading on attributes, with empty macros for unsupported
> compilers. But limit ourselves to GCC/Clang style attributes.
> >
> > 3c. Keep loading on attributes, with empty macros for unsupported
> compilers. But limit ourselves to Microsoft SAL style attributes.
> >
> > 3d. Keep loading on attributes, with empty macros for unsupported
> compilers. But limit ourselves to the most relevant attributes, using
> performance and/or bug detection as criteria when considering
> relevance.
> >
> > I am strongly against both 1 and 2.
> >
> > If bug detection is the primary driver, we could stick with either 3b
> or 3c (i.e. only target one specific build environment) and rely on the
> DPDK CI for detecting bugs. But then application developers would not
> benefit, because they don't run their code through the DPDK CI. So I am
> also against this.
> >
> > I think 3d (keep loading on attributes, but only the most relevant
> ones) is the best choice.
> >
> > GCC/Clang style attributes are already supported as macros prefixed
> by __rte, so let's not change the way we do that.
> >
> > Regarding the Microsoft SAL, I suppose Microsoft already chose
> annotation names to avoid collisions, so we could consider using those
> exact names (i.e. without __rte prefix), and define empty macros for
> non-Microsoft compilers. This would allow using Microsoft SAL
> annotations directly in the DPDK code.
> >
> 
> I have a bit of trouble understanding the difference between 3a and
> 3d.. 3a would be about accepting any annotation?

Correct.

> 
> 3d is the best option as it is not changing anything to what we were
> doing so far: we evaluate the pros and cons of each annotations/tools,
> case per case.

Agree!

Re: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Tyler Retzlaff]

On Tue, Jan 31, 2023 at 01:23:34PM +0100, Morten Brørup wrote:
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Tuesday, 31 January 2023 12.15
> > 
> > On Wed, Jan 18, 2023 at 9:31 AM Morten Brørup
> > <mb@smartsharesystems.com> wrote:
> > >
> > > +To: Thomas & David, you probably have some opinions on this too!
> > >
> > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > Sent: Tuesday, 17 January 2023 22.17
> > > >
> > > > On Tue, Jan 17, 2023 at 09:19:22AM +0100, Morten Brørup wrote:
> > > > > > From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> > > > > > Sent: Monday, 16 January 2023 18.02
> > > > > >
> > > > > > On 1/16/2023 1:07 PM, Morten Brørup wrote:
> > > > > > > Add nonnull function attribute to help the compiler detect a
> > NULL
> > > > > > > pointer being passed to a function not accepting NULL
> > pointers as
> > > > an
> > > > > > > argument at build time.
> > > > > > >
> > > > > > > Add access function attributes to tell the compiler how a
> > > > function
> > > > > > > accesses memory pointed to by its pointer arguments.
> > > > > > >
> > > > > > > Add these attributes to the rte_memcpy() function, as the
> > first
> > > > in
> > > > > > > hopefully many to come.
> > > > > > >
> > > > > > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > > > > > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > > > > ---
> > > > >
> > > > > [...]
> > > > >
> > > > > > > +/**
> > > > > > > + * Tells compiler that the pointer arguments must be non-
> > null.
> > > > > > > + *
> > > > > > > + * @param ...
> > > > > > > + *    Comma separated list of parameter indexes of pointer
> > > > > > arguments.
> > > > > > > + */
> > > > > > > +#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
> > > > > > > +#define __rte_nonnull_params(...) \
> > > > > > > +       __attribute__((nonnull(__VA_ARGS__)))
> > > > > > > +#else
> > > > > > > +#define __rte_nonnull_params(...)
> > > > > > > +#endif
> > > > > > > +
> > > > > >
> > > > > > What do you think to have a namespace for macros like
> > > > > > '__rte_param_xxx',
> > > > > > so name macros as:
> > > > > > __rte_param_nonull
> > > > > > __rte_param_read_only
> > > > > > __rte_param_write_only
> > > > > >
> > > > > > No strong opinion, it just feels tidier this way
> > > > >
> > > > > Being a proponent of the world_country_city naming scheme myself,
> > I
> > > > would usually agree with this proposal.
> > > > >
> > > > > However, in the future, we might add macros without _param for
> > use
> > > > along with the function parameters, e.g.:
> > > > >
> > > > > int foo(int bar __rte_nonnull __rte_read_only);
> > > > >
> > > > > So I decided for this order in the names (treating
> > > > nonnull/access_mode as "country" and param/params as "city"), also
> > > > somewhat looking at the __rte_deprecated and
> > __rte_deprecated_msg(msg)
> > > > macros.
> > > > >
> > > > > I have no strong preference either, so if anyone does, please
> > speak
> > > > up.
> > > > >
> > > > > Slightly related, also note this:
> > > > >
> > > > > The nonnull macro is plural (_params), because it can take
> > multiple
> > > > pointer parameter indexes.
> > > > > The access mode macros are singular (_param), because they only
> > take
> > > > one pointer parameter index, and the optional size parameter index.
> > > > >
> > > > > I considered splitting up the access mode macros even more,
> > making
> > > > two variants of each, e.g. __rte_read_only_param(ptr_index) and
> > > > __rte_read_only_param_size(ptr_index, size_index), but concluded
> > that
> > > > it would be excruciatingly verbose. The only purpose would be to
> > reduce
> > > > the risk of using them incorrectly. I decided against it, thinking
> > that
> > > > any developer clever enough to use these macros is also clever
> > enough
> > > > to understand how to use them (or at least read their parameter
> > > > descriptions to learn how).
> > > > >
> > > >
> > > > microsoft also has a tool & annotation vehicle for this type of
> > stuff.
> > > > this discussion has caused me to wonder what happens if we would
> > like
> > > > to
> > > > add additional annotations for other tools. just load on the
> > > > annotations
> > > > and expand them empty conditionally?
> > > >
> > > > https://learn.microsoft.com/en-us/cpp/code-quality/using-sal-
> > > > annotations-to-reduce-c-cpp-code-defects?view=msvc-170
> > > >
> > > > anyway, just a thought. no serious response required here.
> > >
> > > Excellent input, Tyler!
> > >
> > > If we want DPDK to be considered truly cross-platform, and not treat
> > non-Linux/non-GCC as second class citizens, we need to discuss this.
> > >
> > > Microsoft's Source Code Annotation Language (SAL) seems very good,
> > based on its finer granularity than GCC's attributes (which in
> > comparison seem added as an afterthought, not cleanly structured like
> > SAL). I have only skimmed the documentation, but that is my immediate
> > impression of it.
> > >
> > > SAL uses a completely different syntax than GCC attributes, and
> > Microsoft happens to also use memcpy() as an example in the
> > documentation referred to:
> > >
> > > void * memcpy(
> > >    _Out_writes_bytes_all_(count) void *dest,
> > >    _In_reads_bytes_(count) const void *src,
> > >    size_t count
> > > );
> 
> Stephen had bad experiences with SAL, so let's just consider the SAL memcpy() example a reference only, showing how the syntax of annotations can differ very much between build systems.

yes, if we are in a position to use annotations today that work with
clang/gcc then let's do that even if they aren't compatible with SAL.
so long as they expand empty when not using clang/gcc we can defer discussion
about other tools like SAL.

> 
> > >
> > > Going back to how we can handle this in DPDK, we can either:
> > >
> > > 1. Not annotate the functions at all, and miss out on finding the
> > errors for us.
> > 
> > Seeing how clang safety checks helped me catch bugs, that would be a
> > pity.
> > 
> > >
> > > 2. Invent our own language (or find something existing) for function
> > headers, and use a parser to convert them to compiler specific C/C++
> > headers when building the code.
> > 
> > Argh, no.
> > 
> > >
> > > 3a. Keep loading on attributes, with empty macros for unsupported
> > compilers.
> > >
> > > 3b. Keep loading on attributes, with empty macros for unsupported
> > compilers. But limit ourselves to GCC/Clang style attributes.
> > >
> > > 3c. Keep loading on attributes, with empty macros for unsupported
> > compilers. But limit ourselves to Microsoft SAL style attributes.
> > >
> > > 3d. Keep loading on attributes, with empty macros for unsupported
> > compilers. But limit ourselves to the most relevant attributes, using
> > performance and/or bug detection as criteria when considering
> > relevance.
> > >
> > > I am strongly against both 1 and 2.
> > >
> > > If bug detection is the primary driver, we could stick with either 3b
> > or 3c (i.e. only target one specific build environment) and rely on the
> > DPDK CI for detecting bugs. But then application developers would not
> > benefit, because they don't run their code through the DPDK CI. So I am
> > also against this.
> > >
> > > I think 3d (keep loading on attributes, but only the most relevant
> > ones) is the best choice.
> > >
> > > GCC/Clang style attributes are already supported as macros prefixed
> > by __rte, so let's not change the way we do that.
> > >
> > > Regarding the Microsoft SAL, I suppose Microsoft already chose
> > annotation names to avoid collisions, so we could consider using those
> > exact names (i.e. without __rte prefix), and define empty macros for
> > non-Microsoft compilers. This would allow using Microsoft SAL
> > annotations directly in the DPDK code.
> > >
> > 
> > I have a bit of trouble understanding the difference between 3a and
> > 3d.. 3a would be about accepting any annotation?
> 
> Correct.
> 
> > 
> > 3d is the best option as it is not changing anything to what we were
> > doing so far: we evaluate the pros and cons of each annotations/tools,
> > case per case.
> 
> Agree!
> 

Re: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Thomas Monjalon]

31/01/2023 19:26, Tyler Retzlaff:
> On Tue, Jan 31, 2023 at 01:23:34PM +0100, Morten Brørup wrote:
> > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > Sent: Tuesday, 31 January 2023 12.15
> > > 
> > > On Wed, Jan 18, 2023 at 9:31 AM Morten Brørup
> > > <mb@smartsharesystems.com> wrote:
> > > >
> > > > +To: Thomas & David, you probably have some opinions on this too!
> > > >
> > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > Sent: Tuesday, 17 January 2023 22.17
> > > > >
> > > > > On Tue, Jan 17, 2023 at 09:19:22AM +0100, Morten Brørup wrote:
> > > > > > > From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> > > > > > > Sent: Monday, 16 January 2023 18.02
> > > > > > >
> > > > > > > On 1/16/2023 1:07 PM, Morten Brørup wrote:
> > > > > > > > Add nonnull function attribute to help the compiler detect a
> > > NULL
> > > > > > > > pointer being passed to a function not accepting NULL
> > > pointers as
> > > > > an
> > > > > > > > argument at build time.
> > > > > > > >
> > > > > > > > Add access function attributes to tell the compiler how a
> > > > > function
> > > > > > > > accesses memory pointed to by its pointer arguments.
> > > > > > > >
> > > > > > > > Add these attributes to the rte_memcpy() function, as the
> > > first
> > > > > in
> > > > > > > > hopefully many to come.
> > > > > > > >
> > > > > > > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > > > > > > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > > > > > ---
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > +/**
> > > > > > > > + * Tells compiler that the pointer arguments must be non-
> > > null.
> > > > > > > > + *
> > > > > > > > + * @param ...
> > > > > > > > + *    Comma separated list of parameter indexes of pointer
> > > > > > > arguments.
> > > > > > > > + */
> > > > > > > > +#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
> > > > > > > > +#define __rte_nonnull_params(...) \
> > > > > > > > +       __attribute__((nonnull(__VA_ARGS__)))
> > > > > > > > +#else
> > > > > > > > +#define __rte_nonnull_params(...)
> > > > > > > > +#endif
> > > > > > > > +
> > > > > > >
> > > > > > > What do you think to have a namespace for macros like
> > > > > > > '__rte_param_xxx',
> > > > > > > so name macros as:
> > > > > > > __rte_param_nonull
> > > > > > > __rte_param_read_only
> > > > > > > __rte_param_write_only
> > > > > > > 
> > > > > > > No strong opinion, it just feels tidier this way

I tend to prefer this kind of namespace as well.
Let's compare different naming proposals,
taking into account what we already have for some annotations,
and what is proposed to be added in this patch and David's patch
for lock annotations.

> > > > > > Being a proponent of the world_country_city naming scheme myself,
> > > I
> > > > > would usually agree with this proposal.
> > > > > >
> > > > > > However, in the future, we might add macros without _param for
> > > use
> > > > > along with the function parameters, e.g.:
> > > > > >
> > > > > > int foo(int bar __rte_nonnull __rte_read_only);
> > > > > >
> > > > > > So I decided for this order in the names (treating
> > > > > nonnull/access_mode as "country" and param/params as "city"), also
> > > > > somewhat looking at the __rte_deprecated and
> > > __rte_deprecated_msg(msg)
> > > > > macros.
> > > > > >
> > > > > > I have no strong preference either, so if anyone does, please
> > > speak
> > > > > up.
> > > > > >
> > > > > > Slightly related, also note this:
> > > > > >
> > > > > > The nonnull macro is plural (_params), because it can take
> > > multiple
> > > > > pointer parameter indexes.
> > > > > > The access mode macros are singular (_param), because they only
> > > take
> > > > > one pointer parameter index, and the optional size parameter index.
> > > > > >
> > > > > > I considered splitting up the access mode macros even more,
> > > making
> > > > > two variants of each, e.g. __rte_read_only_param(ptr_index) and
> > > > > __rte_read_only_param_size(ptr_index, size_index), but concluded
> > > that
> > > > > it would be excruciatingly verbose. The only purpose would be to
> > > reduce
> > > > > the risk of using them incorrectly. I decided against it, thinking
> > > that
> > > > > any developer clever enough to use these macros is also clever
> > > enough
> > > > > to understand how to use them (or at least read their parameter
> > > > > descriptions to learn how).

Longer macro names may be better for code readers.


> > > > > microsoft also has a tool & annotation vehicle for this type of
> > > stuff.
> > > > > this discussion has caused me to wonder what happens if we would
> > > like
> > > > > to
> > > > > add additional annotations for other tools. just load on the
> > > > > annotations
> > > > > and expand them empty conditionally?
> > > > >
> > > > > https://learn.microsoft.com/en-us/cpp/code-quality/using-sal-
> > > > > annotations-to-reduce-c-cpp-code-defects?view=msvc-170
> > > > >
> > > > > anyway, just a thought. no serious response required here.
> > > >
> > > > Excellent input, Tyler!
> > > >
> > > > If we want DPDK to be considered truly cross-platform, and not treat
> > > non-Linux/non-GCC as second class citizens, we need to discuss this.
> > > >
> > > > Microsoft's Source Code Annotation Language (SAL) seems very good,
> > > based on its finer granularity than GCC's attributes (which in
> > > comparison seem added as an afterthought, not cleanly structured like
> > > SAL). I have only skimmed the documentation, but that is my immediate
> > > impression of it.
> > > >
> > > > SAL uses a completely different syntax than GCC attributes, and
> > > Microsoft happens to also use memcpy() as an example in the
> > > documentation referred to:
> > > >
> > > > void * memcpy(
> > > >    _Out_writes_bytes_all_(count) void *dest,
> > > >    _In_reads_bytes_(count) const void *src,
> > > >    size_t count
> > > > );
> > 
> > Stephen had bad experiences with SAL, so let's just consider the SAL memcpy() example a reference only, showing how the syntax of annotations can differ very much between build systems.
> 
> yes, if we are in a position to use annotations today that work with
> clang/gcc then let's do that even if they aren't compatible with SAL.
> so long as they expand empty when not using clang/gcc we can defer discussion
> about other tools like SAL.

Yes of course it must expand empty for compilers not supporting the annotation.
Anyway I don't think we need to support all compilers with annotation.
The main goal of annotation is to do more checks in the CI,
so supporting one compiler should be enough.


> > > > Going back to how we can handle this in DPDK, we can either:
> > > >
> > > > 1. Not annotate the functions at all, and miss out on finding the
> > > errors for us.
> > > 
> > > Seeing how clang safety checks helped me catch bugs, that would be a
> > > pity.
> > > 
> > > >
> > > > 2. Invent our own language (or find something existing) for function
> > > headers, and use a parser to convert them to compiler specific C/C++
> > > headers when building the code.
> > > 
> > > Argh, no.
> > > 
> > > >
> > > > 3a. Keep loading on attributes, with empty macros for unsupported
> > > compilers.
> > > >
> > > > 3b. Keep loading on attributes, with empty macros for unsupported
> > > compilers. But limit ourselves to GCC/Clang style attributes.
> > > >
> > > > 3c. Keep loading on attributes, with empty macros for unsupported
> > > compilers. But limit ourselves to Microsoft SAL style attributes.
> > > >
> > > > 3d. Keep loading on attributes, with empty macros for unsupported
> > > compilers. But limit ourselves to the most relevant attributes, using
> > > performance and/or bug detection as criteria when considering
> > > relevance.
> > > >
> > > > I am strongly against both 1 and 2.
> > > >
> > > > If bug detection is the primary driver, we could stick with either 3b
> > > or 3c (i.e. only target one specific build environment) and rely on the
> > > DPDK CI for detecting bugs. But then application developers would not
> > > benefit, because they don't run their code through the DPDK CI. So I am
> > > also against this.
> > > >
> > > > I think 3d (keep loading on attributes, but only the most relevant
> > > ones) is the best choice.
> > > >
> > > > GCC/Clang style attributes are already supported as macros prefixed
> > > by __rte, so let's not change the way we do that.
> > > >
> > > > Regarding the Microsoft SAL, I suppose Microsoft already chose
> > > annotation names to avoid collisions, so we could consider using those
> > > exact names (i.e. without __rte prefix), and define empty macros for
> > > non-Microsoft compilers. This would allow using Microsoft SAL
> > > annotations directly in the DPDK code.
> > > >
> > > 
> > > I have a bit of trouble understanding the difference between 3a and
> > > 3d.. 3a would be about accepting any annotation?
> > 
> > Correct.
> > 
> > > 
> > > 3d is the best option as it is not changing anything to what we were
> > > doing so far: we evaluate the pros and cons of each annotations/tools,
> > > case per case.
> > 
> > Agree!

I am for 3d as well.
We should focus on clang for using compilation checks.


Another question about annotations is how much we want to use them.
I think we should use such check for critical code only.
Adding annotations everywhere is not always worth making the code uglier.

RE: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Morten Brørup]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, 31 January 2023 23.52
> 
> 31/01/2023 19:26, Tyler Retzlaff:
> > On Tue, Jan 31, 2023 at 01:23:34PM +0100, Morten Brørup wrote:
> > > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > > Sent: Tuesday, 31 January 2023 12.15
> > > >
> > > > On Wed, Jan 18, 2023 at 9:31 AM Morten Brørup
> > > > <mb@smartsharesystems.com> wrote:
> > > > >
> > > > > +To: Thomas & David, you probably have some opinions on this
> too!
> > > > >
> > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > > Sent: Tuesday, 17 January 2023 22.17
> > > > > >
> > > > > > On Tue, Jan 17, 2023 at 09:19:22AM +0100, Morten Brørup
> wrote:
> > > > > > > > From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> > > > > > > > Sent: Monday, 16 January 2023 18.02
> > > > > > > >
> > > > > > > > On 1/16/2023 1:07 PM, Morten Brørup wrote:
> > > > > > > > > Add nonnull function attribute to help the compiler
> detect a
> > > > NULL
> > > > > > > > > pointer being passed to a function not accepting NULL
> > > > pointers as
> > > > > > an
> > > > > > > > > argument at build time.
> > > > > > > > >
> > > > > > > > > Add access function attributes to tell the compiler how
> a
> > > > > > function
> > > > > > > > > accesses memory pointed to by its pointer arguments.
> > > > > > > > >
> > > > > > > > > Add these attributes to the rte_memcpy() function, as
> the
> > > > first
> > > > > > in
> > > > > > > > > hopefully many to come.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > > > > > > > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > > > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > > > > > > ---
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > > > +/**
> > > > > > > > > + * Tells compiler that the pointer arguments must be
> non-
> > > > null.
> > > > > > > > > + *
> > > > > > > > > + * @param ...
> > > > > > > > > + *    Comma separated list of parameter indexes of
> pointer
> > > > > > > > arguments.
> > > > > > > > > + */
> > > > > > > > > +#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
> > > > > > > > > +#define __rte_nonnull_params(...) \
> > > > > > > > > +       __attribute__((nonnull(__VA_ARGS__)))
> > > > > > > > > +#else
> > > > > > > > > +#define __rte_nonnull_params(...)
> > > > > > > > > +#endif
> > > > > > > > > +
> > > > > > > >
> > > > > > > > What do you think to have a namespace for macros like
> > > > > > > > '__rte_param_xxx',
> > > > > > > > so name macros as:
> > > > > > > > __rte_param_nonull
> > > > > > > > __rte_param_read_only
> > > > > > > > __rte_param_write_only
> > > > > > > >
> > > > > > > > No strong opinion, it just feels tidier this way
> 
> I tend to prefer this kind of namespace as well.
> Let's compare different naming proposals,
> taking into account what we already have for some annotations,
> and what is proposed to be added in this patch and David's patch
> for lock annotations.

David's lock annotations don't use a dedicated naming convention, but simply replaces __attribute__(name(params)) with __rte_name(params), e.g.:

#define __rte_guarded_by(...) \
	__attribute__((guarded_by(__VA_ARGS__)))
#define __rte_exclusive_locks_required(...) \
	__attribute__((exclusive_locks_required(__VA_ARGS__)))
#define __rte_assert_exclusive_lock(...) \
	__attribute__((assert_exclusive_lock(__VA_ARGS__)))

This follows the existing convention in rte_common.h, which is easily readable, because they directly translate to GCC attribute names, e.g.:

#define __rte_warn_unused_result __attribute__((warn_unused_result))
#define __rte_always_inline inline __attribute__((always_inline))
#define __rte_noinline __attribute__((noinline))
#define __rte_hot __attribute__((hot))
#define __rte_cold __attribute__((cold))

I could follow this convention too, e.g.:

#define __rte_nonnull(...) __attribute__((nonnull(__VA_ARGS__)))

#define __rte_access_read_only(...) \
	__attribute__((access(read_only, __VA_ARGS__)))
#define __rte_access_write_only(...) \
	__attribute__((access(write_only, __VA_ARGS__)))

> 
> > > > > > > Being a proponent of the world_country_city naming scheme
> myself,
> > > > I
> > > > > > would usually agree with this proposal.
> > > > > > >
> > > > > > > However, in the future, we might add macros without _param
> for
> > > > use
> > > > > > along with the function parameters, e.g.:
> > > > > > >
> > > > > > > int foo(int bar __rte_nonnull __rte_read_only);
> > > > > > >
> > > > > > > So I decided for this order in the names (treating
> > > > > > nonnull/access_mode as "country" and param/params as "city"),
> also
> > > > > > somewhat looking at the __rte_deprecated and
> > > > __rte_deprecated_msg(msg)
> > > > > > macros.
> > > > > > >
> > > > > > > I have no strong preference either, so if anyone does,
> please
> > > > speak
> > > > > > up.
> > > > > > >
> > > > > > > Slightly related, also note this:
> > > > > > >
> > > > > > > The nonnull macro is plural (_params), because it can take
> > > > multiple
> > > > > > pointer parameter indexes.
> > > > > > > The access mode macros are singular (_param), because they
> only
> > > > take
> > > > > > one pointer parameter index, and the optional size parameter
> index.
> > > > > > >
> > > > > > > I considered splitting up the access mode macros even more,
> > > > making
> > > > > > two variants of each, e.g. __rte_read_only_param(ptr_index)
> and
> > > > > > __rte_read_only_param_size(ptr_index, size_index), but
> concluded
> > > > that
> > > > > > it would be excruciatingly verbose. The only purpose would be
> to
> > > > reduce
> > > > > > the risk of using them incorrectly. I decided against it,
> thinking
> > > > that
> > > > > > any developer clever enough to use these macros is also
> clever
> > > > enough
> > > > > > to understand how to use them (or at least read their
> parameter
> > > > > > descriptions to learn how).
> 
> Longer macro names may be better for code readers.

You have a good point here, Thomas. It will also prevent using the access mode attribute macros incorrectly.

Let's proceed with two macros (without and with size parameter) instead of the combined macros (with optional size parameter).

Referring to the existing convention in rte_common.h, what do you think about naming the new macros as follows?

__rte_access_read_only(ptr_index)
__rte_access_read_only_size(ptr_index, size_index)

Or just:

__rte_read_only(ptr_index)
__rte_read_only_size(ptr_index, size_index)

> 
> 
> > > > > > microsoft also has a tool & annotation vehicle for this type
> of
> > > > stuff.
> > > > > > this discussion has caused me to wonder what happens if we
> would
> > > > like
> > > > > > to
> > > > > > add additional annotations for other tools. just load on the
> > > > > > annotations
> > > > > > and expand them empty conditionally?
> > > > > >
> > > > > > https://learn.microsoft.com/en-us/cpp/code-quality/using-sal-
> > > > > > annotations-to-reduce-c-cpp-code-defects?view=msvc-170
> > > > > >
> > > > > > anyway, just a thought. no serious response required here.
> > > > >
> > > > > Excellent input, Tyler!
> > > > >
> > > > > If we want DPDK to be considered truly cross-platform, and not
> treat
> > > > non-Linux/non-GCC as second class citizens, we need to discuss
> this.
> > > > >
> > > > > Microsoft's Source Code Annotation Language (SAL) seems very
> good,
> > > > based on its finer granularity than GCC's attributes (which in
> > > > comparison seem added as an afterthought, not cleanly structured
> like
> > > > SAL). I have only skimmed the documentation, but that is my
> immediate
> > > > impression of it.
> > > > >
> > > > > SAL uses a completely different syntax than GCC attributes, and
> > > > Microsoft happens to also use memcpy() as an example in the
> > > > documentation referred to:
> > > > >
> > > > > void * memcpy(
> > > > >    _Out_writes_bytes_all_(count) void *dest,
> > > > >    _In_reads_bytes_(count) const void *src,
> > > > >    size_t count
> > > > > );
> > >
> > > Stephen had bad experiences with SAL, so let's just consider the
> SAL memcpy() example a reference only, showing how the syntax of
> annotations can differ very much between build systems.
> >
> > yes, if we are in a position to use annotations today that work with
> > clang/gcc then let's do that even if they aren't compatible with SAL.
> > so long as they expand empty when not using clang/gcc we can defer
> discussion
> > about other tools like SAL.
> 
> Yes of course it must expand empty for compilers not supporting the
> annotation.
> Anyway I don't think we need to support all compilers with annotation.
> The main goal of annotation is to do more checks in the CI,
> so supporting one compiler should be enough.

There is also a secondary goal of increased performance. When the compiler knows more about a function's behavior, it can optimize more around it. E.g. knowing that a function doesn't modify something already held in a register (or other local memory) allows the compiler to reuse the local copy (in the register) without fetching the original again.

> 
> 
> > > > > Going back to how we can handle this in DPDK, we can either:
> > > > >
> > > > > 1. Not annotate the functions at all, and miss out on finding
> the
> > > > errors for us.
> > > >
> > > > Seeing how clang safety checks helped me catch bugs, that would
> be a
> > > > pity.
> > > >
> > > > >
> > > > > 2. Invent our own language (or find something existing) for
> function
> > > > headers, and use a parser to convert them to compiler specific
> C/C++
> > > > headers when building the code.
> > > >
> > > > Argh, no.
> > > >
> > > > >
> > > > > 3a. Keep loading on attributes, with empty macros for
> unsupported
> > > > compilers.
> > > > >
> > > > > 3b. Keep loading on attributes, with empty macros for
> unsupported
> > > > compilers. But limit ourselves to GCC/Clang style attributes.
> > > > >
> > > > > 3c. Keep loading on attributes, with empty macros for
> unsupported
> > > > compilers. But limit ourselves to Microsoft SAL style attributes.
> > > > >
> > > > > 3d. Keep loading on attributes, with empty macros for
> unsupported
> > > > compilers. But limit ourselves to the most relevant attributes,
> using
> > > > performance and/or bug detection as criteria when considering
> > > > relevance.
> > > > >
> > > > > I am strongly against both 1 and 2.
> > > > >
> > > > > If bug detection is the primary driver, we could stick with
> either 3b
> > > > or 3c (i.e. only target one specific build environment) and rely
> on the
> > > > DPDK CI for detecting bugs. But then application developers would
> not
> > > > benefit, because they don't run their code through the DPDK CI.
> So I am
> > > > also against this.
> > > > >
> > > > > I think 3d (keep loading on attributes, but only the most
> relevant
> > > > ones) is the best choice.
> > > > >
> > > > > GCC/Clang style attributes are already supported as macros
> prefixed
> > > > by __rte, so let's not change the way we do that.
> > > > >
> > > > > Regarding the Microsoft SAL, I suppose Microsoft already chose
> > > > annotation names to avoid collisions, so we could consider using
> those
> > > > exact names (i.e. without __rte prefix), and define empty macros
> for
> > > > non-Microsoft compilers. This would allow using Microsoft SAL
> > > > annotations directly in the DPDK code.
> > > > >
> > > >
> > > > I have a bit of trouble understanding the difference between 3a
> and
> > > > 3d.. 3a would be about accepting any annotation?
> > >
> > > Correct.
> > >
> > > >
> > > > 3d is the best option as it is not changing anything to what we
> were
> > > > doing so far: we evaluate the pros and cons of each
> annotations/tools,
> > > > case per case.
> > >
> > > Agree!
> 
> I am for 3d as well.
> We should focus on clang for using compilation checks.

Clang doesn't support the access mode attribute (yet?). It is only supported by GCC.

The CI also uses GCC, so let's not limit ourselves to what clang can do. Using the access mode attribute with GCC is still useful for the CI to reveal bugs.

I agree with you on the principle: We don't need to add the same bug-finding attributes to all compiler environments; adding them to one CI supported compiler environment should suffice.

However, performance-enhancing attributes would be useful to support in multiple compiler environments.

> 
> 
> Another question about annotations is how much we want to use them.
> I think we should use such check for critical code only.

Agree. And this includes some fast path code, where they might benefit performance.

> Adding annotations everywhere is not always worth making the code
> uglier.

I agree. I suppose Stephen stressed the same point when referring his experience working with SAL.

It will be impossible to define a criteria for when to use or not use such attributes, but we can probably agree on this: When adding new functions to DPDK, adding such attributes is not a requirement. This should make it up to the contributor to decide if she wants to add them or not, which can be discussed on a case by case basis if the maintainer disagrees.

This principle should limit the big discussions to which attributes we want to allow into rte_common.h; like these and the ones in David's lock annotation patch.

Re: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Thomas Monjalon]

01/02/2023 13:50, Morten Brørup:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 31/01/2023 19:26, Tyler Retzlaff:
> > > On Tue, Jan 31, 2023 at 01:23:34PM +0100, Morten Brørup wrote:
> > > > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > > > On Wed, Jan 18, 2023 at 9:31 AM Morten Brørup
> > > > > <mb@smartsharesystems.com> wrote:
> > > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > > > On Tue, Jan 17, 2023 at 09:19:22AM +0100, Morten Brørup
> > > > > > > > > > + */
> > > > > > > > > > +#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
> > > > > > > > > > +#define __rte_nonnull_params(...) \
> > > > > > > > > > +       __attribute__((nonnull(__VA_ARGS__)))
> > > > > > > > > > +#else
> > > > > > > > > > +#define __rte_nonnull_params(...)
> > > > > > > > > > +#endif
> > > > > > > > > > +
> > > > > > > > >
> > > > > > > > > What do you think to have a namespace for macros like
> > > > > > > > > '__rte_param_xxx',
> > > > > > > > > so name macros as:
> > > > > > > > > __rte_param_nonull
> > > > > > > > > __rte_param_read_only
> > > > > > > > > __rte_param_write_only
> > > > > > > > >
> > > > > > > > > No strong opinion, it just feels tidier this way
> > 
> > I tend to prefer this kind of namespace as well.
> > Let's compare different naming proposals,
> > taking into account what we already have for some annotations,
> > and what is proposed to be added in this patch and David's patch
> > for lock annotations.
> 
> David's lock annotations don't use a dedicated naming convention, but simply replaces __attribute__(name(params)) with __rte_name(params), e.g.:
> 
> #define __rte_guarded_by(...) \
> 	__attribute__((guarded_by(__VA_ARGS__)))
> #define __rte_exclusive_locks_required(...) \
> 	__attribute__((exclusive_locks_required(__VA_ARGS__)))
> #define __rte_assert_exclusive_lock(...) \
> 	__attribute__((assert_exclusive_lock(__VA_ARGS__)))
> 
> This follows the existing convention in rte_common.h, which is easily readable, because they directly translate to GCC attribute names, e.g.:
> 
> #define __rte_warn_unused_result __attribute__((warn_unused_result))
> #define __rte_always_inline inline __attribute__((always_inline))
> #define __rte_noinline __attribute__((noinline))
> #define __rte_hot __attribute__((hot))
> #define __rte_cold __attribute__((cold))
> 
> I could follow this convention too, e.g.:
> 
> #define __rte_nonnull(...) __attribute__((nonnull(__VA_ARGS__)))
> 
> #define __rte_access_read_only(...) \
> 	__attribute__((access(read_only, __VA_ARGS__)))
> #define __rte_access_write_only(...) \
> 	__attribute__((access(write_only, __VA_ARGS__)))
> 
[...]
> > Longer macro names may be better for code readers.
> 
> You have a good point here, Thomas. It will also prevent using the access mode attribute macros incorrectly.
> 
> Let's proceed with two macros (without and with size parameter) instead of the combined macros (with optional size parameter).
> 
> Referring to the existing convention in rte_common.h, what do you think about naming the new macros as follows?
> 
> __rte_access_read_only(ptr_index)
> __rte_access_read_only_size(ptr_index, size_index)
> 
> Or just:
> 
> __rte_read_only(ptr_index)
> __rte_read_only_size(ptr_index, size_index)

I think we don't need the word "access", so I prefer the second form.

What about the proposal of having "param" in the name?
We could also have "function" is some macro names.
Does it bring a benefit?

Please let's have a naming discussion with many opinions.


> > > > > > > microsoft also has a tool & annotation vehicle for this type
> > of
> > > > > stuff.
> > > > > > > this discussion has caused me to wonder what happens if we
> > would
> > > > > like
> > > > > > > to
> > > > > > > add additional annotations for other tools. just load on the
> > > > > > > annotations
> > > > > > > and expand them empty conditionally?
> > > > > > >
> > > > > > > https://learn.microsoft.com/en-us/cpp/code-quality/using-sal-
> > > > > > > annotations-to-reduce-c-cpp-code-defects?view=msvc-170
> > > > > > >
> > > > > > > anyway, just a thought. no serious response required here.
> > > > > >
> > > > > > Excellent input, Tyler!
> > > > > >
> > > > > > If we want DPDK to be considered truly cross-platform, and not
> > treat
> > > > > non-Linux/non-GCC as second class citizens, we need to discuss
> > this.
> > > > > >
> > > > > > Microsoft's Source Code Annotation Language (SAL) seems very
> > good,
> > > > > based on its finer granularity than GCC's attributes (which in
> > > > > comparison seem added as an afterthought, not cleanly structured
> > like
> > > > > SAL). I have only skimmed the documentation, but that is my
> > immediate
> > > > > impression of it.
> > > > > >
> > > > > > SAL uses a completely different syntax than GCC attributes, and
> > > > > Microsoft happens to also use memcpy() as an example in the
> > > > > documentation referred to:
> > > > > >
> > > > > > void * memcpy(
> > > > > >    _Out_writes_bytes_all_(count) void *dest,
> > > > > >    _In_reads_bytes_(count) const void *src,
> > > > > >    size_t count
> > > > > > );
> > > >
> > > > Stephen had bad experiences with SAL, so let's just consider the
> > SAL memcpy() example a reference only, showing how the syntax of
> > annotations can differ very much between build systems.
> > >
> > > yes, if we are in a position to use annotations today that work with
> > > clang/gcc then let's do that even if they aren't compatible with SAL.
> > > so long as they expand empty when not using clang/gcc we can defer
> > discussion
> > > about other tools like SAL.
> > 
> > Yes of course it must expand empty for compilers not supporting the
> > annotation.
> > Anyway I don't think we need to support all compilers with annotation.
> > The main goal of annotation is to do more checks in the CI,
> > so supporting one compiler should be enough.
> 
> There is also a secondary goal of increased performance. When the compiler knows more about a function's behavior, it can optimize more around it. E.g. knowing that a function doesn't modify something already held in a register (or other local memory) allows the compiler to reuse the local copy (in the register) without fetching the original again.

If we want to enable such performance optimizations with all compilers,
we may have issues to find a common syntax.
In general I am OK to try (best effort)
but we should be reasonnable in case things are getting complex.

[...]
> > > > > 3d is the best option as it is not changing anything to what we
> > were
> > > > > doing so far: we evaluate the pros and cons of each
> > annotations/tools,
> > > > > case per case.
> > > >
> > > > Agree!
> > 
> > I am for 3d as well.
> > We should focus on clang for using compilation checks.
> 
> Clang doesn't support the access mode attribute (yet?). It is only supported by GCC.

OK

> The CI also uses GCC, so let's not limit ourselves to what clang can do. Using the access mode attribute with GCC is still useful for the CI to reveal bugs.
> 
> I agree with you on the principle: We don't need to add the same bug-finding attributes to all compiler environments; adding them to one CI supported compiler environment should suffice.
> 
> However, performance-enhancing attributes would be useful to support in multiple compiler environments.
> 
> > Another question about annotations is how much we want to use them.
> > I think we should use such check for critical code only.
> 
> Agree. And this includes some fast path code, where they might benefit performance.
> 
> > Adding annotations everywhere is not always worth making the code
> > uglier.
> 
> I agree. I suppose Stephen stressed the same point when referring his experience working with SAL.
> 
> It will be impossible to define a criteria for when to use or not use such attributes, but we can probably agree on this: When adding new functions to DPDK, adding such attributes is not a requirement. This should make it up to the contributor to decide if she wants to add them or not, which can be discussed on a case by case basis if the maintainer disagrees.

Yes

> This principle should limit the big discussions to which attributes we want to allow into rte_common.h; like these and the ones in David's lock annotation patch.

OK

Re: [PATCH v7 4/4] eal: add nonnull and access function attributes

[David Marchand]

On Wed, Feb 1, 2023 at 2:16 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > I tend to prefer this kind of namespace as well.
> > > Let's compare different naming proposals,
> > > taking into account what we already have for some annotations,
> > > and what is proposed to be added in this patch and David's patch
> > > for lock annotations.
> >
> > David's lock annotations don't use a dedicated naming convention, but simply replaces __attribute__(name(params)) with __rte_name(params), e.g.:
> >
> > #define __rte_guarded_by(...) \
> >       __attribute__((guarded_by(__VA_ARGS__)))
> > #define __rte_exclusive_locks_required(...) \
> >       __attribute__((exclusive_locks_required(__VA_ARGS__)))
> > #define __rte_assert_exclusive_lock(...) \
> >       __attribute__((assert_exclusive_lock(__VA_ARGS__)))
> >
> > This follows the existing convention in rte_common.h, which is easily readable, because they directly translate to GCC attribute names, e.g.:
> >
> > #define __rte_warn_unused_result __attribute__((warn_unused_result))
> > #define __rte_always_inline inline __attribute__((always_inline))
> > #define __rte_noinline __attribute__((noinline))
> > #define __rte_hot __attribute__((hot))
> > #define __rte_cold __attribute__((cold))
> >
> > I could follow this convention too, e.g.:
> >
> > #define __rte_nonnull(...) __attribute__((nonnull(__VA_ARGS__)))
> >
> > #define __rte_access_read_only(...) \
> >       __attribute__((access(read_only, __VA_ARGS__)))
> > #define __rte_access_write_only(...) \
> >       __attribute__((access(write_only, __VA_ARGS__)))
> >
> [...]
> > > Longer macro names may be better for code readers.
> >
> > You have a good point here, Thomas. It will also prevent using the access mode attribute macros incorrectly.
> >
> > Let's proceed with two macros (without and with size parameter) instead of the combined macros (with optional size parameter).
> >
> > Referring to the existing convention in rte_common.h, what do you think about naming the new macros as follows?
> >
> > __rte_access_read_only(ptr_index)
> > __rte_access_read_only_size(ptr_index, size_index)
> >
> > Or just:
> >
> > __rte_read_only(ptr_index)
> > __rte_read_only_size(ptr_index, size_index)
>
> I think we don't need the word "access", so I prefer the second form.
>
> What about the proposal of having "param" in the name?
> We could also have "function" is some macro names.
> Does it bring a benefit?
>
> Please let's have a naming discussion with many opinions.

Thomas, I think naming convention/discussion is the most likely to
never conclude.

Just copying my previous suggestion.
__rte_read_only_params(indexes...)
__rte_write_only_params(indexes...)
__rte_no_access_params(indexes...)

So if we are not going with the existing (kind of) convention to just
prefix with __rte_, I prefer Morten second form too and I have no
better idea.


As for the lock annotations series, if you are not confident with the
form I went with, I don't mind deferring to a later release.
Though it adds more work on my pile like rebasing the vhost library.
Additionnally, we lose the opportunity to catch introduction of new
lock issues in the dpdk tree.


-- 
David Marchand

RE: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Morten Brørup]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Monday, 6 February 2023 17.11
> 
> On Wed, Feb 1, 2023 at 2:16 PM Thomas Monjalon <thomas@monjalon.net>
> wrote:
> > > > I tend to prefer this kind of namespace as well.
> > > > Let's compare different naming proposals,
> > > > taking into account what we already have for some annotations,
> > > > and what is proposed to be added in this patch and David's patch
> > > > for lock annotations.
> > >
> > > David's lock annotations don't use a dedicated naming convention,
> but simply replaces __attribute__(name(params)) with
> __rte_name(params), e.g.:
> > >
> > > #define __rte_guarded_by(...) \
> > >       __attribute__((guarded_by(__VA_ARGS__)))
> > > #define __rte_exclusive_locks_required(...) \
> > >       __attribute__((exclusive_locks_required(__VA_ARGS__)))
> > > #define __rte_assert_exclusive_lock(...) \
> > >       __attribute__((assert_exclusive_lock(__VA_ARGS__)))
> > >
> > > This follows the existing convention in rte_common.h, which is
> easily readable, because they directly translate to GCC attribute
> names, e.g.:
> > >
> > > #define __rte_warn_unused_result
> __attribute__((warn_unused_result))
> > > #define __rte_always_inline inline __attribute__((always_inline))
> > > #define __rte_noinline __attribute__((noinline))
> > > #define __rte_hot __attribute__((hot))
> > > #define __rte_cold __attribute__((cold))
> > >
> > > I could follow this convention too, e.g.:
> > >
> > > #define __rte_nonnull(...) __attribute__((nonnull(__VA_ARGS__)))
> > >
> > > #define __rte_access_read_only(...) \
> > >       __attribute__((access(read_only, __VA_ARGS__)))
> > > #define __rte_access_write_only(...) \
> > >       __attribute__((access(write_only, __VA_ARGS__)))
> > >
> > [...]
> > > > Longer macro names may be better for code readers.
> > >
> > > You have a good point here, Thomas. It will also prevent using the
> access mode attribute macros incorrectly.
> > >
> > > Let's proceed with two macros (without and with size parameter)
> instead of the combined macros (with optional size parameter).
> > >
> > > Referring to the existing convention in rte_common.h, what do you
> think about naming the new macros as follows?
> > >
> > > __rte_access_read_only(ptr_index)
> > > __rte_access_read_only_size(ptr_index, size_index)
> > >
> > > Or just:
> > >
> > > __rte_read_only(ptr_index)
> > > __rte_read_only_size(ptr_index, size_index)
> >
> > I think we don't need the word "access", so I prefer the second form.
> >
> > What about the proposal of having "param" in the name?
> > We could also have "function" is some macro names.
> > Does it bring a benefit?

The existing __rte_format_printf macro also follows the simple convention, and does not add "param" or "function" in the name:

#define __rte_format_printf(format_index, first_arg) \
	__attribute__((format(printf, format_index, first_arg)))

> >
> > Please let's have a naming discussion with many opinions.
> 
> Thomas, I think naming convention/discussion is the most likely to
> never conclude.

No one else seem to have strong enough opinions to join the discussion. So let's try to conclude the discussion.

> 
> Just copying my previous suggestion.
> __rte_read_only_params(indexes...)
> __rte_write_only_params(indexes...)
> __rte_no_access_params(indexes...)
> 
> So if we are not going with the existing (kind of) convention to just
> prefix with __rte_, I prefer Morten second form too and I have no
> better idea.

I'm leaning towards following the existing convention in rte_common.h, and embrace Thomas' argument to make them more verbose in order to reduce the risk of wrong use. In other words, define these:

__rte_nonnull(...)
__rte_read_only(ptr_index)
__rte_read_only_size(ptr_index, size_index)
__rte_write_only(ptr_index)
__rte_write_only_size(ptr_index, size_index)
__rte_read_write(ptr_index)
__rte_read_write_size(ptr_index, size_index)
__rte_no_access(ptr_index)
__rte_no_access_size(ptr_index, size_index)

> 
> 
> As for the lock annotations series, if you are not confident with the
> form I went with, I don't mind deferring to a later release.

The form follows the existing convention in rte_common.h, and I think we should stick with it.

> Though it adds more work on my pile like rebasing the vhost library.
> Additionnally, we lose the opportunity to catch introduction of new
> lock issues in the dpdk tree.

Conclusion:

The names I listed in this email, and what David already has in his lock annotation patch, are both in line with an existing convention already established in rte_common.h. So unless someone objects very soon, let's go for that.

Re: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Tyler Retzlaff]

On Mon, Feb 06, 2023 at 05:49:18PM +0100, Morten Brørup wrote:
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Monday, 6 February 2023 17.11
> > 
> > On Wed, Feb 1, 2023 at 2:16 PM Thomas Monjalon <thomas@monjalon.net>
> > wrote:
> > > > > I tend to prefer this kind of namespace as well.
> > > > > Let's compare different naming proposals,
> > > > > taking into account what we already have for some annotations,
> > > > > and what is proposed to be added in this patch and David's patch
> > > > > for lock annotations.
> > > >
> > > > David's lock annotations don't use a dedicated naming convention,
> > but simply replaces __attribute__(name(params)) with
> > __rte_name(params), e.g.:
> > > >
> > > > #define __rte_guarded_by(...) \
> > > >       __attribute__((guarded_by(__VA_ARGS__)))
> > > > #define __rte_exclusive_locks_required(...) \
> > > >       __attribute__((exclusive_locks_required(__VA_ARGS__)))
> > > > #define __rte_assert_exclusive_lock(...) \
> > > >       __attribute__((assert_exclusive_lock(__VA_ARGS__)))
> > > >
> > > > This follows the existing convention in rte_common.h, which is
> > easily readable, because they directly translate to GCC attribute
> > names, e.g.:
> > > >
> > > > #define __rte_warn_unused_result
> > __attribute__((warn_unused_result))
> > > > #define __rte_always_inline inline __attribute__((always_inline))
> > > > #define __rte_noinline __attribute__((noinline))
> > > > #define __rte_hot __attribute__((hot))
> > > > #define __rte_cold __attribute__((cold))
> > > >
> > > > I could follow this convention too, e.g.:
> > > >
> > > > #define __rte_nonnull(...) __attribute__((nonnull(__VA_ARGS__)))
> > > >
> > > > #define __rte_access_read_only(...) \
> > > >       __attribute__((access(read_only, __VA_ARGS__)))
> > > > #define __rte_access_write_only(...) \
> > > >       __attribute__((access(write_only, __VA_ARGS__)))
> > > >
> > > [...]
> > > > > Longer macro names may be better for code readers.
> > > >
> > > > You have a good point here, Thomas. It will also prevent using the
> > access mode attribute macros incorrectly.
> > > >
> > > > Let's proceed with two macros (without and with size parameter)
> > instead of the combined macros (with optional size parameter).
> > > >
> > > > Referring to the existing convention in rte_common.h, what do you
> > think about naming the new macros as follows?
> > > >
> > > > __rte_access_read_only(ptr_index)
> > > > __rte_access_read_only_size(ptr_index, size_index)
> > > >
> > > > Or just:
> > > >
> > > > __rte_read_only(ptr_index)
> > > > __rte_read_only_size(ptr_index, size_index)
> > >
> > > I think we don't need the word "access", so I prefer the second form.
> > >
> > > What about the proposal of having "param" in the name?
> > > We could also have "function" is some macro names.
> > > Does it bring a benefit?
> 
> The existing __rte_format_printf macro also follows the simple convention, and does not add "param" or "function" in the name:
> 
> #define __rte_format_printf(format_index, first_arg) \
> 	__attribute__((format(printf, format_index, first_arg)))
> 
> > >
> > > Please let's have a naming discussion with many opinions.
> > 
> > Thomas, I think naming convention/discussion is the most likely to
> > never conclude.
> 
> No one else seem to have strong enough opinions to join the discussion. So let's try to conclude the discussion.

+1 the value of getting this in with ~whatever names outweighs a
prolonged discussion about naming. my view is this isn't part of the
dpdk stable public api and it is incidental that the headers introduce
the names. if applications choose to consume them, we document that they
change from time to time without notice.

i also think if naming is of high importance it can be done post-merge.
slightly related it would be nice to establish a more streamlined
contribution process for non-semantic changes. it's painful to review
changes filled with gratuitous style refactors mixed amongst functional
change or code move / rename etc.. it's much easier to spot errors when
such changes are in separate series, so let's make integrating them a
fast path.

> 
> > 
> > Just copying my previous suggestion.
> > __rte_read_only_params(indexes...)
> > __rte_write_only_params(indexes...)
> > __rte_no_access_params(indexes...)
> > 
> > So if we are not going with the existing (kind of) convention to just
> > prefix with __rte_, I prefer Morten second form too and I have no
> > better idea.
> 
> I'm leaning towards following the existing convention in rte_common.h, and embrace Thomas' argument to make them more verbose in order to reduce the risk of wrong use. In other words, define these:
> 
> __rte_nonnull(...)
> __rte_read_only(ptr_index)
> __rte_read_only_size(ptr_index, size_index)
> __rte_write_only(ptr_index)
> __rte_write_only_size(ptr_index, size_index)
> __rte_read_write(ptr_index)
> __rte_read_write_size(ptr_index, size_index)
> __rte_no_access(ptr_index)
> __rte_no_access_size(ptr_index, size_index)
> 
> > 
> > 
> > As for the lock annotations series, if you are not confident with the
> > form I went with, I don't mind deferring to a later release.
> 
> The form follows the existing convention in rte_common.h, and I think we should stick with it.
> 
> > Though it adds more work on my pile like rebasing the vhost library.
> > Additionnally, we lose the opportunity to catch introduction of new
> > lock issues in the dpdk tree.
> 
> Conclusion:
> 
> The names I listed in this email, and what David already has in his lock annotation patch, are both in line with an existing convention already established in rte_common.h. So unless someone objects very soon, let's go for that.
> 

RE: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Morten Brørup]

PING bnx2x maintainers. Care to review this bugfix, so it can be included in 23.03?

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Monday, 16 January 2023 14.07
> 
> Bugfix: The vlan in the bulletin does not contain a VLAN header, only
> the
> VLAN ID, so only copy 2 byte, not 4. The target structure has padding
> after the field, so copying 2 byte too many is effectively harmless.
> There is no need to backport this patch.
> 
> Use RTE_PTR_ADD where copying arrays to the offset of a first field in
> a
> structure holding multiple fields, to avoid compiler warnings with
> decorated rte_memcpy.
> 
> Bugzilla ID: 1146
> 
> Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core")
> Cc: stephen@networkplumber.org
> Cc: rmody@marvell.com
> Cc: shshaikh@marvell.com
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> v7:
> * No changes.
> v6:
> * Add Fixes to patch description.
> * Fix checkpatch warnings.
> v5:
> * No changes.
> v4:
> * Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
> v3:
> * First patch in series.
> ---
>  drivers/net/bnx2x/bnx2x_stats.c | 11 +++++++----
>  drivers/net/bnx2x/bnx2x_vfpf.c  |  2 +-
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/bnx2x/bnx2x_stats.c
> b/drivers/net/bnx2x/bnx2x_stats.c
> index c07b01510a..bc4a8b8e71 100644
> --- a/drivers/net/bnx2x/bnx2x_stats.c
> +++ b/drivers/net/bnx2x/bnx2x_stats.c
> @@ -819,8 +819,9 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
> 
>      rte_memcpy(old, new, sizeof(struct nig_stats));
> 
> -    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats-
> >mac_stx[1]),
> -	   sizeof(struct mac_stx));
> +	rte_memcpy(RTE_PTR_ADD(estats,
> +			offsetof(struct bnx2x_eth_stats,
> rx_stat_ifhcinbadoctets_hi)),
> +			&pstats->mac_stx[1], sizeof(struct mac_stx));
>      estats->brb_drop_hi = pstats->brb_drop_hi;
>      estats->brb_drop_lo = pstats->brb_drop_lo;
> 
> @@ -1492,9 +1493,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
>  		REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
>  	if (!CHIP_IS_E3(sc)) {
>  		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
> -				&(sc->port.old_nig_stats.egress_mac_pkt0_lo),
> 2);
> +				RTE_PTR_ADD(&sc->port.old_nig_stats,
> +				offsetof(struct nig_stats,
> egress_mac_pkt0_lo)), 2);
>  		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
> -				&(sc->port.old_nig_stats.egress_mac_pkt1_lo),
> 2);
> +				RTE_PTR_ADD(&sc->port.old_nig_stats,
> +				offsetof(struct nig_stats,
> egress_mac_pkt1_lo)), 2);
>  	}
> 
>  	/* function stats */
> diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c
> b/drivers/net/bnx2x/bnx2x_vfpf.c
> index 63953c2979..87631c76ca 100644
> --- a/drivers/net/bnx2x/bnx2x_vfpf.c
> +++ b/drivers/net/bnx2x/bnx2x_vfpf.c
> @@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
>  	if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac, sc-
> >old_bulletin.mac, ETH_ALEN))
>  		rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
>  	if (valid_bitmap & (1 << VLAN_VALID))
> -		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan,
> RTE_VLAN_HLEN);
> +		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan,
> sizeof(bull->vlan));
> 
>  	sc->old_bulletin = *bull;
> 
> --
> 2.17.1

RE: [PATCH v7 2/4] event/dlb2: remove superfluous rte_memcpy

[Morten Brørup]

PING dlb2 maintainers. Care to review this, so it can be included in 23.03?

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Monday, 16 January 2023 14.07
> 
> Copying with the same src and dst address has no effect; removed to
> avoid compiler warning with decorated rte_memcpy.
> 
> Fixes: e7c9971a857a ("event/dlb2: add probe-time hardware init")
> Cc: timothy.mcdaniel@intel.com
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v7:
> * No changes.
> v6:
> * Add Fixes to patch description. (Stephen)
> v5:
> * First patch in series.
> ---
>  drivers/event/dlb2/dlb2.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
> index 60c5cd4804..03d32c779f 100644
> --- a/drivers/event/dlb2/dlb2.c
> +++ b/drivers/event/dlb2/dlb2.c
> @@ -215,7 +215,6 @@ static int
>  dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
>  {
>  	struct dlb2_hw_dev *handle = &dlb2->qm_instance;
> -	struct dlb2_hw_resource_info *dlb2_info = &handle->info;
>  	int num_ldb_ports;
>  	int ret;
> 
> @@ -277,8 +276,6 @@ dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
>  	handle->info.hw_rsrc_max.reorder_window_size =
>  		dlb2->hw_rsrc_query_results.num_hist_list_entries;
> 
> -	rte_memcpy(dlb2_info, &handle->info.hw_rsrc_max,
> sizeof(*dlb2_info));
> -
>  	return 0;
>  }
> 
> --
> 2.17.1

RE: [PATCH v7 3/4] net/mlx5: fix warning about rte_memcpy length

[Morten Brørup]

PING mlx5 maintainers. Care to review this minor improvement, so it can be included in 23.03?

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Monday, 16 January 2023 14.07
> 
> Use RTE_PTR_ADD where copying to the offset of a field in a structure
> holding multiple fields, to avoid compiler warnings with decorated
> rte_memcpy.
> 
> Fixes: 16a7dbc4f69006cc1c96ca2a2c6d3e3c51a2ff50 ("net/mlx5: make flow
> modify action list thread safe")
> Cc: xuemingl@nvidia.com
> Cc: matan@nvidia.com
> Cc: viacheslavo@nvidia.com
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> v7:
> * No changes.
> v6:
> * Add Fixes to patch description.
> v5:
> * First patch in series.
> ---
>  drivers/net/mlx5/mlx5_flow_dv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c
> index 62c38b87a1..dd9f5fda1a 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -5662,7 +5662,7 @@ flow_dv_modify_create_cb(void *tool_ctx, void
> *cb_ctx)
>  				   "cannot allocate resource memory");
>  		return NULL;
>  	}
> -	rte_memcpy(&entry->ft_type,
> +	rte_memcpy(RTE_PTR_ADD(entry, offsetof(typeof(*entry), ft_type)),
>  		   RTE_PTR_ADD(ref, offsetof(typeof(*ref), ft_type)),
>  		   key_len + data_len);
>  	if (entry->ft_type == MLX5DV_FLOW_TABLE_TYPE_FDB)
> --
> 2.17.1

RE: [PATCH v7 2/4] event/dlb2: remove superfluous rte_memcpy

[Sevincer, Abdullah]

Acked: by abdullah.sevincer@intel.com

-----Original Message-----
From: Morten Brørup <mb@smartsharesystems.com> 
Sent: Thursday, February 9, 2023 8:51 AM
To: dev@dpdk.org; roretzla@linux.microsoft.com; rmody@marvell.com; McDaniel, Timothy <timothy.mcdaniel@intel.com>; matan@nvidia.com; viacheslavo@nvidia.com
Cc: ruifeng.wang@arm.com; zhoumin@loongson.cn; drc@linux.vnet.ibm.com; kda@semihalf.com; Richardson, Bruce <bruce.richardson@intel.com>; konstantin.v.ananyev@yandex.ru
Subject: RE: [PATCH v7 2/4] event/dlb2: remove superfluous rte_memcpy

PING dlb2 maintainers. Care to review this, so it can be included in 23.03?

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Monday, 16 January 2023 14.07
> 
> Copying with the same src and dst address has no effect; removed to 
> avoid compiler warning with decorated rte_memcpy.
> 
> Fixes: e7c9971a857a ("event/dlb2: add probe-time hardware init")
> Cc: timothy.mcdaniel@intel.com
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v7:
> * No changes.
> v6:
> * Add Fixes to patch description. (Stephen)
> v5:
> * First patch in series.
> ---
>  drivers/event/dlb2/dlb2.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c 
> index 60c5cd4804..03d32c779f 100644
> --- a/drivers/event/dlb2/dlb2.c
> +++ b/drivers/event/dlb2/dlb2.c
> @@ -215,7 +215,6 @@ static int
>  dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)  {
>  	struct dlb2_hw_dev *handle = &dlb2->qm_instance;
> -	struct dlb2_hw_resource_info *dlb2_info = &handle->info;
>  	int num_ldb_ports;
>  	int ret;
> 
> @@ -277,8 +276,6 @@ dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
>  	handle->info.hw_rsrc_max.reorder_window_size =
>  		dlb2->hw_rsrc_query_results.num_hist_list_entries;
> 
> -	rte_memcpy(dlb2_info, &handle->info.hw_rsrc_max,
> sizeof(*dlb2_info));
> -
>  	return 0;
>  }
> 
> --
> 2.17.1

RE: [PATCH v7 2/4] event/dlb2: remove superfluous rte_memcpy

[Morten Brørup]

> From: Sevincer, Abdullah [mailto:abdullah.sevincer@intel.com]
> Sent: Thursday, 9 February 2023 19.50
> 
> Acked: by abdullah.sevincer@intel.com

Thanks.

Patchwork didn't catch it due to formatting, but the point is obvious:

Acked-by: Abdullah Sevincer <abdullah.sevincer@intel.com>

> 
> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Thursday, February 9, 2023 8:51 AM
> To: dev@dpdk.org; roretzla@linux.microsoft.com; rmody@marvell.com;
> McDaniel, Timothy <timothy.mcdaniel@intel.com>; matan@nvidia.com;
> viacheslavo@nvidia.com
> Cc: ruifeng.wang@arm.com; zhoumin@loongson.cn; drc@linux.vnet.ibm.com;
> kda@semihalf.com; Richardson, Bruce <bruce.richardson@intel.com>;
> konstantin.v.ananyev@yandex.ru
> Subject: RE: [PATCH v7 2/4] event/dlb2: remove superfluous rte_memcpy
> 
> PING dlb2 maintainers. Care to review this, so it can be included in
> 23.03?
> 
> > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > Sent: Monday, 16 January 2023 14.07
> >
> > Copying with the same src and dst address has no effect; removed to
> > avoid compiler warning with decorated rte_memcpy.
> >
> > Fixes: e7c9971a857a ("event/dlb2: add probe-time hardware init")
> > Cc: timothy.mcdaniel@intel.com
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > v7:
> > * No changes.
> > v6:
> > * Add Fixes to patch description. (Stephen)
> > v5:
> > * First patch in series.
> > ---
> >  drivers/event/dlb2/dlb2.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
> > index 60c5cd4804..03d32c779f 100644
> > --- a/drivers/event/dlb2/dlb2.c
> > +++ b/drivers/event/dlb2/dlb2.c
> > @@ -215,7 +215,6 @@ static int
> >  dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)  {
> >  	struct dlb2_hw_dev *handle = &dlb2->qm_instance;
> > -	struct dlb2_hw_resource_info *dlb2_info = &handle->info;
> >  	int num_ldb_ports;
> >  	int ret;
> >
> > @@ -277,8 +276,6 @@ dlb2_hw_query_resources(struct dlb2_eventdev
> *dlb2)
> >  	handle->info.hw_rsrc_max.reorder_window_size =
> >  		dlb2->hw_rsrc_query_results.num_hist_list_entries;
> >
> > -	rte_memcpy(dlb2_info, &handle->info.hw_rsrc_max,
> > sizeof(*dlb2_info));
> > -
> >  	return 0;
> >  }
> >
> > --
> > 2.17.1

RE: [PATCH v7 3/4] net/mlx5: fix warning about rte_memcpy length

[Slava Ovsiienko]

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Thursday, February 9, 2023 6:54 PM
> To: dev@dpdk.org; Matan Azrad <matan@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Xueming(Steven) Li <xuemingl@nvidia.com>
> Cc: ruifeng.wang@arm.com; zhoumin@loongson.cn; drc@linux.vnet.ibm.com;
> kda@semihalf.com; bruce.richardson@intel.com;
> konstantin.v.ananyev@yandex.ru; timothy.mcdaniel@intel.com;
> rmody@marvell.com; roretzla@linux.microsoft.com
> Subject: RE: [PATCH v7 3/4] net/mlx5: fix warning about rte_memcpy length
> 
> PING mlx5 maintainers. Care to review this minor improvement, so it can be
> included in 23.03?
> 
> > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > Sent: Monday, 16 January 2023 14.07
> >
> > Use RTE_PTR_ADD where copying to the offset of a field in a structure
> > holding multiple fields, to avoid compiler warnings with decorated
> > rte_memcpy.
> >
> > Fixes: 16a7dbc4f69006cc1c96ca2a2c6d3e3c51a2ff50 ("net/mlx5: make flow
> > modify action list thread safe")
> > Cc: xuemingl@nvidia.com
> > Cc: matan@nvidia.com
> > Cc: viacheslavo@nvidia.com
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

Marvell QLogic bnx2x PMD support status

[Morten Brørup]

Marvell bnx2x PMD maintainers are silent. Is the PMD not maintained anymore?

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Thursday, 9 February 2023 17.50
> To: dev@dpdk.org; rmody@marvell.com; [...]
> Cc: shshaikh@marvell.com; [...]
> Subject: RE: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy
> lengths
> 
> PING bnx2x maintainers. Care to review this bugfix, so it can be
> included in 23.03?
> 
> > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > Sent: Monday, 16 January 2023 14.07
> >
> > Bugfix: The vlan in the bulletin does not contain a VLAN header, only
> > the
> > VLAN ID, so only copy 2 byte, not 4. The target structure has padding
> > after the field, so copying 2 byte too many is effectively harmless.
> > There is no need to backport this patch.
> >
> > Use RTE_PTR_ADD where copying arrays to the offset of a first field in
> > a
> > structure holding multiple fields, to avoid compiler warnings with
> > decorated rte_memcpy.
> >
> > Bugzilla ID: 1146
> >
> > Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core")
> > Cc: stephen@networkplumber.org
> > Cc: rmody@marvell.com
> > Cc: shshaikh@marvell.com
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[David Marchand]

On Thu, Feb 9, 2023 at 5:49 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> PING bnx2x maintainers. Care to review this bugfix, so it can be included in 23.03?
>

Still no luck in getting attention from bnx2x maintainers.
One option is to declare this driver as UNMAINTAINED and disable its
compilation when the memcpy annotations are finalised and merged in a
next release.


-- 
David Marchand

Re: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Thomas Monjalon]

09/03/2023 11:25, David Marchand:
> On Thu, Feb 9, 2023 at 5:49 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > PING bnx2x maintainers. Care to review this bugfix, so it can be included in 23.03?
> >
> 
> Still no luck in getting attention from bnx2x maintainers.
> One option is to declare this driver as UNMAINTAINED and disable its
> compilation when the memcpy annotations are finalised and merged in a
> next release.

Yes that's an option.
Jerin, what is your opinion?

RE: [EXT] Re: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Akhil Goyal]

++Alok Prasad

> 09/03/2023 11:25, David Marchand:
> > On Thu, Feb 9, 2023 at 5:49 PM Morten Brørup
> <mb@smartsharesystems.com> wrote:
> > >
> > > PING bnx2x maintainers. Care to review this bugfix, so it can be included in
> 23.03?
> > >
> >
> > Still no luck in getting attention from bnx2x maintainers.
> > One option is to declare this driver as UNMAINTAINED and disable its
> > compilation when the memcpy annotations are finalised and merged in a
> > next release.
> 
> Yes that's an option.
> Jerin, what is your opinion?
> 

Re: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Devendra Singh Rawat]

-----Original Message-----
From: Akhil Goyal <gakhil@marvell.com>
Sent: 09 March 2023 21:41
To: Thomas Monjalon <thomas@monjalon.net>; David Marchand <david.marchand@redhat.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>
Cc: Morten Brørup <mb@smartsharesystems.com>; Rasesh Mody <rmody@marvell.com>; Shahed Shaikh <shshaikh@marvell.com>; dev@dpdk.org; Alok Prasad <palok@marvell.com>
Subject: RE: [EXT] Re: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

++Alok Prasad

> 09/03/2023 11:25, David Marchand:
> > On Thu, Feb 9, 2023 at 5:49 PM Morten Brørup
> <mb@smartsharesystems.com> wrote:
> > >
> > > PING bnx2x maintainers. Care to review this bugfix, so it can be 
> > > included in
> 23.03?
> > >
> >
> > Still no luck in getting attention from bnx2x maintainers.
> > One option is to declare this driver as UNMAINTAINED and disable its 
> > compilation when the memcpy annotations are finalised and merged in 
> > a next release.
> 
> Yes that's an option.
> Jerin, what is your opinion?
> 

Both the maintainers for bnx2x PMD have left Marvell, We are in process of finalizing new maintainers for this PMD.
Meanwhile I have reviewed the patch and I am Acking it.

Acked-by: Devendra Singh Rawat <dsinghrawat@marvell.com>

Re: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Stephen Hemminger]

On Thu, 9 Feb 2023 17:49:31 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:

> >      rte_memcpy(old, new, sizeof(struct nig_stats));
> > 
> > -    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats-  
> > >mac_stx[1]),  
> > -	   sizeof(struct mac_stx));
> > +	rte_memcpy(RTE_PTR_ADD(estats,
> > +			offsetof(struct bnx2x_eth_stats,
> > rx_stat_ifhcinbadoctets_hi)),
> > +			&pstats->mac_stx[1], sizeof(struct mac_stx));

Stop using rte_memcpy() in slow path like this.
memcpy() is just as fast, compiler can optimize, and the checking tools
are better with it.

RE: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Morten Brørup]

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Monday, 16 January 2023 14.07
> 
> Add nonnull function attribute to help the compiler detect a NULL
> pointer being passed to a function not accepting NULL pointers as an
> argument at build time.
> 
> Add access function attributes to tell the compiler how a function
> accesses memory pointed to by its pointer arguments.
> 
> Add these attributes to the rte_memcpy() function, as the first in
> hopefully many to come.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---

David,

Patch parts 1, 2 and 3 are essentially individual stand-alone patches, and can be merged as such.

I only made them part of this series because they were required for the primary patch 4.

When I have some time to spare, I will send a new patch, adding the nonnull and access attributes, based on the received feedback.


PS: This patch series got the wrong name by mistake.

Re: [PATCH v7 4/4] eal: add nonnull and access function attributes

[David Marchand]

Hello Morten,

On Tue, Apr 4, 2023 at 3:42 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> > Add nonnull function attribute to help the compiler detect a NULL
> > pointer being passed to a function not accepting NULL pointers as an
> > argument at build time.
> >
> > Add access function attributes to tell the compiler how a function
> > accesses memory pointed to by its pointer arguments.
> >
> > Add these attributes to the rte_memcpy() function, as the first in
> > hopefully many to come.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
>
> David,
>
> Patch parts 1, 2 and 3 are essentially individual stand-alone patches, and can be merged as such.

I am aware of them.

Btw, those were marked as "awaiting upstream", so I suspect someone
(you maybe?) changed their state manually.
This state is something used between subtree maintainers and main repository.

I fixed their state as NEW in patchwork yesterday so they can be
considered for merge.

>
> I only made them part of this series because they were required for the primary patch 4.
>
> When I have some time to spare, I will send a new patch, adding the nonnull and access attributes, based on the received feedback.

Ok.


-- 
David Marchand

RE: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Morten Brørup]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Tuesday, 4 April 2023 15.51
> 
> Hello Morten,
> 
> On Tue, Apr 4, 2023 at 3:42 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> > > Add nonnull function attribute to help the compiler detect a NULL
> > > pointer being passed to a function not accepting NULL pointers as an
> > > argument at build time.
> > >
> > > Add access function attributes to tell the compiler how a function
> > > accesses memory pointed to by its pointer arguments.
> > >
> > > Add these attributes to the rte_memcpy() function, as the first in
> > > hopefully many to come.
> > >
> > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > ---
> >
> > David,
> >
> > Patch parts 1, 2 and 3 are essentially individual stand-alone patches, and
> can be merged as such.
> 
> I am aware of them.
> 
> Btw, those were marked as "awaiting upstream", so I suspect someone
> (you maybe?) changed their state manually.
> This state is something used between subtree maintainers and main repository.

Yes, I changed their state, trying to help... Seems I was wrong. Thank you for informing me of the usage.

Do you know where I can find guidance to using DPDK Patchwork as a contributor? Or can I look for Linux Patchwork and assume it generally is used the same way?

> 
> I fixed their state as NEW in patchwork yesterday so they can be
> considered for merge.

Great, thanks.

> 
> >
> > I only made them part of this series because they were required for the
> primary patch 4.
> >
> > When I have some time to spare, I will send a new patch, adding the nonnull
> and access attributes, based on the received feedback.
> 
> Ok.
> 
> 
> --
> David Marchand

Re: [PATCH v7 4/4] eal: add nonnull and access function attributes

[David Marchand]

On Tue, Apr 4, 2023 at 4:02 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> > Btw, those were marked as "awaiting upstream", so I suspect someone
> > (you maybe?) changed their state manually.
> > This state is something used between subtree maintainers and main repository.
>
> Yes, I changed their state, trying to help... Seems I was wrong. Thank you for informing me of the usage.
>
> Do you know where I can find guidance to using DPDK Patchwork as a contributor? Or can I look for Linux Patchwork and assume it generally is used the same way?

In my experience, patchwork states are only touched by maintainers of a project.
I don't know if there is one globally accepted policy for patches states in pw.


In DPDK, we ask, in the contributors guide, that submitters update
their series with the "superseded" state.
The other states are manipulated by maintainers to sync (mainly the
"awaiting upstream") or to hide older patches (like the "archived"
state for resolved, as in accepted or rejected, patches sent in
previous releases).
I know some maintainers do set the "Under review" state, but this is
not done by everyone.


-- 
David Marchand

RE: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Morten Brørup]

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Monday, 6 February 2023 18.29
> 
> On Mon, Feb 06, 2023 at 05:49:18PM +0100, Morten Brørup wrote:
> > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > Sent: Monday, 6 February 2023 17.11
> > >
> > > On Wed, Feb 1, 2023 at 2:16 PM Thomas Monjalon <thomas@monjalon.net>
> > > wrote:

[...]

> >
> > I'm leaning towards following the existing convention in rte_common.h, and
> embrace Thomas' argument to make them more verbose in order to reduce the risk
> of wrong use. In other words, define these:
> >
> > __rte_nonnull(...)
> > __rte_read_only(ptr_index)
> > __rte_read_only_size(ptr_index, size_index)
> > __rte_write_only(ptr_index)
> > __rte_write_only_size(ptr_index, size_index)
> > __rte_read_write(ptr_index)
> > __rte_read_write_size(ptr_index, size_index)
> > __rte_no_access(ptr_index)
> > __rte_no_access_size(ptr_index, size_index)
> >
> > >
> > >
> > > As for the lock annotations series, if you are not confident with the
> > > form I went with, I don't mind deferring to a later release.
> >
> > The form follows the existing convention in rte_common.h, and I think we
> should stick with it.
> >
> > > Though it adds more work on my pile like rebasing the vhost library.
> > > Additionnally, we lose the opportunity to catch introduction of new
> > > lock issues in the dpdk tree.
> >
> > Conclusion:
> >
> > The names I listed in this email, and what David already has in his lock
> annotation patch, are both in line with an existing convention already
> established in rte_common.h. So unless someone objects very soon, let's go for
> that.

David, Thomas,

FYI:

I am deferring a new version this patch until a later DPDK release, so it doesn't get too much in the way of Tyler's MSVC patches.

Stretch goal: I'm considering if these new attributes could somehow also support MSVC, but let's not discuss that now!

PS: The other patches in the series are independent of this patch, and can be considered individually.

-Morten

RE: [PATCH v7 4/4] eal: add nonnull and access function attributes

[Morten Brørup]

David,

As mentioned below, the first 3 patches in this series [26561] can be considered as individual driver patches.

I guess this request was somehow missed. Should I delegate these 3 patches to the respective maintainers for the net-next-xxx trees in Patchwork, or how should I proceed?

Patches 1 and 2 are still valid as is (compared to the main branch). Patch 3 is a one-line modification where the modified line has moved further down.

[26561]: https://patchwork.dpdk.org/project/dpdk/list/?series=26561

Med venlig hilsen / Kind regards,
-Morten Brørup

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Monday, 8 May 2023 14.33
> 
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Monday, 6 February 2023 18.29
> >
> > On Mon, Feb 06, 2023 at 05:49:18PM +0100, Morten Brørup wrote:
> > > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > > Sent: Monday, 6 February 2023 17.11
> > > >
> > > > On Wed, Feb 1, 2023 at 2:16 PM Thomas Monjalon
> <thomas@monjalon.net>
> > > > wrote:
> 
> [...]
> 
> > >
> > > I'm leaning towards following the existing convention in
> rte_common.h, and
> > embrace Thomas' argument to make them more verbose in order to reduce
> the risk
> > of wrong use. In other words, define these:
> > >
> > > __rte_nonnull(...)
> > > __rte_read_only(ptr_index)
> > > __rte_read_only_size(ptr_index, size_index)
> > > __rte_write_only(ptr_index)
> > > __rte_write_only_size(ptr_index, size_index)
> > > __rte_read_write(ptr_index)
> > > __rte_read_write_size(ptr_index, size_index)
> > > __rte_no_access(ptr_index)
> > > __rte_no_access_size(ptr_index, size_index)
> > >
> > > >
> > > >
> > > > As for the lock annotations series, if you are not confident with
> the
> > > > form I went with, I don't mind deferring to a later release.
> > >
> > > The form follows the existing convention in rte_common.h, and I
> think we
> > should stick with it.
> > >
> > > > Though it adds more work on my pile like rebasing the vhost
> library.
> > > > Additionnally, we lose the opportunity to catch introduction of
> new
> > > > lock issues in the dpdk tree.
> > >
> > > Conclusion:
> > >
> > > The names I listed in this email, and what David already has in his
> lock
> > annotation patch, are both in line with an existing convention already
> > established in rte_common.h. So unless someone objects very soon,
> let's go for
> > that.
> 
> David, Thomas,
> 
> FYI:
> 
> I am deferring a new version this patch until a later DPDK release, so
> it doesn't get too much in the way of Tyler's MSVC patches.
> 
> Stretch goal: I'm considering if these new attributes could somehow also
> support MSVC, but let's not discuss that now!
> 
> PS: The other patches in the series are independent of this patch, and
> can be considered individually.
> 
> -Morten

Re: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Jerin Jacob]

On Thu, Mar 9, 2023 at 9:53 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Thu, 9 Feb 2023 17:49:31 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > >      rte_memcpy(old, new, sizeof(struct nig_stats));
> > >
> > > -    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats-
> > > >mac_stx[1]),
> > > -      sizeof(struct mac_stx));
> > > +   rte_memcpy(RTE_PTR_ADD(estats,
> > > +                   offsetof(struct bnx2x_eth_stats,
> > > rx_stat_ifhcinbadoctets_hi)),
> > > +                   &pstats->mac_stx[1], sizeof(struct mac_stx));
>
> Stop using rte_memcpy() in slow path like this.
> memcpy() is just as fast, compiler can optimize, and the checking tools
> are better with it.

+1

@Morten Brørup Could you send the next version? I am marking as Change
requested.

Re: [PATCH v7 2/4] event/dlb2: remove superfluous rte_memcpy

[Jerin Jacob]

On Fri, Feb 10, 2023 at 1:13 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Sevincer, Abdullah [mailto:abdullah.sevincer@intel.com]
> > Sent: Thursday, 9 February 2023 19.50
> >
> > Acked: by abdullah.sevincer@intel.com
>
> Thanks.
>
> Patchwork didn't catch it due to formatting, but the point is obvious:
>
> Acked-by: Abdullah Sevincer <abdullah.sevincer@intel.com>

Applied to dpdk-next-eventdev/for-main. Thanks

[PATCH v8] net/bnx2x: fix warnings about rte_memcpy lengths

[Morten Brørup]

Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
VLAN ID, so only copy 2 byte, not 4. The target structure has padding
after the field, so copying 2 byte too many is effectively harmless.
There is no need to backport this patch.

Use RTE_PTR_ADD where copying arrays to the offset of a first field in a
structure holding multiple fields, to avoid compiler warnings with
decorated rte_memcpy.

Bugzilla ID: 1146

Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core")
Cc: stephen@networkplumber.org
Cc: rmody@marvell.com
Cc: shshaikh@marvell.com
Cc: palok@marvell.com

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Devendra Singh Rawat <dsinghrawat@marvell.com>
---
v8:
* Use memcpy instead of rte_memcpy in slow path. (Stephen Hemminger)
v7:
* No changes.
v6:
* Add Fixes to patch description.
* Fix checkpatch warnings.
v5:
* No changes.
v4:
* Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
v3:
* First patch in series.
---
 drivers/net/bnx2x/bnx2x_stats.c | 14 ++++++++------
 drivers/net/bnx2x/bnx2x_vfpf.c  | 14 +++++++-------
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x_stats.c b/drivers/net/bnx2x/bnx2x_stats.c
index c07b01510a..8105375d44 100644
--- a/drivers/net/bnx2x/bnx2x_stats.c
+++ b/drivers/net/bnx2x/bnx2x_stats.c
@@ -114,7 +114,7 @@ bnx2x_hw_stats_post(struct bnx2x_softc *sc)
 
 	/* Update MCP's statistics if possible */
 	if (sc->func_stx) {
-		rte_memcpy(BNX2X_SP(sc, func_stats), &sc->func_stats,
+		memcpy(BNX2X_SP(sc, func_stats), &sc->func_stats,
 				sizeof(sc->func_stats));
 	}
 
@@ -817,10 +817,10 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
 			  etherstatspktsover1522octets);
     }
 
-    rte_memcpy(old, new, sizeof(struct nig_stats));
+    memcpy(old, new, sizeof(struct nig_stats));
 
-    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
-	   sizeof(struct mac_stx));
+	memcpy(RTE_PTR_ADD(estats, offsetof(struct bnx2x_eth_stats, rx_stat_ifhcinbadoctets_hi)),
+			&pstats->mac_stx[1], sizeof(struct mac_stx));
     estats->brb_drop_hi = pstats->brb_drop_hi;
     estats->brb_drop_lo = pstats->brb_drop_lo;
 
@@ -1492,9 +1492,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
 		REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
 	if (!CHIP_IS_E3(sc)) {
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
+				RTE_PTR_ADD(&sc->port.old_nig_stats,
+				offsetof(struct nig_stats, egress_mac_pkt0_lo)), 2);
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
+				RTE_PTR_ADD(&sc->port.old_nig_stats,
+				offsetof(struct nig_stats, egress_mac_pkt1_lo)), 2);
 	}
 
 	/* function stats */
diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfpf.c
index 63953c2979..5411df3a38 100644
--- a/drivers/net/bnx2x/bnx2x_vfpf.c
+++ b/drivers/net/bnx2x/bnx2x_vfpf.c
@@ -52,9 +52,9 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
 
 	/* check the mac address and VLAN and allocate memory if valid */
 	if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac, sc->old_bulletin.mac, ETH_ALEN))
-		rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
+		memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
 	if (valid_bitmap & (1 << VLAN_VALID))
-		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
+		memcpy(&bull->vlan, &sc->old_bulletin.vlan, sizeof(bull->vlan));
 
 	sc->old_bulletin = *bull;
 
@@ -569,7 +569,7 @@ bnx2x_vf_set_mac(struct bnx2x_softc *sc, int set)
 
 	bnx2x_check_bull(sc);
 
-	rte_memcpy(query->filters[0].mac, sc->link_params.mac_addr, ETH_ALEN);
+	memcpy(query->filters[0].mac, sc->link_params.mac_addr, ETH_ALEN);
 
 	bnx2x_add_tlv(sc, query, query->first_tlv.tl.length,
 		      BNX2X_VF_TLV_LIST_END,
@@ -583,9 +583,9 @@ bnx2x_vf_set_mac(struct bnx2x_softc *sc, int set)
 	while (BNX2X_VF_STATUS_FAILURE == reply->status &&
 			bnx2x_check_bull(sc)) {
 		/* A new mac was configured by PF for us */
-		rte_memcpy(sc->link_params.mac_addr, sc->pf2vf_bulletin->mac,
+		memcpy(sc->link_params.mac_addr, sc->pf2vf_bulletin->mac,
 				ETH_ALEN);
-		rte_memcpy(query->filters[0].mac, sc->pf2vf_bulletin->mac,
+		memcpy(query->filters[0].mac, sc->pf2vf_bulletin->mac,
 				ETH_ALEN);
 
 		rc = bnx2x_do_req4pf(sc, sc->vf2pf_mbox_mapping.paddr);
@@ -622,10 +622,10 @@ bnx2x_vf_config_rss(struct bnx2x_softc *sc,
 		      BNX2X_VF_TLV_LIST_END,
 		      sizeof(struct channel_list_end_tlv));
 
-	rte_memcpy(query->rss_key, params->rss_key, sizeof(params->rss_key));
+	memcpy(query->rss_key, params->rss_key, sizeof(params->rss_key));
 	query->rss_key_size = T_ETH_RSS_KEY;
 
-	rte_memcpy(query->ind_table, params->ind_table, T_ETH_INDIRECTION_TABLE_SIZE);
+	memcpy(query->ind_table, params->ind_table, T_ETH_INDIRECTION_TABLE_SIZE);
 	query->ind_table_size = T_ETH_INDIRECTION_TABLE_SIZE;
 
 	query->rss_result_mask = params->rss_result_mask;
-- 
2.17.1

[PATCH v9] net/bnx2x: fix warnings about rte_memcpy lengths

[Morten Brørup]

Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
VLAN ID, so only copy 2 byte, not 4. The target structure has padding
after the field, so copying 2 byte too many is effectively harmless.
There is no need to backport this patch.

Use RTE_PTR_ADD where copying arrays to the offset of a first field in a
structure holding multiple fields, to avoid compiler warnings with
decorated rte_memcpy.

Bugzilla ID: 1146

Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core")
Cc: stephen@networkplumber.org
Cc: rmody@marvell.com
Cc: shshaikh@marvell.com
Cc: palok@marvell.com

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Devendra Singh Rawat <dsinghrawat@marvell.com>
---
v9:
* Fix checkpatch warning about spaces.
v8:
* Use memcpy instead of rte_memcpy in slow path. (Stephen Hemminger)
v7:
* No changes.
v6:
* Add Fixes to patch description.
* Fix checkpatch warnings.
v5:
* No changes.
v4:
* Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
v3:
* First patch in series.
---
 drivers/net/bnx2x/bnx2x_stats.c | 14 ++++++++------
 drivers/net/bnx2x/bnx2x_vfpf.c  | 14 +++++++-------
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x_stats.c b/drivers/net/bnx2x/bnx2x_stats.c
index c07b01510a..8105375d44 100644
--- a/drivers/net/bnx2x/bnx2x_stats.c
+++ b/drivers/net/bnx2x/bnx2x_stats.c
@@ -114,7 +114,7 @@ bnx2x_hw_stats_post(struct bnx2x_softc *sc)
 
 	/* Update MCP's statistics if possible */
 	if (sc->func_stx) {
-		rte_memcpy(BNX2X_SP(sc, func_stats), &sc->func_stats,
+		memcpy(BNX2X_SP(sc, func_stats), &sc->func_stats,
 				sizeof(sc->func_stats));
 	}
 
@@ -817,10 +817,10 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
 			  etherstatspktsover1522octets);
     }
 
-    rte_memcpy(old, new, sizeof(struct nig_stats));
+	memcpy(old, new, sizeof(struct nig_stats));
 
-    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
-	   sizeof(struct mac_stx));
+	memcpy(RTE_PTR_ADD(estats, offsetof(struct bnx2x_eth_stats, rx_stat_ifhcinbadoctets_hi)),
+			&pstats->mac_stx[1], sizeof(struct mac_stx));
     estats->brb_drop_hi = pstats->brb_drop_hi;
     estats->brb_drop_lo = pstats->brb_drop_lo;
 
@@ -1492,9 +1492,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
 		REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
 	if (!CHIP_IS_E3(sc)) {
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
+				RTE_PTR_ADD(&sc->port.old_nig_stats,
+				offsetof(struct nig_stats, egress_mac_pkt0_lo)), 2);
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
+				RTE_PTR_ADD(&sc->port.old_nig_stats,
+				offsetof(struct nig_stats, egress_mac_pkt1_lo)), 2);
 	}
 
 	/* function stats */
diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfpf.c
index 63953c2979..5411df3a38 100644
--- a/drivers/net/bnx2x/bnx2x_vfpf.c
+++ b/drivers/net/bnx2x/bnx2x_vfpf.c
@@ -52,9 +52,9 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
 
 	/* check the mac address and VLAN and allocate memory if valid */
 	if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac, sc->old_bulletin.mac, ETH_ALEN))
-		rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
+		memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
 	if (valid_bitmap & (1 << VLAN_VALID))
-		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
+		memcpy(&bull->vlan, &sc->old_bulletin.vlan, sizeof(bull->vlan));
 
 	sc->old_bulletin = *bull;
 
@@ -569,7 +569,7 @@ bnx2x_vf_set_mac(struct bnx2x_softc *sc, int set)
 
 	bnx2x_check_bull(sc);
 
-	rte_memcpy(query->filters[0].mac, sc->link_params.mac_addr, ETH_ALEN);
+	memcpy(query->filters[0].mac, sc->link_params.mac_addr, ETH_ALEN);
 
 	bnx2x_add_tlv(sc, query, query->first_tlv.tl.length,
 		      BNX2X_VF_TLV_LIST_END,
@@ -583,9 +583,9 @@ bnx2x_vf_set_mac(struct bnx2x_softc *sc, int set)
 	while (BNX2X_VF_STATUS_FAILURE == reply->status &&
 			bnx2x_check_bull(sc)) {
 		/* A new mac was configured by PF for us */
-		rte_memcpy(sc->link_params.mac_addr, sc->pf2vf_bulletin->mac,
+		memcpy(sc->link_params.mac_addr, sc->pf2vf_bulletin->mac,
 				ETH_ALEN);
-		rte_memcpy(query->filters[0].mac, sc->pf2vf_bulletin->mac,
+		memcpy(query->filters[0].mac, sc->pf2vf_bulletin->mac,
 				ETH_ALEN);
 
 		rc = bnx2x_do_req4pf(sc, sc->vf2pf_mbox_mapping.paddr);
@@ -622,10 +622,10 @@ bnx2x_vf_config_rss(struct bnx2x_softc *sc,
 		      BNX2X_VF_TLV_LIST_END,
 		      sizeof(struct channel_list_end_tlv));
 
-	rte_memcpy(query->rss_key, params->rss_key, sizeof(params->rss_key));
+	memcpy(query->rss_key, params->rss_key, sizeof(params->rss_key));
 	query->rss_key_size = T_ETH_RSS_KEY;
 
-	rte_memcpy(query->ind_table, params->ind_table, T_ETH_INDIRECTION_TABLE_SIZE);
+	memcpy(query->ind_table, params->ind_table, T_ETH_INDIRECTION_TABLE_SIZE);
 	query->ind_table_size = T_ETH_INDIRECTION_TABLE_SIZE;
 
 	query->rss_result_mask = params->rss_result_mask;
-- 
2.17.1

Re: [PATCH v9] net/bnx2x: fix warnings about rte_memcpy lengths

[Jerin Jacob]

On Fri, Feb 23, 2024 at 7:30 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
> VLAN ID, so only copy 2 byte, not 4. The target structure has padding
> after the field, so copying 2 byte too many is effectively harmless.
> There is no need to backport this patch.
>
> Use RTE_PTR_ADD where copying arrays to the offset of a first field in a
> structure holding multiple fields, to avoid compiler warnings with
> decorated rte_memcpy.
>
> Bugzilla ID: 1146
>
> Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core")
> Cc: stephen@networkplumber.org
> Cc: rmody@marvell.com
> Cc: shshaikh@marvell.com
> Cc: palok@marvell.com
>
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Devendra Singh Rawat <dsinghrawat@marvell.com>
> ---
> v9:
> * Fix checkpatch warning about spaces.

Fixed the following issues[1] and updated the git commit as follows
and applied to dpdk-next-net-mrvl/for-main. Thanks

    net/bnx2x: fix warnings about memcpy lengths

    The vlan in the bulletin does not contain a VLAN header, only the
    VLAN ID, so only copy 2 byte, not 4. The target structure has padding
    after the field, so copying 2 byte too many is effectively harmless.
    Fix it by using generic memcpy version instead of specialized
    rte version as it not used in fast path.

    Also, Use RTE_PTR_ADD where copying arrays to the offset of a first field
    in a structure holding multiple fields, to avoid compiler warnings with
    decorated memcpy.

    Bugzilla ID: 1146
    Fixes: 540a211084a7 ("bnx2x: driver core")
    Cc: stable@dpdk.org

    Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
    Acked-by: Devendra Singh Rawat <dsinghrawat@marvell.com>


[1]
Wrong headline format:
        net/bnx2x: fix warnings about rte_memcpy lengths
Wrong tag:
        Bugfix: The vlan in the bulletin does not contain a VLAN
header, only the
Is it candidate for Cc: stable@dpdk.org backport?
        net/bnx2x: fix warnings about rte_memcpy lengths

Invalid patch(es) found - checked 1 patch
check-git-log failed

### [PATCH] net/bnx2x: fix warnings about rte_memcpy lengths

WARNING:BAD_FIXES_TAG: Please use correct Fixes: style 'Fixes: <12
chars of sha1> ("<title line>")' - ie: 'Fixes: 540a211084a7 ("bnx2x:
driver core")'
#20:
Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core")

total: 0 errors, 1 warnings, 0 checks, 76 lines checked

0/1 valid patch
checkpatch failed

RE: [PATCH v9] net/bnx2x: fix warnings about rte_memcpy lengths

[Morten Brørup]

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Monday, 26 February 2024 09.34
> 
> On Fri, Feb 23, 2024 at 7:30 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > Bugfix: The vlan in the bulletin does not contain a VLAN header, only
> the
> > VLAN ID, so only copy 2 byte, not 4. The target structure has padding
> > after the field, so copying 2 byte too many is effectively harmless.
> > There is no need to backport this patch.
> >
> > Use RTE_PTR_ADD where copying arrays to the offset of a first field in
> a
> > structure holding multiple fields, to avoid compiler warnings with
> > decorated rte_memcpy.
> >
> > Bugzilla ID: 1146
> >
> > Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core")
> > Cc: stephen@networkplumber.org
> > Cc: rmody@marvell.com
> > Cc: shshaikh@marvell.com
> > Cc: palok@marvell.com
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > Acked-by: Devendra Singh Rawat <dsinghrawat@marvell.com>
> > ---
> > v9:
> > * Fix checkpatch warning about spaces.
> 
> Fixed the following issues[1] and updated the git commit as follows
> and applied to dpdk-next-net-mrvl/for-main. Thanks

Thank you, Jerin.

[...]

> Is it candidate for Cc: stable@dpdk.org backport?

No, I don't think so:
1. The extra 2 byte copy is effectively harmless due to padding, as mentioned in the commit message.
2. The decorated rte_memcpy (if work on that patch series is ever resumed) is an improvement, not a bug fix, and will not be backported. So the memcpy parts of this patch are irrelevant for the stable versions.

Re: [PATCH v9] net/bnx2x: fix warnings about rte_memcpy lengths

[Stephen Hemminger]

On Fri, 23 Feb 2024 15:00:56 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:

> Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
> VLAN ID, so only copy 2 byte, not 4. The target structure has padding
> after the field, so copying 2 byte too many is effectively harmless.
> There is no need to backport this patch.
> 
> Use RTE_PTR_ADD where copying arrays to the offset of a first field in a
> structure holding multiple fields, to avoid compiler warnings with
> decorated rte_memcpy.
> 
> Bugzilla ID: 1146
> 
> Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core")
> Cc: stephen@networkplumber.org
> Cc: rmody@marvell.com
> Cc: shshaikh@marvell.com
> Cc: palok@marvell.com
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Devendra Singh Rawat <dsinghrawat@marvell.com>
> ---
> v9:
> * Fix checkpatch warning about spaces.
> v8:
> * Use memcpy instead of rte_memcpy in slow path. (Stephen Hemminger)
> v7:
> * No changes.
> v6:
> * Add Fixes to patch description.
> * Fix checkpatch warnings.
> v5:
> * No changes.
> v4:
> * Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
> v3:
> * First patch in series.
> ---
>  drivers/net/bnx2x/bnx2x_stats.c | 14 ++++++++------
>  drivers/net/bnx2x/bnx2x_vfpf.c  | 14 +++++++-------
>  2 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/bnx2x/bnx2x_stats.c b/drivers/net/bnx2x/bnx2x_stats.c
> index c07b01510a..8105375d44 100644
> --- a/drivers/net/bnx2x/bnx2x_stats.c
> +++ b/drivers/net/bnx2x/bnx2x_stats.c
	}
>  
> @@ -817,10 +817,10 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
>  			  etherstatspktsover1522octets);
>      }
>  
> -    rte_memcpy(old, new, sizeof(struct nig_stats));
> +	memcpy(old, new, sizeof(struct nig_stats));

This could just be structure assignment which is type safe.

	*new = *old;

>  
> -    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
> -	   sizeof(struct mac_stx));
> +	memcpy(RTE_PTR_ADD(estats, offsetof(struct bnx2x_eth_stats, rx_stat_ifhcinbadoctets_hi)),
> +			&pstats->mac_stx[1], sizeof(struct mac_stx));
>      estats->brb_drop_hi = pstats->brb_drop_hi;
>      estats->brb_drop_lo = pstats->brb_drop_lo;
>  
> @@ -1492,9 +1492,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
>  		REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
>  	if (!CHIP_IS_E3(sc)) {
>  		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
> -				&(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
> +				RTE_PTR_ADD(&sc->port.old_nig_stats,
> +				offsetof(struct nig_stats, egress_mac_pkt0_lo)), 2);
>  		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
> -				&(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
> +				RTE_PTR_ADD(&sc->port.old_nig_stats,
> +				offsetof(struct nig_stats, egress_mac_pkt1_lo)), 2);
>  	}
>  
>  	/* function stats */
> diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfpf.c
> index 63953c2979..5411df3a38 100644
> --- a/drivers/net/bnx2x/bnx2x_vfpf.c
> +++ b/drivers/net/bnx2x/bnx2x_vfpf.c
> @@ -52,9 +52,9 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
>  
>  	/* check the mac address and VLAN and allocate memory if valid */
>  	if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac, sc->old_bulletin.mac, ETH_ALEN))
> -		rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
> +		memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
>  	if (valid_bitmap & (1 << VLAN_VALID))
> -		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
> +		memcpy(&bull->vlan, &sc->old_bulletin.vlan, sizeof(bull->vlan));
>  
>  	sc->old_bulletin = *bull;
>  
> @@ -569,7 +569,7 @@ bnx2x_vf_set_mac(struct bnx2x_softc *sc, int set)
>  
>  	bnx2x_check_bull(sc);
>  
> -	rte_memcpy(query->filters[0].mac, sc->link_params.mac_addr, ETH_ALEN);
> +	memcpy(query->filters[0].mac, sc->link_params.mac_addr, ETH_ALEN);
>  
>  	bnx2x_add_tlv(sc, query, query->first_tlv.tl.length,
>  		      BNX2X_VF_TLV_LIST_END,
> @@ -583,9 +583,9 @@ bnx2x_vf_set_mac(struct bnx2x_softc *sc, int set)
>  	while (BNX2X_VF_STATUS_FAILURE == reply->status &&
>  			bnx2x_check_bull(sc)) {
>  		/* A new mac was configured by PF for us */
> -		rte_memcpy(sc->link_params.mac_addr, sc->pf2vf_bulletin->mac,
> +		memcpy(sc->link_params.mac_addr, sc->pf2vf_bulletin->mac,
>  				ETH_ALEN);
> -		rte_memcpy(query->filters[0].mac, sc->pf2vf_bulletin->mac,
> +		memcpy(query->filters[0].mac, sc->pf2vf_bulletin->mac,
>  				ETH_ALEN);
>  
>  		rc = bnx2x_do_req4pf(sc, sc->vf2pf_mbox_mapping.paddr);
> @@ -622,10 +622,10 @@ bnx2x_vf_config_rss(struct bnx2x_softc *sc,
>  		      BNX2X_VF_TLV_LIST_END,
>  		      sizeof(struct channel_list_end_tlv));
>  
> -	rte_memcpy(query->rss_key, params->rss_key, sizeof(params->rss_key));
> +	memcpy(query->rss_key, params->rss_key, sizeof(params->rss_key));
>  	query->rss_key_size = T_ETH_RSS_KEY;
>  
> -	rte_memcpy(query->ind_table, params->ind_table, T_ETH_INDIRECTION_TABLE_SIZE);
> +	memcpy(query->ind_table, params->ind_table, T_ETH_INDIRECTION_TABLE_SIZE);
>  	query->ind_table_size = T_ETH_INDIRECTION_TABLE_SIZE;
>  
>  	query->rss_result_mask = params->rss_result_mask;

The driver is also using rte_memcpy for 2 byte values in bnx2x.c.

Another issue is the driver is using one element array as a flexible array.
A good static checker should catch this and report out of bounds access.

union bnx2x_stats_show_data {
    uint32_t op; /* ioctl sub-command */

    struct {
	uint32_t num; /* return number of stats */
	uint32_t len; /* length of each string item */
    } desc;

    /* variable length... */
    char str[1]; /* holds names of desc.num stats, each desc.len in length */

    /* variable length... */
    uint64_t stats[1]; /* holds all stats */
};

Re: [PATCH v9] net/bnx2x: fix warnings about rte_memcpy lengths

[Jerin Jacob]

On Mon, Feb 26, 2024 at 8:17 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > Sent: Monday, 26 February 2024 09.34
> >
> > On Fri, Feb 23, 2024 at 7:30 PM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> > >
> > > Bugfix: The vlan in the bulletin does not contain a VLAN header, only
> > the
> > > VLAN ID, so only copy 2 byte, not 4. The target structure has padding
> > > after the field, so copying 2 byte too many is effectively harmless.
> > > There is no need to backport this patch.
> > >
> > > Use RTE_PTR_ADD where copying arrays to the offset of a first field in
> > a
> > > structure holding multiple fields, to avoid compiler warnings with
> > > decorated rte_memcpy.
> > >
> > > Bugzilla ID: 1146
> > >
> > > Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core")
> > > Cc: stephen@networkplumber.org
> > > Cc: rmody@marvell.com
> > > Cc: shshaikh@marvell.com
> > > Cc: palok@marvell.com
> > >
> > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > Acked-by: Devendra Singh Rawat <dsinghrawat@marvell.com>
> > > ---
> > > v9:
> > > * Fix checkpatch warning about spaces.
> >
> > Fixed the following issues[1] and updated the git commit as follows
> > and applied to dpdk-next-net-mrvl/for-main. Thanks
>
> Thank you, Jerin.
>
> [...]
>
> > Is it candidate for Cc: stable@dpdk.org backport?
>
> No, I don't think so:
> 1. The extra 2 byte copy is effectively harmless due to padding, as mentioned in the commit message.
> 2. The decorated rte_memcpy (if work on that patch series is ever resumed) is an improvement, not a bug fix, and will not be backported. So the memcpy parts of this patch are irrelevant for the stable versions.


Shall remove Fixes: tag then?. Since the patch has a Fixes tag, I
thought good to merge to stable as it is fixing.

Also, could you comment on @Stephen Hemminger latest comments, Should
I wait for new version? or new changes can go as separate patches.


>

RE: [PATCH v9] net/bnx2x: fix warnings about rte_memcpy lengths

[Morten Brørup]

+To: Raslan, regarding MLX5 patch

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Tuesday, 27 February 2024 12.01
> 
> On Mon, Feb 26, 2024 at 8:17 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > Sent: Monday, 26 February 2024 09.34
> > >
> > > On Fri, Feb 23, 2024 at 7:30 PM Morten Brørup <mb@smartsharesystems.com>
> > > wrote:
> > > >
> > > > Bugfix: The vlan in the bulletin does not contain a VLAN header, only
> > > the
> > > > VLAN ID, so only copy 2 byte, not 4. The target structure has padding
> > > > after the field, so copying 2 byte too many is effectively harmless.
> > > > There is no need to backport this patch.
> > > >
> > > > Use RTE_PTR_ADD where copying arrays to the offset of a first field in
> > > a
> > > > structure holding multiple fields, to avoid compiler warnings with
> > > > decorated rte_memcpy.
> > > >
> > > > Bugzilla ID: 1146
> > > >
> > > > Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core")
> > > > Cc: stephen@networkplumber.org
> > > > Cc: rmody@marvell.com
> > > > Cc: shshaikh@marvell.com
> > > > Cc: palok@marvell.com
> > > >
> > > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > > Acked-by: Devendra Singh Rawat <dsinghrawat@marvell.com>
> > > > ---
> > > > v9:
> > > > * Fix checkpatch warning about spaces.
> > >
> > > Fixed the following issues[1] and updated the git commit as follows
> > > and applied to dpdk-next-net-mrvl/for-main. Thanks
> >
> > Thank you, Jerin.
> >
> > [...]
> >
> > > Is it candidate for Cc: stable@dpdk.org backport?
> >
> > No, I don't think so:
> > 1. The extra 2 byte copy is effectively harmless due to padding, as
> mentioned in the commit message.
> > 2. The decorated rte_memcpy (if work on that patch series is ever resumed)
> is an improvement, not a bug fix, and will not be backported. So the memcpy
> parts of this patch are irrelevant for the stable versions.
> 
> 
> Shall remove Fixes: tag then?. Since the patch has a Fixes tag, I
> thought good to merge to stable as it is fixing.

Although the patch formally fixes a bug, the bug is harmless, so I don't think it is worth the effort backporting.
I don't know the policy for Fixes: tags in such cases. However you proceed with it is perfectly fine with me.

> 
> Also, could you comment on @Stephen Hemminger latest comments, Should
> I wait for new version? or new changes can go as separate patches.

I'm not providing a new version of this patch.

This patch was part of a series, where its rte_memcpy changes were required for the primary patch in the series [1]. The purpose of the primary patch was to tighten rte_memcpy's parameter requirements by adding access-mode attributes.

[1]: https://patchwork.dpdk.org/project/dpdk/patch/20230116130724.50277-4-mb@smartsharesystems.com/

It was not really my intention to fix other things in the drivers I submitted patches for. That was only a positive side effect. ;-)

@Raslan: Also, the patch for the mlx5 driver [2] has seen no progress for a year, so I'm going to abandon it. I will resume it if I start working on the decorated rte_memcpy again. Should I change it's state to Not Applicable or something else?

[2]: https://patchwork.dpdk.org/project/dpdk/patch/20230116130724.50277-3-mb@smartsharesystems.com/

Re: [PATCH v9] net/bnx2x: fix warnings about rte_memcpy lengths

[Stephen Hemminger]

On Tue, 27 Feb 2024 12:27:31 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:

> > > 1. The extra 2 byte copy is effectively harmless due to padding, as  
> > mentioned in the commit message.  
> > > 2. The decorated rte_memcpy (if work on that patch series is ever resumed)  
> > is an improvement, not a bug fix, and will not be backported. So the memcpy
> > parts of this patch are irrelevant for the stable versions.

The function rte_memcpy only exists because glibc and gcc version of memcpy
is not optimized enough. Would love to see it go away in future.

RE: [PATCH v9] net/bnx2x: fix warnings about rte_memcpy lengths

[Raslan Darawsheh]

Hi,
> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Tuesday, February 27, 2024 1:28 PM
> To: Jerin Jacob <jerinjacobk@gmail.com>; Raslan Darawsheh
> <rasland@nvidia.com>
> Cc: julien_dpdk@jaube.fr; dev@dpdk.org; stephen@networkplumber.org;
> rmody@marvell.com; shshaikh@marvell.com; palok@marvell.com
> Subject: RE: [PATCH v9] net/bnx2x: fix warnings about rte_memcpy lengths
> 
> +To: Raslan, regarding MLX5 patch
> 
> > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > Sent: Tuesday, 27 February 2024 12.01
> >
> > On Mon, Feb 26, 2024 at 8:17 PM Morten Brørup
> > <mb@smartsharesystems.com>
> > wrote:
> > >
> > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > > Sent: Monday, 26 February 2024 09.34
> > > >
> > > > On Fri, Feb 23, 2024 at 7:30 PM Morten Brørup
> > > > <mb@smartsharesystems.com>
> > > > wrote:
> > > > >
> > > > > Bugfix: The vlan in the bulletin does not contain a VLAN header,
> > > > > only
> > > > the
> > > > > VLAN ID, so only copy 2 byte, not 4. The target structure has
> > > > > padding after the field, so copying 2 byte too many is effectively
> harmless.
> > > > > There is no need to backport this patch.
> > > > >
> > > > > Use RTE_PTR_ADD where copying arrays to the offset of a first
> > > > > field in
> > > > a
> > > > > structure holding multiple fields, to avoid compiler warnings
> > > > > with decorated rte_memcpy.
> > > > >
> > > > > Bugzilla ID: 1146
> > > > >
> > > > > Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x:
> driver
> > > > > core")
> > > > > Cc: stephen@networkplumber.org
> > > > > Cc: rmody@marvell.com
> > > > > Cc: shshaikh@marvell.com
> > > > > Cc: palok@marvell.com
> > > > >
> > > > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > > > Acked-by: Devendra Singh Rawat <dsinghrawat@marvell.com>
> > > > > ---
> > > > > v9:
> > > > > * Fix checkpatch warning about spaces.
> > > >
> > > > Fixed the following issues[1] and updated the git commit as
> > > > follows and applied to dpdk-next-net-mrvl/for-main. Thanks
> > >
> > > Thank you, Jerin.
> > >
> > > [...]
> > >
> > > > Is it candidate for Cc: stable@dpdk.org backport?
> > >
> > > No, I don't think so:
> > > 1. The extra 2 byte copy is effectively harmless due to padding, as
> > mentioned in the commit message.
> > > 2. The decorated rte_memcpy (if work on that patch series is ever
> > > resumed)
> > is an improvement, not a bug fix, and will not be backported. So the
> > memcpy parts of this patch are irrelevant for the stable versions.
> >
> >
> > Shall remove Fixes: tag then?. Since the patch has a Fixes tag, I
> > thought good to merge to stable as it is fixing.
> 
> Although the patch formally fixes a bug, the bug is harmless, so I don't think it
> is worth the effort backporting.
> I don't know the policy for Fixes: tags in such cases. However you proceed with
> it is perfectly fine with me.
> 
> >
> > Also, could you comment on @Stephen Hemminger latest comments,
> Should
> > I wait for new version? or new changes can go as separate patches.
> 
> I'm not providing a new version of this patch.
> 
> This patch was part of a series, where its rte_memcpy changes were required
> for the primary patch in the series [1]. The purpose of the primary patch was
> to tighten rte_memcpy's parameter requirements by adding access-mode
> attributes.
> 
> [1]:
> https://patchwork.dpdk.org/project/dpdk/patch/20230116130724.50277-
> 4-mb@smartsharesystems.com/
> 
> It was not really my intention to fix other things in the drivers I submitted
> patches for. That was only a positive side effect. ;-)
> 
> @Raslan: Also, the patch for the mlx5 driver [2] has seen no progress for a
> year, so I'm going to abandon it. I will resume it if I start working on the
> decorated rte_memcpy again. Should I change it's state to Not Applicable or
> something else?
> 
> [2]:
> https://patchwork.dpdk.org/project/dpdk/patch/20230116130724.50277-
> 3-mb@smartsharesystems.com/
I think I might have forgotten it for some reason, 
I can pick it still as I can see it's still applicable I will take it to next-net-mlx directly, 

Kindest regards
Raslan Darawsheh

Re: [PATCH v9] net/bnx2x: fix warnings about rte_memcpy lengths

[Bruce Richardson]

On Tue, Feb 27, 2024 at 11:06:04AM -0800, Stephen Hemminger wrote:
> On Tue, 27 Feb 2024 12:27:31 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > > > 1. The extra 2 byte copy is effectively harmless due to padding, as  
> > > mentioned in the commit message.  
> > > > 2. The decorated rte_memcpy (if work on that patch series is ever resumed)  
> > > is an improvement, not a bug fix, and will not be backported. So the memcpy
> > > parts of this patch are irrelevant for the stable versions.
> 
> The function rte_memcpy only exists because glibc and gcc version of memcpy
> is not optimized enough. Would love to see it go away in future.

Definite +1.

RE: [PATCH v7 3/4] net/mlx5: fix warning about rte_memcpy length

[Raslan Darawsheh]

Hi,
> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Monday, January 16, 2023 3:07 PM
> To: dev@dpdk.org; roretzla@linux.microsoft.com; rmody@marvell.com;
> timothy.mcdaniel@intel.com; Matan Azrad <matan@nvidia.com>; Slava
> Ovsiienko <viacheslavo@nvidia.com>
> Cc: ruifeng.wang@arm.com; zhoumin@loongson.cn;
> drc@linux.vnet.ibm.com; kda@semihalf.com; bruce.richardson@intel.com;
> konstantin.v.ananyev@yandex.ru; Morten Brørup
> <mb@smartsharesystems.com>; Xueming(Steven) Li <xuemingl@nvidia.com>
> Subject: [PATCH v7 3/4] net/mlx5: fix warning about rte_memcpy length
> 
> Use RTE_PTR_ADD where copying to the offset of a field in a structure holding
> multiple fields, to avoid compiler warnings with decorated rte_memcpy.
> 
> Fixes: 16a7dbc4f69006cc1c96ca2a2c6d3e3c51a2ff50 ("net/mlx5: make flow
> modify action list thread safe")
> Cc: xuemingl@nvidia.com
> Cc: matan@nvidia.com
> Cc: viacheslavo@nvidia.com
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>

Patch applied to next-net-mlx,

Kindest regards
Raslan Darawsheh