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 62C17A09E4; Thu, 28 Jan 2021 18:36:28 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D70C440682; Thu, 28 Jan 2021 18:36:27 +0100 (CET) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mails.dpdk.org (Postfix) with ESMTP id 4B83B4067A; Thu, 28 Jan 2021 18:36:25 +0100 (CET) IronPort-SDR: 8FmLYfX4kUDaJb+8QolwAfWYoGFPYTGGBNF2HOnHgTt+Abvf/RBTOncAmcKu3vkCPrJMswzLju 6B5d4Hzbm8ng== X-IronPort-AV: E=McAfee;i="6000,8403,9878"; a="167375534" X-IronPort-AV: E=Sophos;i="5.79,383,1602572400"; d="scan'208";a="167375534" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Jan 2021 09:36:24 -0800 IronPort-SDR: +j/UDWzgMARcRAU0y0+W+knGmPab343yMVYz8KXICd0LQtLrmUEsoTDJ8T2JtZgRQ9wajw8eaa C20VinRtKxvw== X-IronPort-AV: E=Sophos;i="5.79,383,1602572400"; d="scan'208";a="430609889" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.11.53]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 28 Jan 2021 09:36:22 -0800 Date: Thu, 28 Jan 2021 17:36:19 +0000 From: Bruce Richardson To: Thomas Monjalon Cc: David Marchand , techboard@dpdk.org, dev Message-ID: <20210128173619.GL899@bricha3-MOBL.ger.corp.intel.com> References: <20210114110606.21142-1-bruce.richardson@intel.com> <20210128141610.GI899@bricha3-MOBL.ger.corp.intel.com> <20210128151649.GJ899@bricha3-MOBL.ger.corp.intel.com> <3903937.VWLc1O45Db@thomas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3903937.VWLc1O45Db@thomas> Subject: Re: [dpdk-dev] [dpdk-techboard] [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 05:46:16PM +0100, Thomas Monjalon wrote: > 28/01/2021 16:16, Bruce Richardson: > > On Thu, Jan 28, 2021 at 02:16:10PM +0000, Bruce Richardson wrote: > > > 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. > > > > > Adding Tech-board to see if we can get some more thoughts on this before I do > > another revision of this set. > > What are the alternatives? > The basic problem is what to do when a compiler is used which does not support the required macros to flag an error for use of an internal-only function. For example, we discovered this because clang does not support the #error macro. In those cases, as I see it, we really have two choices: 1 ignore flagging the error and silently allow possible use of the internal function. 2 have the compiler flag an error for an unknown macro The problem that I have with #2 is that without knowing the macro, the compiler will likely error out any time a user app includes any header with an internal function, even if the function is unused. On the other hand, the likelihood of impact if we choose #2 and do error out is quite small, since modern clang versions will support the modern macro checks we need, and just about any gcc versions we care about are going to support #error. For #1, the downside is that we will miss error checks on some older versions of gcc e.g. RHEL 7, and the user may inadvertently use an internal function without knowing it. David, anything else to add here? /Bruce