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 477B041B93; Tue, 31 Jan 2023 23:52:24 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 24DD340684; Tue, 31 Jan 2023 23:52:24 +0100 (CET) Received: from wout4-smtp.messagingengine.com (wout4-smtp.messagingengine.com [64.147.123.20]) by mails.dpdk.org (Postfix) with ESMTP id B9AB24067B for ; Tue, 31 Jan 2023 23:52:21 +0100 (CET) Received: from compute6.internal (compute6.nyi.internal [10.202.2.47]) by mailout.west.internal (Postfix) with ESMTP id 2D11B32001FC; Tue, 31 Jan 2023 17:52:19 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute6.internal (MEProxy); Tue, 31 Jan 2023 17:52:20 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm3; t=1675205538; x= 1675291938; bh=fT8SD/XYfeHTkNmsjx4dczoViUZpQmw80RfZ35OZpnw=; b=C UzCmRK3ib0hB1KCnButn3TDd1DCHmmnyxmftfiMxedi9vfly5h9oEveXErclwZdF HMRZgiJ3FvN+NtidOuJpdK9h0HHd/of2JaZ5fnnf1mYT9HI0+Qp8aYVYSY6Xpo/C H/BxE7WNHF4fldwHILf6ApESaGiWJRSDU5QSyM1MQRzZBd4nnzghkttwI+JflI0w icXJTTx8BEgu1Wc8geV/mndkfOv5Nwe+ETIMgd85gpiw2Zh4OH4J+7+LLs0Q4gIy 5C49tcjeRzxO/U5RbV4zONe1nVAjHq4KYuBMpRIoejm475F6DzOkXxsdKHvEEJLu SBhOq/MKAnL5ofA7QK8IA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:feedback-id:feedback-id:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm3; t=1675205538; x= 1675291938; bh=fT8SD/XYfeHTkNmsjx4dczoViUZpQmw80RfZ35OZpnw=; b=C Er/zg4Q2UF7w/1OcyLHq+ZE1vpn1qgszSUBYvQLyJ72MhytnVgoAbIXSyGWAUPeY 4gvlrQzxeVpO7REynML7DYs2EVlBu6AuL7HLKzhi478wNe/g/4kan6Wl2xpDpXvn DaMIXcZ3tjJbI0KpUe0XAz+KoET/ZmoQfxjaUt/95UHvSWVGBZXnRif/oG7IYcvw jG7AtuyvGoPmhep7c4trYavbwL8Zn5AkCx4nVqGqJU2XQNO9dg3vAg/IEKgznurB wp6PnPtMj/4iJTlTCRTrLY6Hzrc4fKDhHDOhsZu4KC//4+l/y8zXMWJibSxEHNF3 +QaQ8213mxXhsMdy4zO4w== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedrudefhedgtdegucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvvefufffkjghfggfgtgesthhqredttddtudenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpedvjeegvdevleelkefhlefhueetkeehkedvvefgjeefleeuffdt gedtjefgteefteenucffohhmrghinhepmhhitghrohhsohhfthdrtghomhenucevlhhush htvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhhomhgrshesmhho nhhjrghlohhnrdhnvght X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 31 Jan 2023 17:52:15 -0500 (EST) From: Thomas Monjalon To: Tyler Retzlaff Cc: Morten =?ISO-8859-1?Q?Br=F8rup?= , David Marchand , bruce.richardson@intel.com, Ferruh Yigit , 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, stephen@networkplumber.org Subject: Re: [PATCH v7 4/4] eal: add nonnull and access function attributes Date: Tue, 31 Jan 2023 23:52:14 +0100 Message-ID: <3659208.MHq7AAxBmi@thomas> In-Reply-To: <20230131182613.GB11223@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <20221202153432.131023-1-mb@smartsharesystems.com> <98CBD80474FA8B44BF855DF32C47DC35D876DA@smartserver.smartshare.dk> <20230131182613.GB11223@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" 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 31/01/2023 19:26, Tyler Retzlaff: > On Tue, Jan 31, 2023 at 01:23:34PM +0100, Morten Br=F8rup wrote: > > > From: David Marchand [mailto:david.marchand@redhat.com] > > > Sent: Tuesday, 31 January 2023 12.15 > > >=20 > > > On Wed, Jan 18, 2023 at 9:31 AM Morten Br=F8rup > > > wrote: > > > > > > > > +To: Thomas & David, you probably have some opinions on this too! > > > > > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > > > > Sent: Tuesday, 17 January 2023 22.17 > > > > > > > > > > On Tue, Jan 17, 2023 at 09:19:22AM +0100, Morten Br=F8rup 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=F8rup 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=F8rup > > > > > > > > Acked-by: Tyler Retzlaff > > > > > > > > Reviewed-by: Ruifeng Wang > > > > > > > > --- > > > > > > > > > > > > [...] > > > > > > > > > > > > > > +/** > > > > > > > > + * 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 > > > > > > >=20 > > > > > > > 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. > > > > > > Being a proponent of the world_country_city naming scheme mysel= f, > > > 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 inde= x. > > > > > > > > > > > > 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. > > > > > 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=3Dmsvc-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 > > > > ); > >=20 > > Stephen had bad experiences with SAL, so let's just consider the SAL me= mcpy() example a reference only, showing how the syntax of annotations can = differ very much between build systems. >=20 > 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 discus= sion > about other tools like SAL. Yes of course it must expand empty for compilers not supporting the annotat= ion. 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. > > > > 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. > > >=20 > > > Seeing how clang safety checks helped me catch bugs, that would be a > > > pity. > > >=20 > > > > > > > > 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. > > >=20 > > > Argh, no. > > >=20 > > > > > > > > 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 t= he > > > 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. > > > > > > >=20 > > > I have a bit of trouble understanding the difference between 3a and > > > 3d.. 3a would be about accepting any annotation? > >=20 > > Correct. > >=20 > > >=20 > > > 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. > >=20 > > Agree! I am for 3d as well. We should focus on clang for using compilation checks. Another question about annotations is how much we want to use them. I think we should use such check for critical code only. Adding annotations everywhere is not always worth making the code uglier.