From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 26A8842410; Wed, 18 Jan 2023 18:23:15 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0A50B40223; Wed, 18 Jan 2023 18:23:15 +0100 (CET) Received: from mail-pf1-f174.google.com (mail-pf1-f174.google.com [209.85.210.174]) by mails.dpdk.org (Postfix) with ESMTP id D07AA400D6 for ; Wed, 18 Jan 2023 18:23:13 +0100 (CET) Received: by mail-pf1-f174.google.com with SMTP id s3so24218666pfd.12 for ; Wed, 18 Jan 2023 09:23:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=/388Q4/HqBMvXUhTFpWEtLc/YeyC6MTD+cTQ0U8KqIs=; b=4smGdyl9B7EHxLSnNlHebmK9aN5Rg703bJpP4mzDNYBvnvwdSLVMgcXdR/c/gGYucl H0wGUuwyjsUhc5CK4Yd9AFsAuhP2/6LoMCiVQ4PcgHX7c/QtrXvhuEb/D0QunJXLIdch EXf6lzYGp36iEGfxKxwgV8H/U5CiZKQkFzzCH4IqiPYuz05ObZgwX3ZC0NS7TD1NmrXX iSdf7jGCxNIW1JWfdjYdowVCdu8nTyeUify2kDciIfn5GobMxOofJOODxbjVlVFzmQNT PC81erpevcfB1JiRatMUTuETvH/IG9lpwhPCUvbAjGSLUq7QhA7gSim7h8Z5Vf8WbfBd ZvrA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=/388Q4/HqBMvXUhTFpWEtLc/YeyC6MTD+cTQ0U8KqIs=; b=t13JiwDFsg0pIYyiSf8POTeVl+HsDPhAmO1a3+RRwoF55EHECoNlABhjIXdiSwWW8q 4PW9GZMTCNmNr8gC5haIW0lz9w+ixnvcX3xtJmn7EAskgVfe3fTn05prHMOfPWnR6qtj wIlr8MSWvPC3O9xxzwMn+yOlDCZfYWOqqrs97H1gCM3SXrRj4wWBm3OO7ENIRAUePAE8 DpqbdCasI0ppaAaLQY3cTYRnE2yD1OjRcx/DqO4J1rEsBAn23JkwZsYiPc5CBvwwoE46 t5QmHHsIhx5Fv2R0nzoBdlbUvjCNvLTNlLvUPyPuhPzxpvaTlbsnHkGtCGzTwTNNxmMh A4Sw== X-Gm-Message-State: AFqh2kp8jEfyN7nLElymm3mGVjni8r/sGGRDcMO/ppPkljhFwk4MheZ1 56vj2Ozueavu0TrNNCtEcS/tzg== X-Google-Smtp-Source: AMrXdXvTYZ9nXMuDGf7F1PtZDH+Dru4QN7NCYiZ84TsK0qJoxySSgXXumFXx9BVsSh6BopH5pN5z6w== X-Received: by 2002:aa7:9557:0:b0:58a:66a8:edf5 with SMTP id w23-20020aa79557000000b0058a66a8edf5mr8658356pfq.3.1674062592760; Wed, 18 Jan 2023 09:23:12 -0800 (PST) Received: from hermes.local (204-195-120-218.wavecable.com. [204.195.120.218]) by smtp.gmail.com with ESMTPSA id u127-20020a626085000000b00580fb018e4bsm18531394pfb.211.2023.01.18.09.23.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 18 Jan 2023 09:23:12 -0800 (PST) Date: Wed, 18 Jan 2023 09:23:10 -0800 From: Stephen Hemminger To: Morten =?UTF-8?B?QnLDuHJ1cA==?= Cc: "Tyler Retzlaff" , , , , "Ferruh Yigit" , , , , , , , , , , Subject: Re: [PATCH v7 4/4] eal: add nonnull and access function attributes Message-ID: <20230118092310.2dba407d@hermes.local> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87683@smartserver.smartshare.dk> References: <20221202153432.131023-1-mb@smartsharesystems.com> <20230116130724.50277-1-mb@smartsharesystems.com> <20230116130724.50277-4-mb@smartsharesystems.com> <98CBD80474FA8B44BF855DF32C47DC35D8767B@smartserver.smartshare.dk> <20230117211656.GA30892@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35D87683@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Wed, 18 Jan 2023 09:31:42 +0100 Morten Br=C3=B8rup wrote: > > > So I decided for this order in the names (treating =20 > > nonnull/access_mode as "country" and param/params as "city"), also > > somewhat looking at the __rte_deprecated and __rte_deprecated_msg(msg) > > macros. =20 > > > > > > I have no strong preference either, so if anyone does, please speak = =20 > > up. =20 > > > > > > Slightly related, also note this: > > > > > > The nonnull macro is plural (_params), because it can take multiple = =20 > > pointer parameter indexes. =20 > > > The access mode macros are singular (_param), because they only take = =20 > > one pointer parameter index, and the optional size parameter index. =20 > > > > > > I considered splitting up the access mode macros even more, making =20 > > 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). =20 > > > =20 > >=20 > > 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? > >=20 > > https://learn.microsoft.com/en-us/cpp/code-quality/using-sal- > > annotations-to-reduce-c-cpp-code-defects?view=3Dmsvc-170 > >=20 > > anyway, just a thought. no serious response required here. =20 >=20 > Excellent input, Tyler! >=20 > 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. >=20 > Microsoft's Source Code Annotation Language (SAL) seems very good, based = on its finer granularity than GCC's attributes (which in comparison seem ad= ded as an afterthought, not cleanly structured like SAL). I have only skimm= ed the documentation, but that is my immediate impression of it. >=20 > SAL uses a completely different syntax than GCC attributes, and Microsoft= happens to also use memcpy() as an example in the documentation referred t= o: >=20 > void * memcpy( > _Out_writes_bytes_all_(count) void *dest, > _In_reads_bytes_(count) const void *src, > size_t count > ); >=20 > Going back to how we can handle this in DPDK, we can either: >=20 > 1. Not annotate the functions at all, and miss out on finding the errors = for us. >=20 > 2. Invent our own language (or find something existing) for function head= ers, and use a parser to convert them to compiler specific C/C++ headers wh= en building the code. >=20 > 3a. Keep loading on attributes, with empty macros for unsupported compile= rs. >=20 > 3b. Keep loading on attributes, with empty macros for unsupported compile= rs. But limit ourselves to GCC/Clang style attributes. >=20 > 3c. Keep loading on attributes, with empty macros for unsupported compile= rs. But limit ourselves to Microsoft SAL style attributes. >=20 > 3d. Keep loading on attributes, with empty macros for unsupported compile= rs. But limit ourselves to the most relevant attributes, using performance = and/or bug detection as criteria when considering relevance. >=20 > I am strongly against both 1 and 2. >=20 > 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 C= I for detecting bugs. But then application developers would not benefit, be= cause they don't run their code through the DPDK CI. So I am also against t= his. >=20 > I think 3d (keep loading on attributes, but only the most relevant ones) = is the best choice. >=20 > GCC/Clang style attributes are already supported as macros prefixed by __= rte, so let's not change the way we do that. >=20 > 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 compil= ers. This would allow using Microsoft SAL annotations directly in the DPDK = code. Looks like SAL was developed outside of all the other compilers, and probab= ly 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 th= e automated tooling that supports it. Doing more annotation than the CI system uses is worthle= ss. It would be good if there was a common set support by VS, Gcc, and Clang wi= th DPDK macros for that. We already have annotations for format and allocations.