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 6FA00A09E4; Thu, 28 Jan 2021 12:20:15 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E4C5B4067A; Thu, 28 Jan 2021 12:20:14 +0100 (CET) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mails.dpdk.org (Postfix) with ESMTP id 4CB3840395 for ; Thu, 28 Jan 2021 12:20:13 +0100 (CET) IronPort-SDR: ZdQEgSUVybmiqz5daz3ktQKS6iNZdZhXQmd/bPd5voFj5MRaGTPwO3gGL5IgFZiVJtB8q4grCF 5emCX+F+8NjQ== X-IronPort-AV: E=McAfee;i="6000,8403,9877"; a="167890225" X-IronPort-AV: E=Sophos;i="5.79,382,1602572400"; d="scan'208";a="167890225" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Jan 2021 03:20:12 -0800 IronPort-SDR: 6s8yYKYEZYST66AQ9Pxnv307Q/mlVzLWbHDn3LIpc06wP0O4Uv0+0x+t8hN8HWlKaaeX8A0kQV SLhH2xSKpuIQ== X-IronPort-AV: E=Sophos;i="5.79,382,1602572400"; d="scan'208";a="473508180" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.11.53]) by fmsmga001-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 28 Jan 2021 03:20:10 -0800 Date: Thu, 28 Jan 2021 11:20:07 +0000 From: Bruce Richardson To: David Marchand Cc: dev , "Wang, Haiyue" , Ray Kinsella , Neil Horman Message-ID: <20210128112007.GA899@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 12:00:46PM +0100, David Marchand wrote: > On Wed, Jan 27, 2021 at 6:33 PM Bruce Richardson > wrote: > > > > Clang does not have an "error" attribute for functions, so for marking > > internal functions we need to check for the error attribute, and provide > > a fallback if it is not present. For clang, we can use "diagnose_if" > > attribute, similarly checking for its presence before use. > > > > Fixes: fba5af82adc8 ("eal: add internal ABI tag definition") > > Cc: haiyue.wang@intel.com > > Cc: stable@dpdk.org > > > > > Signed-off-by: Bruce Richardson > > --- > > lib/librte_eal/include/rte_compat.h | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/lib/librte_eal/include/rte_compat.h b/lib/librte_eal/include/rte_compat.h > > index 4cd8f68d68..c30f072aa3 100644 > > --- a/lib/librte_eal/include/rte_compat.h > > +++ b/lib/librte_eal/include/rte_compat.h > > @@ -19,12 +19,18 @@ __attribute__((section(".text.experimental"))) > > > > #endif > > > > -#ifndef ALLOW_INTERNAL_API > > +#if !defined ALLOW_INTERNAL_API && __has_attribute(error) /* For GCC */ > > > > #define __rte_internal \ > > __attribute__((error("Symbol is not public ABI"), \ > > section(".text.internal"))) > > > > +#elif !defined ALLOW_INTERNAL_API && __has_attribute(diagnose_if) /* For clang */ > > + > > +#define __rte_internal \ > > +__attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \ > > +section(".text.internal"))) > > + > > 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. /Bruce