build: check functionality rather than binutils version
Checks
Commit Message
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.
[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 <bruce.richardson@intel.com>
---
buildtools/binutils-avx512-check.sh | 26 ++++++++++++++++++++++++++
buildtools/meson.build | 3 +--
config/x86/meson.build | 10 +++-------
meson.build | 5 ++++-
4 files changed, 34 insertions(+), 10 deletions(-)
create mode 100755 buildtools/binutils-avx512-check.sh
Comments
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)?
> [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 <bruce.richardson@intel.com>
> ---
> --- a/buildtools/meson.build
> +++ b/buildtools/meson.build
> -subdir('pmdinfogen')
Why removing pmdinfogen?
> --- 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?
[...]
> - 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".
> + message('Binutils error with AVX512 assembly, disabling AVX512 support')
It looks like something which should be part of meson itself.
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 <bruce.richardson@intel.com>
> > ---
> > --- 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
new file mode 100755
@@ -0,0 +1,26 @@
+#! /bin/sh
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2020 Intel Corporation
+
+AS=${AS:-as}
+MESON_BUILD_ROOT=${MESON_BUILD_ROOT:-/tmp}
+OBJFILE=$MESON_BUILD_ROOT/binutils-avx512-check.o
+# from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90028
+GATHER_PARAMS='0x8(,%ymm1,1),%ymm0{%k2}'
+# from https://sourceware.org/bugzilla/show_bug.cgi?id=23465
+MOVAPS_PARAMS='0x40(,%rax,1),%zmm0'
+
+# assemble vmovaps statement to file
+echo "vmovaps $MOVAPS_PARAMS" | $AS --64 -o $OBJFILE -
+# check the parameters are correct in the output, using objdump
+objdump -d --no-show-raw-insn $OBJFILE | grep -q $MOVAPS_PARAMS || {
+ echo "vmovaps displacement error with as"
+ exit 1
+}
+
+# assemble vpgather to file and similarly check
+echo "vpgatherqq $GATHER_PARAMS" | $AS --64 -o $OBJFILE -
+objdump -d --no-show-raw-insn $OBJFILE | grep -q $GATHER_PARAMS || {
+ echo "vpgatherqq displacement error with as"
+ exit 1
+}
@@ -1,13 +1,12 @@
# SPDX-License-Identifier: BSD-3-Clause
# Copyright(c) 2017-2019 Intel Corporation
-subdir('pmdinfogen')
-
pkgconf = find_program('pkg-config', 'pkgconf', required: false)
pmdinfo = find_program('gen-pmdinfo-cfile.sh')
list_dir_globs = find_program('list-dir-globs.py')
check_symbols = find_program('check-symbols.sh')
ldflags_ibverbs_static = find_program('options-ibverbs-static.sh')
+binutils_avx512_check = find_program('binutils-avx512-check.sh')
# set up map-to-def script using python, either built-in or external
python3 = import('python').find_installation(required: false)
@@ -3,14 +3,10 @@
# get binutils version for the workaround of Bug 97
if not is_windows
- 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')
machine_args += '-mno-avx512f'
- message('Binutils 2.30 detected, disabling AVX512 support as workaround for bug #97')
- endif
- if ldver.contains('2.31') and cc.has_argument('-mno-avx512f')
- machine_args += '-mno-avx512f'
- message('Binutils 2.31 detected, disabling AVX512 support as workaround for bug #249')
+ message('Binutils error with AVX512 assembly, disabling AVX512 support')
endif
endif
@@ -41,10 +41,13 @@ global_inc = include_directories('.', 'config',
'lib/librte_eal/@0@/include'.format(host_machine.system()),
'lib/librte_eal/@0@/include'.format(arch_subdir),
)
+
+# do configuration and get tool paths
+subdir('buildtools')
subdir('config')
# build libs and drivers
-subdir('buildtools')
+subdir('buildtools/pmdinfogen')
subdir('lib')
subdir('drivers')