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 9291842401; Tue, 17 Jan 2023 22:16:59 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 498FE4067E; Tue, 17 Jan 2023 22:16:59 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 44B3440151 for ; Tue, 17 Jan 2023 22:16:57 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 73D5720DFE9D; Tue, 17 Jan 2023 13:16:56 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 73D5720DFE9D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1673990216; bh=/FbYhTCKToB4mUI7yz8+n/w71Zxps2QwXiAyGuJf+eA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=XqnnW3EM3o6cuJI5P1IdKwFock5MbW/T5/JAg8C7RN/r7aX4rjjFJCl1O/OeGYweK qu8XgjBQAT2/2d/2oD7RISCJ7x0miLJPgx6t/+brhRBAC7GfZb7gpXKRfTde7rEHz5 DhYvJ/zjJ1fxcrNfIaE8GgR0lGn5Kai77eA25ck0= Date: Tue, 17 Jan 2023 13:16:56 -0800 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: 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, bruce.richardson@intel.com, konstantin.v.ananyev@yandex.ru Subject: Re: [PATCH v7 4/4] eal: add nonnull and access function attributes Message-ID: <20230117211656.GA30892@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <20221202153432.131023-1-mb@smartsharesystems.com> <20230116130724.50277-1-mb@smartsharesystems.com> <20230116130724.50277-4-mb@smartsharesystems.com> <98CBD80474FA8B44BF855DF32C47DC35D8767B@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D8767B@smartserver.smartshare.dk> User-Agent: Mutt/1.5.21 (2010-09-15) 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 Tue, Jan 17, 2023 at 09:19:22AM +0100, Morten Brørup 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ø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 > > > 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 > > > > No strong opinion, it just feels tidier this way > > 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). > 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.