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 87AA441B8E; Tue, 31 Jan 2023 12:14:55 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 72D5940DFB; Tue, 31 Jan 2023 12:14:55 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id 765AD4067B for ; Tue, 31 Jan 2023 12:14:53 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1675163693; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=hGumPzcf/lQe1Rdj9cJpeGl3KgI5R9GMDcaZBHqbqyE=; b=WdSL0JJb5eOWXUYUhqHTcc5KBo1L1s5orLKJ3ZIIQ3T0llzlCEA9UV1bkHxV8cXZK0cfFJ 4BU0XDlAOyQtM50yEGXNVbAH8kvWu34xxRGnKWrz8d3hIFDZczLq67HRsANKeNRg/jzPoT NgrZY/z2UE2WlOrUP4Ovde3jR2luPqQ= Received: from mail-pj1-f71.google.com (mail-pj1-f71.google.com [209.85.216.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-651-JXqwJ7LeN26fLxT4nFqnWA-1; Tue, 31 Jan 2023 06:14:49 -0500 X-MC-Unique: JXqwJ7LeN26fLxT4nFqnWA-1 Received: by mail-pj1-f71.google.com with SMTP id o11-20020a17090aac0b00b0022c579ce0f0so4053024pjq.1 for ; Tue, 31 Jan 2023 03:14:49 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=hGumPzcf/lQe1Rdj9cJpeGl3KgI5R9GMDcaZBHqbqyE=; b=1taFiUWXTLirhLQK28Inqb01tQCu0l/nbZjaICuYVG3dIbKFD+TCVQhdYH2jV5E1MN x4Q8Okd0dCirJiJ5pLkkSEYLPLpK48xbRm4FHXANU9gfcr/Fsgouwl7ot/+NFSbrbNji oVxkUT5Rd+fl25aTfxaPV/IUty2hAZeqqNf4UZn2W/bLYeXPkEme7ItycYaJrdxgirAK DcOd60+2BGNA8KCv/3gLekgb3TMlSqohnQMeElYg8LDlXrfqHZkh6wr40Mm/aWu+ay+7 3zje048VndHozTtfOP1LtQ5mpVLAK9nDzl0PQBEBatE8Vda4DBHoZzwd9PqRuV7T7FxE tceg== X-Gm-Message-State: AFqh2kofjIA0QvC/Gmn5IlrgFFsrsViSM+8lXby+nHAxdb2bHIvHiCyd wW9NUrHXihOsJpQaCrrlXSBbfNB8ebuSuUcj8odCKYO2d8uA1ieYmk0qI9vAoOzDjRf1F649TTX dgxw+8iPbV3+hYbQ/35g= X-Received: by 2002:a17:90a:6b07:b0:22b:b70f:fa46 with SMTP id v7-20020a17090a6b0700b0022bb70ffa46mr5871047pjj.107.1675163688650; Tue, 31 Jan 2023 03:14:48 -0800 (PST) X-Google-Smtp-Source: AMrXdXv56CC9G8lC2wR+zd0m19505QrWIkDu5/a9yiCocqE1XFJHfJCz3T3ghDgb/QVNKVLMPFadM8POq8lFKTDWEkQ= X-Received: by 2002:a17:90a:6b07:b0:22b:b70f:fa46 with SMTP id v7-20020a17090a6b0700b0022bb70ffa46mr5871038pjj.107.1675163688312; Tue, 31 Jan 2023 03:14:48 -0800 (PST) MIME-Version: 1.0 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> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87683@smartserver.smartshare.dk> From: David Marchand Date: Tue, 31 Jan 2023 12:14:37 +0100 Message-ID: Subject: Re: [PATCH v7 4/4] eal: add nonnull and access function attributes To: =?UTF-8?Q?Morten_Br=C3=B8rup?= Cc: Tyler Retzlaff , thomas@monjalon.net, 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 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com 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, Jan 18, 2023 at 9:31 AM Morten Br=C3=B8rup 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=C3=B8rup 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=C3=B8rup 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=C3=B8rup > > > > > 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=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 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. > > 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: > > 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. 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 head= ers, and use a parser to convert them to compiler specific C/C++ headers wh= en building the code. Argh, no. > > 3a. Keep loading on attributes, with empty macros for unsupported compile= rs. > > 3b. Keep loading on attributes, with empty macros for unsupported compile= rs. But limit ourselves to GCC/Clang style attributes. > > 3c. Keep loading on attributes, with empty macros for unsupported compile= rs. But limit ourselves to Microsoft SAL style attributes. > > 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. > > 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 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. > > 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 compil= ers. 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? 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 David Marchand