> From: Thomas Monjalon [mailto:tho...@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.march...@redhat.com]
> > > > Sent: Tuesday, 31 January 2023 12.15
> > > >
> > > > On Wed, Jan 18, 2023 at 9:31 AM Morten Brørup
> > > > <m...@smartsharesystems.com> wrote:
> > > > >
> > > > > +To: Thomas & David, you probably have some opinions on this
> too!
> > > > >
> > > > > > From: Tyler Retzlaff [mailto:roret...@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.yi...@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 <m...@smartsharesystems.com>
> > > > > > > > > Acked-by: Tyler Retzlaff <roret...@linux.microsoft.com>
> > > > > > > > > Reviewed-by: Ruifeng Wang <ruifeng.w...@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.
