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 CBC3F4237E; Mon, 9 Jan 2023 13:16:56 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C084442B77; Mon, 9 Jan 2023 13:16:56 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id A567A4067C for ; Mon, 9 Jan 2023 13:16:55 +0100 (CET) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v5 1/4] eal: add nonnull and access function attributes Date: Mon, 9 Jan 2023 13:16:54 +0100 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87640@smartserver.smartshare.dk> In-Reply-To: <5925056.DvuYhMxLoT@thomas> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v5 1/4] eal: add nonnull and access function attributes Thread-Index: AdkkGseB/K2/UwZDSs2yBuSOp01ZbwAAMWZQ References: <20221202153432.131023-1-mb@smartsharesystems.com> <20221228151019.101309-1-mb@smartsharesystems.com> <5925056.DvuYhMxLoT@thomas> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Thomas Monjalon" Cc: , , , , , , , , , , , 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 > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Monday, 9 January 2023 12.09 >=20 > 28/12/2022 16:10, Morten Br=F8rup: > > 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 attribute to tell the compiler how a function > > accesses its pointer arguments. >=20 > Why access specification is needed? > Isn't it enough to have the const keyword? No, it the const keyword is not enough. The read_only access attribute = implies a stronger guarantee than the const qualifier which, when cast = away from a pointer, does not prevent the pointed-to object from being = modified [1]. [1]: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html These attributes help finding bugs at build time (which I consider = rather important). Proof: The other patches in the series are bugs = revealed by using this attribute - bugs that were completely undetected = without these attributes. I guess they might also somehow help the optimizer, similar to "const" = and "restrict". I noticed that we define __rte_weak, but not __rte_pure, = so perhaps I should add that too? > I'm afraid we are going to make the code ugly to read with such > attribute. The same can be said about ASAN. Ugly, but helpful. In the extreme, the same could be said about the "const" and "restrict" = keywords... The more decoration on the functions, the uglier the code. = :-) If you prefer, I could split the __rte_access_param(access_mode, ...) up = into one macro per access mode, so this: +__rte_nonnull_params(1, 2) +__rte_access_param(write_only, 1, 3) +__rte_access_param(read_only, 2, 3) static inline void * rte_memcpy_func(void *dst, const void *src, size_t n) would be shortened to this instead: +__rte_params_nonnull(1, 2) +__rte_param_writeonly(1, 3) +__rte_param_readonly(2, 3) static inline void * rte_memcpy_func(void *dst, const void *src, size_t n) or something else. But I don't think it's the improvement you are hoping for. In essence, = it is only a change of the names of the macros. Please note: I prefer keeping the word "params" in the _nonnull macro, = so we are prepared for defining an __rte_nonnull macro (without = parameters) to be used with function parameters like the C22 NonNull = keyword. Keeping "param" in the name(s) of the access macro(s) would = ensure similar future proofing. That, and the similarity with the = __rte_nonnull_params() name was the reason for the access macro name to = include "param".