From: Stephen Hemminger <stephen@networkplumber.org>
To: "Morten Brørup" <mb@smartsharesystems.com>
Cc: "Tyler Retzlaff" <roretzla@linux.microsoft.com>,
<thomas@monjalon.net>, <david.marchand@redhat.com>,
<bruce.richardson@intel.com>,
"Ferruh Yigit" <ferruh.yigit@amd.com>, <dev@dpdk.org>,
<rmody@marvell.com>, <timothy.mcdaniel@intel.com>,
<matan@nvidia.com>, <viacheslavo@nvidia.com>,
<ruifeng.wang@arm.com>, <zhoumin@loongson.cn>,
<drc@linux.vnet.ibm.com>, <kda@semihalf.com>,
<konstantin.v.ananyev@yandex.ru>
Subject: Re: [PATCH v7 4/4] eal: add nonnull and access function attributes
Date: Wed, 18 Jan 2023 09:23:10 -0800 [thread overview]
Message-ID: <20230118092310.2dba407d@hermes.local> (raw)
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87683@smartserver.smartshare.dk>
On Wed, 18 Jan 2023 09:31:42 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:
> > > 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.
>
> 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
> );
>
> 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.
>
> 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.
>
> 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.
Looks like SAL was developed outside of all the other compilers, and probably pre-dates it.
Having had to deal with it, my impression was it that it became a nuisance on a large code base.
The value starts to drop off fast. And any annotation is only as good as the automated tooling
that supports it. Doing more annotation than the CI system uses is worthless.
It would be good if there was a common set support by VS, Gcc, and Clang with DPDK macros
for that. We already have annotations for format and allocations.
next prev parent reply other threads:[~2023-01-18 17:23 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-02 15:34 [PATCH] " Morten Brørup
2022-12-02 20:02 ` Tyler Retzlaff
2022-12-03 14:22 ` [PATCH v2] " Morten Brørup
2022-12-05 10:17 ` Ruifeng Wang
2022-12-12 7:40 ` Morten Brørup
2022-12-28 10:27 ` [PATCH v3 1/2] " Morten Brørup
2022-12-28 10:27 ` [PATCH v3 2/2] net/bnx2x: fix warnings about rte_memcopy lengths Morten Brørup
2022-12-28 11:40 ` [PATCH v4 1/2] eal: add nonnull and access function attributes Morten Brørup
2022-12-28 11:40 ` [PATCH v4 2/2] net/bnx2x: fix warnings about rte_memcpy lengths Morten Brørup
2022-12-28 15:10 ` [PATCH v5 1/4] eal: add nonnull and access function attributes Morten Brørup
2022-12-28 15:10 ` [PATCH v5 2/4] net/bnx2x: fix warnings about rte_memcpy lengths Morten Brørup
2022-12-28 16:13 ` Stanisław Kardach
2022-12-28 16:38 ` Morten Brørup
2022-12-28 17:03 ` Stephen Hemminger
2022-12-28 17:37 ` Morten Brørup
2023-01-09 10:36 ` David Marchand
2022-12-28 15:10 ` [PATCH v5 3/4] event/dlb2: remove superfluous rte_memcpy Morten Brørup
2022-12-28 17:01 ` Stephen Hemminger
2022-12-28 15:10 ` [PATCH v5 4/4] net/mlx5: fix warning about rte_memcpy length Morten Brørup
2023-01-09 11:08 ` [PATCH v5 1/4] eal: add nonnull and access function attributes Thomas Monjalon
2023-01-09 12:16 ` Morten Brørup
2023-01-09 11:22 ` David Marchand
2023-01-09 12:28 ` Morten Brørup
2023-01-16 12:49 ` Morten Brørup
2023-01-16 12:44 ` [PATCH v6 1/4] net/bnx2x: fix warnings about rte_memcpy lengths Morten Brørup
2023-01-16 12:44 ` [PATCH v6 2/4] event/dlb2: remove superfluous rte_memcpy Morten Brørup
2023-01-16 12:44 ` [PATCH v6 3/4] net/mlx5: fix warning about rte_memcpy length Morten Brørup
2023-01-16 12:44 ` [PATCH v6 4/4] eal: add nonnull and access function attributes Morten Brørup
2023-01-16 13:07 ` [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths Morten Brørup
2023-01-16 13:07 ` [PATCH v7 2/4] event/dlb2: remove superfluous rte_memcpy Morten Brørup
2023-02-09 16:51 ` Morten Brørup
2023-02-09 18:50 ` Sevincer, Abdullah
2023-02-10 7:43 ` Morten Brørup
2024-02-23 13:19 ` Jerin Jacob
2024-02-23 13:49 ` [PATCH v8] net/bnx2x: fix warnings about rte_memcpy lengths Morten Brørup
2024-02-23 14:00 ` [PATCH v9] " Morten Brørup
2024-02-26 8:34 ` Jerin Jacob
2024-02-26 14:47 ` Morten Brørup
2024-02-27 11:00 ` Jerin Jacob
2024-02-27 11:27 ` Morten Brørup
2024-02-27 19:06 ` Stephen Hemminger
2024-02-28 9:02 ` Bruce Richardson
2024-02-28 8:27 ` Raslan Darawsheh
2024-02-26 21:45 ` Stephen Hemminger
2023-01-16 13:07 ` [PATCH v7 3/4] net/mlx5: fix warning about rte_memcpy length Morten Brørup
2023-02-09 16:54 ` Morten Brørup
2023-02-10 9:13 ` Slava Ovsiienko
2024-03-13 10:00 ` Raslan Darawsheh
2023-01-16 13:07 ` [PATCH v7 4/4] eal: add nonnull and access function attributes Morten Brørup
2023-01-16 17:02 ` Ferruh Yigit
2023-01-17 8:19 ` Morten Brørup
2023-01-17 21:16 ` Tyler Retzlaff
2023-01-18 8:31 ` Morten Brørup
2023-01-18 17:23 ` Stephen Hemminger [this message]
2023-01-31 11:14 ` David Marchand
2023-01-31 12:23 ` Morten Brørup
2023-01-31 18:26 ` Tyler Retzlaff
2023-01-31 22:52 ` Thomas Monjalon
2023-02-01 12:50 ` Morten Brørup
2023-02-01 13:15 ` Thomas Monjalon
2023-02-06 16:11 ` David Marchand
2023-02-06 16:49 ` Morten Brørup
2023-02-06 17:28 ` Tyler Retzlaff
2023-05-08 12:32 ` Morten Brørup
2023-10-13 23:31 ` Morten Brørup
2023-04-04 13:41 ` Morten Brørup
2023-04-04 13:51 ` David Marchand
2023-04-04 14:01 ` Morten Brørup
2023-04-04 14:19 ` David Marchand
2023-02-09 16:49 ` [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths Morten Brørup
2023-02-26 9:21 ` Marvell QLogic bnx2x PMD support status Morten Brørup
2023-03-09 10:25 ` [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths David Marchand
2023-03-09 10:33 ` Thomas Monjalon
2023-03-09 16:11 ` [EXT] " Akhil Goyal
[not found] ` <SJ0PR18MB390039A8C34E485F8D2D518EA1B59@SJ0PR18MB3900.namprd18.prod.outlook.com>
2023-03-09 16:19 ` Devendra Singh Rawat
2023-03-09 16:23 ` Stephen Hemminger
2024-02-23 12:39 ` Jerin Jacob
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230118092310.2dba407d@hermes.local \
--to=stephen@networkplumber.org \
--cc=bruce.richardson@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=drc@linux.vnet.ibm.com \
--cc=ferruh.yigit@amd.com \
--cc=kda@semihalf.com \
--cc=konstantin.v.ananyev@yandex.ru \
--cc=matan@nvidia.com \
--cc=mb@smartsharesystems.com \
--cc=rmody@marvell.com \
--cc=roretzla@linux.microsoft.com \
--cc=ruifeng.wang@arm.com \
--cc=thomas@monjalon.net \
--cc=timothy.mcdaniel@intel.com \
--cc=viacheslavo@nvidia.com \
--cc=zhoumin@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).