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

[Morten Brørup]

> From: Ferruh Yigit [mailto:ferruh.yigit at 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 at smartsharesystems.com>
> > Acked-by: Tyler Retzlaff <roretzla at linux.microsoft.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang at 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
> > +