diff mbox

[v3,2/2] avx2 configure: Use primitives in test

Message ID 1465557378-24105-3-git-send-email-dgilbert@redhat.com
State New
Headers show

Commit Message

Dr. David Alan Gilbert June 10, 2016, 11:16 a.m. UTC 
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Use the avx2 primitives during the test, thus making sure that the
compiler and assembler could actually use avx2.

This also detects the failure case on gcc 4.8.x with -save-temps
and avoids the need for the gcc version check in cutils.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 configure     | 17 ++++++++++++-----
 util/cutils.c |  8 +-------
 2 files changed, 13 insertions(+), 12 deletions(-)

Comments

Aaron Lindsay July 14, 2016, 1:13 p.m. UTC | #1 
On Jun 10 12:16, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Use the avx2 primitives during the test, thus making sure that the
> compiler and assembler could actually use avx2.
> 
> This also detects the failure case on gcc 4.8.x with -save-temps
> and avoids the need for the gcc version check in cutils.

I'm getting a segfault when running the latest tip compiled with gcc
4.8.4 on Ubuntu 14.04 and I've bisected it to this commit.

# gcc --version
gcc (Ubuntu 4.8.4-2ubuntu1~14.04) 4.8.4

I'm configuring with:
# ./configure \
    --static \
	--disable-gtk \
	--target-list=aarch64-softmmu

When run under gdb, I get:
Program received signal SIGSEGV, Segmentation fault.
buffer_find_nonzero_offset_ifunc () at ./util/cutils.c:333
333     {
(gdb) bt
#0  buffer_find_nonzero_offset_ifunc () at ./util/cutils.c:333
#1  0x0000000000939c58 in __libc_start_main ()
#2  0x0000000000419337 in _start ()

I confess I don't understand the intricacies here, but I'm willing to
test fixes if you have any ideas for how to make this also work for my
compiler without blindly excluding all gcc < 4.9.

-Aaron
Paolo Bonzini July 14, 2016, 1:15 p.m. UTC | #2 
On 14/07/2016 15:13, Aaron Lindsay wrote:
> On Jun 10 12:16, Dr. David Alan Gilbert (git) wrote:
>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>
>> Use the avx2 primitives during the test, thus making sure that the
>> compiler and assembler could actually use avx2.
>>
>> This also detects the failure case on gcc 4.8.x with -save-temps
>> and avoids the need for the gcc version check in cutils.
> 
> I'm getting a segfault when running the latest tip compiled with gcc
> 4.8.4 on Ubuntu 14.04 and I've bisected it to this commit.
> 
> # gcc --version
> gcc (Ubuntu 4.8.4-2ubuntu1~14.04) 4.8.4
> 
> I'm configuring with:
> # ./configure \
>     --static \
> 	--disable-gtk \
> 	--target-list=aarch64-softmmu
> 
> When run under gdb, I get:
> Program received signal SIGSEGV, Segmentation fault.
> buffer_find_nonzero_offset_ifunc () at ./util/cutils.c:333
> 333     {
> (gdb) bt
> #0  buffer_find_nonzero_offset_ifunc () at ./util/cutils.c:333
> #1  0x0000000000939c58 in __libc_start_main ()
> #2  0x0000000000419337 in _start ()
> 
> I confess I don't understand the intricacies here, but I'm willing to
> test fixes if you have any ideas for how to make this also work for my
> compiler without blindly excluding all gcc < 4.9.

Hmm, it's possible that we have to disable ifunc together with --static.

Paolo
Peter Maydell July 14, 2016, 1:23 p.m. UTC | #3 
On 14 July 2016 at 14:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 14/07/2016 15:13, Aaron Lindsay wrote:
>> I'm configuring with:
>> # ./configure \
>>     --static \
>>       --disable-gtk \
>>       --target-list=aarch64-softmmu

> Hmm, it's possible that we have to disable ifunc together with --static.

I'm still tempted to say we should just forbid building the softmmu
binaries with --static, unless somebody has a serious use case for it...

thanks
-- PMM
Dr. David Alan Gilbert July 14, 2016, 1:33 p.m. UTC | #4 
* Aaron Lindsay (alindsay@codeaurora.org) wrote:
> On Jun 10 12:16, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Use the avx2 primitives during the test, thus making sure that the
> > compiler and assembler could actually use avx2.
> > 
> > This also detects the failure case on gcc 4.8.x with -save-temps
> > and avoids the need for the gcc version check in cutils.
> 
> I'm getting a segfault when running the latest tip compiled with gcc
> 4.8.4 on Ubuntu 14.04 and I've bisected it to this commit.
> 
> # gcc --version
> gcc (Ubuntu 4.8.4-2ubuntu1~14.04) 4.8.4
> 
> I'm configuring with:
> # ./configure \
>     --static \
> 	--disable-gtk \
> 	--target-list=aarch64-softmmu
> 
> When run under gdb, I get:
> Program received signal SIGSEGV, Segmentation fault.
> buffer_find_nonzero_offset_ifunc () at ./util/cutils.c:333
> 333     {
> (gdb) bt
> #0  buffer_find_nonzero_offset_ifunc () at ./util/cutils.c:333
> #1  0x0000000000939c58 in __libc_start_main ()
> #2  0x0000000000419337 in _start ()
> 
> I confess I don't understand the intricacies here, but I'm willing to
> test fixes if you have any ideas for how to make this also work for my
> compiler without blindly excluding all gcc < 4.9.

Does it work if you configure without the --static?

Dave

> 
> -Aaron
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Aaron Lindsay July 14, 2016, 2:18 p.m. UTC | #5 
On Jul 14 14:33, Dr. David Alan Gilbert wrote:
> * Aaron Lindsay (alindsay@codeaurora.org) wrote:
> > I'm configuring with:
> > # ./configure \
> >     --static \
> > 	--disable-gtk \
> > 	--target-list=aarch64-softmmu
> 
> Does it work if you configure without the --static?

Yes, it works if I configure without --static.

-Aaron
Aaron Lindsay July 14, 2016, 2:27 p.m. UTC | #6 
On Jul 14 14:23, Peter Maydell wrote:
> On 14 July 2016 at 14:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 14/07/2016 15:13, Aaron Lindsay wrote:
> >> I'm configuring with:
> >> # ./configure \
> >>     --static \
> >>       --disable-gtk \
> >>       --target-list=aarch64-softmmu
> 
> > Hmm, it's possible that we have to disable ifunc together with --static.
> 
> I'm still tempted to say we should just forbid building the softmmu
> binaries with --static, unless somebody has a serious use case for it...

FWIW, we do find it convenient to be able to compile one binary capable
of running on multiple systems with different (and incompatible) library
versions.

-Aaron
Peter Maydell July 14, 2016, 2:35 p.m. UTC | #7 
On 14 July 2016 at 15:27, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> On Jul 14 14:23, Peter Maydell wrote:
>> On 14 July 2016 at 14:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > On 14/07/2016 15:13, Aaron Lindsay wrote:
>> >> I'm configuring with:
>> >> # ./configure \
>> >>     --static \
>> >>       --disable-gtk \
>> >>       --target-list=aarch64-softmmu
>>
>> > Hmm, it's possible that we have to disable ifunc together with --static.
>>
>> I'm still tempted to say we should just forbid building the softmmu
>> binaries with --static, unless somebody has a serious use case for it...
>
> FWIW, we do find it convenient to be able to compile one binary capable
> of running on multiple systems with different (and incompatible) library
> versions.

The difficulty here is that the system emulators may call
functions in glibc which do dynamic resolution anyway, and so
"require the shared libraries from the glibc version used for
linking", to quote the linker warning. For the user-only binaries
we can claim we don't make those function calls, but for softmmu
we do.

thanks
-- PMM
Dr. David Alan Gilbert July 14, 2016, 3:05 p.m. UTC | #8 
* Aaron Lindsay (alindsay@codeaurora.org) wrote:
> On Jul 14 14:33, Dr. David Alan Gilbert wrote:
> > * Aaron Lindsay (alindsay@codeaurora.org) wrote:
> > > I'm configuring with:
> > > # ./configure \
> > >     --static \
> > > 	--disable-gtk \
> > > 	--target-list=aarch64-softmmu
> > 
> > Does it work if you configure without the --static?
> 
> Yes, it works if I configure without --static.

Hmm; I wonder what the best fix is here; we could just disable
it with static; it seems an easy fix assuming very few people
care about the combination of avx2 performance and static,
or we could try and detect the problem/old version.
(it seems fine on a modern Fedora; have you tried using a modern
Ubuntu?)

Dave

> 
> -Aaron
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Aaron Lindsay July 14, 2016, 7:36 p.m. UTC | #9 
On Jul 14 16:05, Dr. David Alan Gilbert wrote:
> * Aaron Lindsay (alindsay@codeaurora.org) wrote:
> > On Jul 14 14:33, Dr. David Alan Gilbert wrote:
> > > * Aaron Lindsay (alindsay@codeaurora.org) wrote:
> > > > I'm configuring with:
> > > > # ./configure \
> > > >     --static \
> > > > 	--disable-gtk \
> > > > 	--target-list=aarch64-softmmu
> > > 
> > > Does it work if you configure without the --static?
> > 
> > Yes, it works if I configure without --static.
> 
> Hmm; I wonder what the best fix is here; we could just disable
> it with static; it seems an easy fix assuming very few people
> care about the combination of avx2 performance and static,
> or we could try and detect the problem/old version.
> (it seems fine on a modern Fedora; have you tried using a modern
> Ubuntu?)

That seems like a reasonable fix to me. Assuming your intention was to
do this in 'configure', I'll send out a patch shortly.

Unfortunately I haven't been able to try on a 'modern' Ubuntu yet.

-Aaron

--
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
Aaron Lindsay July 14, 2016, 7:44 p.m. UTC | #10 
On Jul 14 15:35, Peter Maydell wrote:
> On 14 July 2016 at 15:27, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > On Jul 14 14:23, Peter Maydell wrote:
> >> On 14 July 2016 at 14:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> > On 14/07/2016 15:13, Aaron Lindsay wrote:
> >> >> I'm configuring with:
> >> >> # ./configure \
> >> >>     --static \
> >> >>       --disable-gtk \
> >> >>       --target-list=aarch64-softmmu
> >>
> >> > Hmm, it's possible that we have to disable ifunc together with --static.
> >>
> >> I'm still tempted to say we should just forbid building the softmmu
> >> binaries with --static, unless somebody has a serious use case for it...
> >
> > FWIW, we do find it convenient to be able to compile one binary capable
> > of running on multiple systems with different (and incompatible) library
> > versions.
> 
> The difficulty here is that the system emulators may call
> functions in glibc which do dynamic resolution anyway, and so
> "require the shared libraries from the glibc version used for
> linking", to quote the linker warning. For the user-only binaries
> we can claim we don't make those function calls, but for softmmu
> we do.

We haven't observed any problems with this approach so far, but your
point is well taken. Perhaps we get what we deserve if we insist on
statically compiling ;-)

-Aaron
diff mbox

Patch

diff --git a/configure b/configure
index ffa5b62..3204785 100755
--- a/configure
+++ b/configure
@@ -1782,13 +1782,20 @@  fi
 # avx2 optimization requirement check
 
 cat > $TMPC << EOF
-static void bar(void) {}
+#pragma GCC push_options
+#pragma GCC target("avx2")
+#include <cpuid.h>
+#include <immintrin.h>
+
+static int bar(void *a) {
+    return _mm256_movemask_epi8(_mm256_cmpeq_epi8(*(__m256i *)a, (__m256i){0}));
+}
 static void *bar_ifunc(void) {return (void*) bar;}
-void foo(void) __attribute__((ifunc("bar_ifunc")));
-int main(void) { foo(); return 0; }
+int foo(void *a) __attribute__((ifunc("bar_ifunc")));
+int main(int argc, char *argv[]) { return foo(argv[0]);}
 EOF
-if compile_prog "-mavx2" "" ; then
-    if readelf --syms $TMPE |grep "IFUNC.*foo" >/dev/null 2>&1; then
+if compile_object "" ; then
+    if readelf --syms $TMPO |grep "IFUNC.*foo" >/dev/null 2>&1; then
         avx2_opt="yes"
     fi
 fi
diff --git a/util/cutils.c b/util/cutils.c
index 43d1afb..5830a68 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -256,13 +256,7 @@  static size_t buffer_find_nonzero_offset_inner(const void *buf, size_t len)
     return i * sizeof(VECTYPE);
 }
 
-/*
- * GCC before version 4.9 has a bug which will cause the target
- * attribute work incorrectly and failed to compile in some case,
- * restrict the gcc version to 4.9+ to prevent the failure.
- */
-
-#if defined CONFIG_AVX2_OPT && QEMU_GNUC_PREREQ(4, 9)
+#if defined CONFIG_AVX2_OPT
 #pragma GCC push_options
 #pragma GCC target("avx2")
 #include <cpuid.h>