[v5,1/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 attribute to tell the compiler how a function
accesses its pointer arguments.
Add these attributes to the rte_memcpy() function, as the first in
hopefully many to come.
v5:
* No changes.
v4:
* No changes.
v3:
* No changes.
v2:
* Only define "nonnull" for GCC and CLANG.
* Append _param/_params to prepare for possible future attributes
attached directly to the individual parameters, like __rte_unused.
* Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints about
GCC_VERSION being undefined.
* Try to fix Doxygen compliants.
Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
lib/eal/arm/include/rte_memcpy_32.h | 8 ++++++++
lib/eal/arm/include/rte_memcpy_64.h | 6 ++++++
lib/eal/include/rte_common.h | 29 +++++++++++++++++++++++++++++
lib/eal/ppc/include/rte_memcpy.h | 3 +++
lib/eal/x86/include/rte_memcpy.h | 6 ++++++
5 files changed, 52 insertions(+)
Comments
28/12/2022 16:10, Morten Brørup:
> Add "nonnull" function attribute to help the compiler detect a NULL
> pointer being passed to a function not accepting NULL pointers as an
> argument at build time.
>
> Add "access" function attribute to tell the compiler how a function
> accesses its pointer arguments.
Why access specification is needed?
Isn't it enough to have the const keyword?
I'm afraid we are going to make the code ugly to read with such attribute.
On Wed, Dec 28, 2022 at 4:10 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> Add "nonnull" function attribute to help the compiler detect a NULL
> pointer being passed to a function not accepting NULL pointers as an
> argument at build time.
>
> Add "access" function attribute to tell the compiler how a function
> accesses its pointer arguments.
>
> Add these attributes to the rte_memcpy() function, as the first in
> hopefully many to come.
>
Compilation is broken starting first patch, so patches must be
reordered to have the fixes first.
> v5:
> * No changes.
> v4:
> * No changes.
> v3:
> * No changes.
> v2:
> * Only define "nonnull" for GCC and CLANG.
> * Append _param/_params to prepare for possible future attributes
> attached directly to the individual parameters, like __rte_unused.
> * Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints about
> GCC_VERSION being undefined.
> * Try to fix Doxygen compliants.
Patch history should be put as annotations (i.e. after --- but before
patch content).
>
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
[snip]
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index 15765b408d..6e4011aa85 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -149,6 +149,35 @@ typedef uint16_t unaligned_uint16_t;
> __attribute__((format(printf, format_index, first_arg)))
> #endif
>
> +/**
> + * Check pointer arguments at compile-time.
> + *
> + * @param ...
> + * Comma separated list of parameter indexes of pointer arguments.
> + */
> +#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
> +#define __rte_nonnull_params(...) \
> + __attribute__((nonnull(__VA_ARGS__)))
> +#else
> +#define __rte_nonnull_params(...)
> +#endif
> +
> +/**
> + * Tells compiler about the access mode of a pointer argument.
> + *
> + * @param access_mode
> + * Access mode: read_only, read_write, write_only, or none.
> + * @param ...
> + * Parameter index of pointer argument.
> + * Optionally followeded by comma and parameter index of size argument.
> + */
> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
> +#define __rte_access_param(access_mode, ...) \
> + __attribute__((access(access_mode, __VA_ARGS__)))
> +#else
> +#define __rte_access_param(access_mode, ...)
> +#endif
> +
This is tightly bound to gcc syntax.
With dedicated macros (which I find easier to read too), we can hope
to adapt to other compilers if some of them add support for this kind
of code cookies.
__rte_read_only_params(indexes...)
__rte_write_only_params(indexes...)
__rte_no_access_params(indexes...)
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, 9 January 2023 12.09
>
> 28/12/2022 16:10, Morten Brørup:
> > Add "nonnull" function attribute to help the compiler detect a NULL
> > pointer being passed to a function not accepting NULL pointers as an
> > argument at build time.
> >
> > Add "access" function attribute to tell the compiler how a function
> > accesses its pointer arguments.
>
> Why access specification is needed?
> Isn't it enough to have the const keyword?
No, it the const keyword is not enough. The read_only access attribute implies a stronger guarantee than the const qualifier which, when cast away from a pointer, does not prevent the pointed-to object from being modified [1].
[1]: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html
These attributes help finding bugs at build time (which I consider rather important). Proof: The other patches in the series are bugs revealed by using this attribute - bugs that were completely undetected without these attributes.
I guess they might also somehow help the optimizer, similar to "const" and "restrict". I noticed that we define __rte_weak, but not __rte_pure, so perhaps I should add that too?
> I'm afraid we are going to make the code ugly to read with such
> attribute.
The same can be said about ASAN. Ugly, but helpful.
In the extreme, the same could be said about the "const" and "restrict" keywords... The more decoration on the functions, the uglier the code. :-)
If you prefer, I could split the __rte_access_param(access_mode, ...) up into one macro per access mode, so this:
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static inline void *
rte_memcpy_func(void *dst, const void *src, size_t n)
would be shortened to this instead:
+__rte_params_nonnull(1, 2)
+__rte_param_writeonly(1, 3)
+__rte_param_readonly(2, 3)
static inline void *
rte_memcpy_func(void *dst, const void *src, size_t n)
or something else.
But I don't think it's the improvement you are hoping for. In essence, it is only a change of the names of the macros.
Please note: I prefer keeping the word "params" in the _nonnull macro, so we are prepared for defining an __rte_nonnull macro (without parameters) to be used with function parameters like the C22 NonNull keyword. Keeping "param" in the name(s) of the access macro(s) would ensure similar future proofing. That, and the similarity with the __rte_nonnull_params() name was the reason for the access macro name to include "param".
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Monday, 9 January 2023 12.22
> attributes
>
> On Wed, Dec 28, 2022 at 4:10 PM Morten Brørup
> <mb@smartsharesystems.com> wrote:
> >
> > Add "nonnull" function attribute to help the compiler detect a NULL
> > pointer being passed to a function not accepting NULL pointers as an
> > argument at build time.
> >
> > Add "access" function attribute to tell the compiler how a function
> > accesses its pointer arguments.
> >
> > Add these attributes to the rte_memcpy() function, as the first in
> > hopefully many to come.
> >
>
> Compilation is broken starting first patch, so patches must be
> reordered to have the fixes first.
Will do.
>
>
> > v5:
> > * No changes.
> > v4:
> > * No changes.
> > v3:
> > * No changes.
> > v2:
> > * Only define "nonnull" for GCC and CLANG.
> > * Append _param/_params to prepare for possible future attributes
> > attached directly to the individual parameters, like __rte_unused.
> > * Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints
> about
> > GCC_VERSION being undefined.
> > * Try to fix Doxygen compliants.
>
> Patch history should be put as annotations (i.e. after --- but before
> patch content).
Will do.
[...]
> This is tightly bound to gcc syntax.
> With dedicated macros (which I find easier to read too), we can hope
> to adapt to other compilers if some of them add support for this kind
> of code cookies.
> __rte_read_only_params(indexes...)
> __rte_write_only_params(indexes...)
> __rte_no_access_params(indexes...)
I agree. Splitting the generic access macro up into dedicated access macros is probably better.
> From: Morten Brørup
> Sent: Monday, 9 January 2023 13.28
>
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Monday, 9 January 2023 12.22
> > attributes
> >
> > On Wed, Dec 28, 2022 at 4:10 PM Morten Brørup
> > <mb@smartsharesystems.com> wrote:
> > >
> > > Add "nonnull" function attribute to help the compiler detect a NULL
> > > pointer being passed to a function not accepting NULL pointers as
> an
> > > argument at build time.
> > >
> > > Add "access" function attribute to tell the compiler how a function
> > > accesses its pointer arguments.
> > >
> > > Add these attributes to the rte_memcpy() function, as the first in
> > > hopefully many to come.
> > >
> >
> > Compilation is broken starting first patch, so patches must be
> > reordered to have the fixes first.
>
> Will do.
>
> >
> >
> > > v5:
> > > * No changes.
> > > v4:
> > > * No changes.
> > > v3:
> > > * No changes.
> > > v2:
> > > * Only define "nonnull" for GCC and CLANG.
> > > * Append _param/_params to prepare for possible future attributes
> > > attached directly to the individual parameters, like
> __rte_unused.
> > > * Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints
> > about
> > > GCC_VERSION being undefined.
> > > * Try to fix Doxygen compliants.
> >
> > Patch history should be put as annotations (i.e. after --- but before
> > patch content).
>
> Will do.
>
> [...]
>
> > This is tightly bound to gcc syntax.
> > With dedicated macros (which I find easier to read too), we can hope
> > to adapt to other compilers if some of them add support for this kind
> > of code cookies.
> > __rte_read_only_params(indexes...)
> > __rte_write_only_params(indexes...)
> > __rte_no_access_params(indexes...)
>
> I agree. Splitting the generic access macro up into dedicated access
> macros is probably better.
Version 6 of this patch series is here:
https://patchwork.dpdk.org/project/dpdk/list/?series=26560
Please ignore the name of the series.
-Morten
@@ -14,6 +14,8 @@ extern "C" {
#include "generic/rte_memcpy.h"
+#include <rte_common.h>
+
#ifdef RTE_ARCH_ARM_NEON_MEMCPY
#ifndef __ARM_NEON
@@ -125,6 +127,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
memcpy((dst), (src), (n)) : \
rte_memcpy_func((dst), (src), (n)); })
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static inline void *
rte_memcpy_func(void *dst, const void *src, size_t n)
{
@@ -290,6 +295,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
memcpy(dst, src, 256);
}
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static inline void *
rte_memcpy(void *dst, const void *src, size_t n)
{
@@ -282,6 +282,9 @@ void rte_memcpy_ge64(uint8_t *dst, const uint8_t *src, size_t n)
}
#if RTE_CACHE_LINE_SIZE >= 128
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static __rte_always_inline
void *rte_memcpy(void *dst, const void *src, size_t n)
{
@@ -303,6 +306,9 @@ void *rte_memcpy(void *dst, const void *src, size_t n)
}
#else
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static __rte_always_inline
void *rte_memcpy(void *dst, const void *src, size_t n)
{
@@ -149,6 +149,35 @@ typedef uint16_t unaligned_uint16_t;
__attribute__((format(printf, format_index, first_arg)))
#endif
+/**
+ * Check pointer arguments at compile-time.
+ *
+ * @param ...
+ * Comma separated list of parameter indexes of pointer arguments.
+ */
+#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
+#define __rte_nonnull_params(...) \
+ __attribute__((nonnull(__VA_ARGS__)))
+#else
+#define __rte_nonnull_params(...)
+#endif
+
+/**
+ * Tells compiler about the access mode of a pointer argument.
+ *
+ * @param access_mode
+ * Access mode: read_only, read_write, write_only, or none.
+ * @param ...
+ * Parameter index of pointer argument.
+ * Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_access_param(access_mode, ...) \
+ __attribute__((access(access_mode, __VA_ARGS__)))
+#else
+#define __rte_access_param(access_mode, ...)
+#endif
+
/**
* Tells compiler that the function returns a value that points to
* memory, where the size is given by the one or two arguments.
@@ -84,6 +84,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
memcpy((dst), (src), (n)) : \
rte_memcpy_func((dst), (src), (n)); })
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static inline void *
rte_memcpy_func(void *dst, const void *src, size_t n)
{
@@ -42,6 +42,9 @@ extern "C" {
* @return
* Pointer to the destination data.
*/
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static __rte_always_inline void *
rte_memcpy(void *dst, const void *src, size_t n);
@@ -859,6 +862,9 @@ rte_memcpy_aligned(void *dst, const void *src, size_t n)
return ret;
}
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static __rte_always_inline void *
rte_memcpy(void *dst, const void *src, size_t n)
{