[v7,4/4] eal: add nonnull and access function attributes
Checks
Commit Message
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(+)
Comments
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)
> {
> 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
> > +
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.
+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.
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.
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.
> 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!
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!
>
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.
> 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.
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
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.
> 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.
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.
>
> 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.
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.
> 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
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.
> 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
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
@@ -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)
{
@@ -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)
{
@@ -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.
@@ -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)
{
@@ -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)
{