From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 037E9A04A5; Wed, 17 Jun 2020 16:33:22 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5045E29C6; Wed, 17 Jun 2020 16:33:21 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 1125D25B3 for ; Wed, 17 Jun 2020 16:33:19 +0200 (CEST) IronPort-SDR: W+4BfYiFq1hhIe4PjcELKF0ME+/BDmzDwsC74o8anH4RIKFlfDaQcp+mOx13Js1/qdTzmXTSGo epGmboLWnhSQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Jun 2020 07:32:51 -0700 IronPort-SDR: LwJ0xHHcIJUK+ohETH4IJCfe7KW5dRCLI/dR1Lm6zISu1/4x2neXmumypG4rlkV12bCS3mGlyg TrfXBOjPi25g== X-IronPort-AV: E=Sophos;i="5.73,522,1583222400"; d="scan'208";a="421155571" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.27.7]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 17 Jun 2020 07:32:49 -0700 Date: Wed, 17 Jun 2020 15:32:46 +0100 From: Bruce Richardson To: Thomas Monjalon Cc: dev@dpdk.org, ferruh.yigit@intel.com Message-ID: <20200617143246.GA85@bricha3-MOBL.ger.corp.intel.com> References: <20200617104012.470617-1-bruce.richardson@intel.com> <159240636.HSrAOgkoKl@thomas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <159240636.HSrAOgkoKl@thomas> Subject: Re: [dpdk-dev] [PATCH] build: check functionality rather than binutils version X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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 Wed, Jun 17, 2020 at 01:45:57PM +0200, Thomas Monjalon wrote: > 17/06/2020 12:40, Bruce Richardson: > > Rather than checking the binutils version number, which can lead to > > unnecessary disabling of AVX512 if fixes have been backported to distro > > versions, we can instead check the output of "as" from binutils to see if > > it is correct. > > > > These checks use the minimal assembly reproduction code posted to the > > public bug tracker for gcc/binutils for those issues [1][2]. If the > > binutils bug is present, the instruction parameters - specifically the > > displacement parameter - will be different in the disassembled output > > compared to the input. Therefore each check involves assembling a single > > instruction and disassembling it again, checking that the two match. > > Whaoh, that's a very specific optimization :) > Which distro is affected (backported binutils fix)? > Not an optimization, it's a bug, which necessitates disabling a whole instruction set, so we should really check for the bug rather than assuming all versions are affected. From [2] below, we have confirmation that the fix has been backported to both 2.30 and 2.31 binutils branches. > > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90028 > > [2] https://sourceware.org/bugzilla/show_bug.cgi?id=23465 > > > > Signed-off-by: Bruce Richardson > > --- > > --- a/buildtools/meson.build > > +++ b/buildtools/meson.build > > -subdir('pmdinfogen') > > Why removing pmdinfogen? The pmdinfogen requires values that are set in the config folder, but the config folder processing now uses scripts that are in the buildtools folder. While we could refer to them using relative paths, I felt it better to separate out the compilation of pmdinfogen to the rest of the buildutils processing, which is simply assigning variables to scripts to make them easier to call later in the processing. > > > --- a/meson.build > > +++ b/meson.build > > +# do configuration and get tool paths > > +subdir('buildtools') > > subdir('config') > > > > # build libs and drivers > > -subdir('buildtools') > > +subdir('buildtools/pmdinfogen') > > Is it related to binutils check? > Pretty much. It just makes it easier to have the config processing use scripts in the buildtools folder. > [...] > > - ldver = run_command('ld', '-v').stdout().strip() > > - if ldver.contains('2.30') and cc.has_argument('-mno-avx512f') > > + as_ok = run_command(binutils_avx512_check) > > + if as_ok.returncode() != 0 and cc.has_argument('-mno-avx512f') > > "as_ok" is difficult to understand. > I would suggest "binutils_avx512_ok". > Sure, can change for v2 - though I'd suggest the slightly shorter binutils_ok. > > + message('Binutils error with AVX512 assembly, disabling AVX512 support') > > It looks like something which should be part of meson itself. > That would certainly be nice to have, but I consider it infeasible, since there will be, even in the best case, a lag between a bug being discovered and workarounds making its way into tools. Even then you still have an update problem to work around it - update binutils and bug goes away, or update meson and you can work around it. In both cases you are requiring the user to update something, so fixing in DPDK is best. /Bruce