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

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

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.