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 282C4A09E4; Thu, 28 Jan 2021 15:16:19 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8FB0A4067A; Thu, 28 Jan 2021 15:16:18 +0100 (CET) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mails.dpdk.org (Postfix) with ESMTP id 6626040395 for ; Thu, 28 Jan 2021 15:16:16 +0100 (CET) IronPort-SDR: 9G5VU+m3ESeFPV6sf6Arp5HtXKttWECw5DCQtMLF/rBAL+JxdfpaG4qlsa201mNv3yhXdXkFeV 58f8Kvp8PrUw== X-IronPort-AV: E=McAfee;i="6000,8403,9877"; a="159411159" X-IronPort-AV: E=Sophos;i="5.79,382,1602572400"; d="scan'208";a="159411159" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Jan 2021 06:16:15 -0800 IronPort-SDR: PO4ChJlX3cweBOCNscCtyyMi8nyce4MhcQIhLm/Fx8jSskxWYlEf4n8Hfa621bt7QyZCyi/76h 2kexN7H98Avw== X-IronPort-AV: E=Sophos;i="5.79,382,1602572400"; d="scan'208";a="354187779" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.11.53]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 28 Jan 2021 06:16:13 -0800 Date: Thu, 28 Jan 2021 14:16:10 +0000 From: Bruce Richardson To: David Marchand Cc: dev , "Wang, Haiyue" , Ray Kinsella , Neil Horman Message-ID: <20210128141610.GI899@bricha3-MOBL.ger.corp.intel.com> References: <20210114110606.21142-1-bruce.richardson@intel.com> <20210127173330.1671341-1-bruce.richardson@intel.com> <20210127173330.1671341-3-bruce.richardson@intel.com> <20210128112007.GA899@bricha3-MOBL.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [dpdk-dev] [PATCH v6 2/8] eal: fix error attribute use for clang 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 Sender: "dev" On Thu, Jan 28, 2021 at 02:36:25PM +0100, David Marchand wrote: > On Thu, Jan 28, 2021 at 12:20 PM Bruce Richardson > wrote: > > > If the compiler has neither error or diagnose_if support, an internal > > > API can be called without ALLOW_INTERNAL_API. > > > I prefer a build error complaining on an unknown attribute rather than > > > silence a check. > > > > > > I.e. > > > > > > #ifndef ALLOW_INTERNAL_API > > > > > > #if __has_attribute(diagnose_if) /* For clang */ > > > #define __rte_internal \ > > > __attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \ > > > section(".text.internal"))) > > > > > > #else > > > #define __rte_internal \ > > > __attribute__((error("Symbol is not public ABI"), \ > > > section(".text.internal"))) > > > > > > #endif > > > > > > #else /* ALLOW_INTERNAL_API */ > > > > > > #define __rte_internal \ > > > section(".text.internal"))) > > > > > > #endif > > > > > > > Would this not mean that if someone was using a compiler that supported > > neither that they could never include any header file which contained any > > internal functions? I'd rather err on the side of allowing this, on the > > basis that the symbol should be already documented as internal and this is > > only an additional sanity check. > > - Still not a fan. > We will never know about those compilers behavior, like how we caught > the clang issue and found an alternative. > So I understand, but I'm still concerned about breaking something that was previously working. It's one thing a DPDK developer catching issues with clang, quite another a user catching it when trying to build their own application. We probably need some other opinions on this one. > > - I just caught a build error with RHEL7 gcc: > > [1/2127] Compiling C object > lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o > FAILED: lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o > ccache cc -Ilib/librte_eal.a.p -Ilib -I../lib -I. -I.. -Iconfig > -I../config -Ilib/librte_eal/include -I../lib/librte_eal/include > -Ilib/librte_eal/linux/include -I../lib/librte_eal/linux/include > -Ilib/librte_eal/x86/include -I../lib/librte_eal/x86/include > -Ilib/librte_eal/common -I../lib/librte_eal/common -Ilib/librte_eal > -I../lib/librte_eal -Ilib/librte_kvargs -I../lib/librte_kvargs > -Ilib/librte_metrics -I../lib/librte_metrics -Ilib/librte_telemetry > -I../lib/librte_telemetry -pipe -D_FILE_OFFSET_BITS=64 -Wall > -Winvalid-pch -O3 -include rte_config.h -Wextra -Wcast-qual > -Wdeprecated -Wformat -Wformat-nonliteral -Wformat-security > -Wmissing-declarations -Wmissing-prototypes -Wnested-externs > -Wold-style-definition -Wpointer-arith -Wsign-compare > -Wstrict-prototypes -Wundef -Wwrite-strings > -Wno-missing-field-initializers -D_GNU_SOURCE -fPIC -march=native > -DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API '-DABI_VERSION="21.1"' > -MD -MQ lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o -MF > lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o.d -o > lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o -c > ../lib/librte_eal/common/eal_common_config.c > In file included from ../lib/librte_eal/include/rte_dev.h:24:0, > from ../lib/librte_eal/common/eal_private.h:12, > from ../lib/librte_eal/common/eal_common_config.c:9: > ../lib/librte_eal/include/rte_compat.h:22:51: error: missing binary > operator before token "(" > #if !defined ALLOW_INTERNAL_API && __has_attribute(error) /* For GCC */ > ^ > ../lib/librte_eal/include/rte_compat.h:28:53: error: missing binary > operator before token "(" > #elif !defined ALLOW_INTERNAL_API && __has_attribute(diagnose_if) /* > For clang */ > ^ > > I can see that gcc doc recommends checking for __has_attribute availability. > Pasting from google cache, since the gcc.gnu.org doc website seems unavailable. > > """ > 4.2.6 __has_attribute > > The special operator __has_attribute (operand) may be used in ‘#if’ > and ‘#elif’ expressions to test whether the attribute referenced by > its operand is recognized by GCC. Using the operator in other contexts > is not valid. In C code, if compiling for strict conformance to > standards before C2x, operand must be a valid identifier. Otherwise, > operand may be optionally introduced by the attribute-scope:: prefix. > The attribute-scope prefix identifies the “namespace” within which the > attribute is recognized. The scope of GCC attributes is ‘gnu’ or > ‘__gnu__’. The __has_attribute operator by itself, without any operand > or parentheses, acts as a predefined macro so that support for it can > be tested in portable code. Thus, the recommended use of the operator > is as follows: > > #if defined __has_attribute > # if __has_attribute (nonnull) > # define ATTR_NONNULL __attribute__ ((nonnull)) > # endif > #endif > > The first ‘#if’ test succeeds only when the operator is supported by > the version of GCC (or another compiler) being used. Only when that > test succeeds is it valid to use __has_attribute as a preprocessor > operator. As a result, combining the two tests into a single > expression as shown below would only be valid with a compiler that > supports the operator but not with others that don’t. > > #if defined __has_attribute && __has_attribute (nonnull) /* not portable */ > … > #endif > """ > I really wish other tools would do like meson and document per-feature the version in which it was introduced! Anyway, this is something I'll fix in next version, though again we need to decide in the case of __has_attribute not being supported do we fall to erroring out? Again that runs the risk of users not being able to include a header which has an internal function, so I'd prefer us to ignore errors if the appropriate macros are unsupported. Again, other opinions probably needed. /Bruce