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

["Morten Brørup" <mb@smartsharesystems.com>]

> 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".