qemu-efi: hangs in kvm mode when built w/ gcc-5

[dann frazier]

When I rebuild edk2_0~20150106.5c2d456b-1build1 in wily or sid, KVM-accelerated arm64 VMs hang before displaying any console output. I bisected this down to a change in gcc (see below).

Note that this does not impact fully emulated instances. That is, if I drop '-enable-kvm' and change '-cpu host' to '-cpu cortex-a57', it boots fine.

Here's the gcc changeset that introduced the issue:

From 470d5bb5a2aba04db8d9d9dce0c5a3d5efe85882 Mon Sep 17 00:00:00 2001
From: thopre01 <thopre01@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Fri, 23 May 2014 03:33:28 +0000
Subject: [PATCH] 2014-05-23 Thomas Preud'homme <email address hidden>

        PR tree-optimization/54733
gcc/
        * tree-ssa-math-opts.c (nop_stats): New "bswap_stats" structure.
        (CMPNOP): Define.
        (find_bswap_or_nop_load): New.
        (find_bswap_1): Renamed to ...
        (find_bswap_or_nop_1): This. Also add support for memory source.
        (find_bswap): Renamed to ...
        (find_bswap_or_nop): This. Also add support for memory source and
        detection of bitwise operations equivalent to load in host endianness.
        (execute_optimize_bswap): Likewise. Also move its leading comment back
        in place and split statement transformation into ...
        (bswap_replace): This.

gcc/testsuite
        * gcc.dg/optimize-bswapdi-3.c: New test to check extension of bswap
        optimization to support memory sources and bitwise operations
        equivalent to load in host endianness.
        * gcc.dg/optimize-bswaphi-1.c: Likewise.
        * gcc.dg/optimize-bswapsi-2.c: Likewise.
        * gcc.c-torture/execute/bswap-2.c: Likewise.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@210843 138bc75d-0d04-0410-961f-82ee72b054a4

[Steve Langasek (vorlon)]

if this is a code generation bug, then it should be a high priority to fix in gcc.

[Steve Langasek (vorlon)]

Matthias, please have a look at this when you can

[Matthias Klose (doko)]

CCed Thomas

does this go away when building edk2 with lowered optimization? without optimization?
is building using GCC 4.9 a good enough work around?

please could you describe how to reproduce this issue?

[Matthias Klose (doko)]

looks like a build is a mix of no opt, -O and -Os. So maybe experiment with -O2 instead of -Os?
also we are shipping a snapshot which predates the GCC 5 release. Please could you check with a recent edk2 snapshot as well?

[Steve Langasek (vorlon)]

On Sun, Aug 30, 2015 at 02:04:17PM -0000, Matthias Klose wrote:
> is building using GCC 4.9 a good enough work around?

This bug was discovered in the process of trying to transition the qemu-efi
binary package to be Architecture: all and built using a cross-compiler,
which in Debian we only have for gcc-5 and above. So that's not a very good
workaround in this case.

[Thomas Preud'homme (thomas-preudhomme)]

Ok, I'll build edk2 with bswap dumps enabled and check all the cases this pass optimizes. There might be many of them so if you manage to pinpoint it to some specific file or testcase please let me know as it would make debugging easier.

Best regards

[dann frazier (dannf)]

On Sun, Aug 30, 2015 at 8:17 AM, Matthias Klose <email address hidden> wrote:
> looks like a build is a mix of no opt, -O and -Os. So maybe experiment with -O2 instead of -Os?
> also we are shipping a snapshot which predates the GCC 5 release. Please could you check with a recent edk2 snapshot as well?

I tested a build of edk2 trunk last week and the issue persists.

 -dann

[dann frazier (dannf)]

Here's the script I used to reproduce it. Note that it is using KVM, so it needs to run natively on an arm64 system.
The trusty image I used is just a download from cloud-images.ubuntu.com. But you could just use an empty file for that since the hang occurs way before the disk would be accessed.

[Thomas Preud'homme (thomas-preudhomme)]

The commit mentionned in the description concern the bswap pass which is enabled at O2 and Os. No need to resort to -O1 though, -fno-expensive-optimizations should be enough to disable it.

[Thomas Preud'homme (thomas-preudhomme)]

grep -E '[cC]{2}[ "].*' ../edk2.log | grep -Ev " (-E|(-x assembler(-with-cpp)?)) " | grep -v -- "-fdump-tree-bswap" shows that all invocation of cc or gcc for compiling had a -fdump-tree-bswap in it. This confuses me because the only "found at" I could find in the bswap dumps are for:

./Build/Shell/RELEASE_ARMGCC/AARCH64/MdePkg/Library/BaseLib/BaseLib/OUTPUT/SwapBytes16.c.090t.bswap:16 bit bswap implementation found at: _7 = _4 | _6;
./Build/ArmVirtualizationQemu-AARCH64/RELEASE_ARMGCC/AARCH64/MdePkg/Library/BaseLib/BaseLib/OUTPUT/SwapBytes16.c.090t.bswap:16 bit bswap implementation found at: _7 = _4 | _6;
./Build/Fat/RELEASE_ARMGCC/AARCH64/MdePkg/Library/BaseLib/BaseLib/OUTPUT/SwapBytes16.c.090t.bswap:16 bit bswap implementation found at: _7 = _4 | _6;

All these conversion are fine and should have triggered with GCC 4.9 as well.

Can you add -fdump-tree-bswap to CFLAGS in:
BaseTools/Conf/tools_def.template
BaseTools/Source/C/VfrCompile/Pccts/antlr/makefile
BaseTools/Source/C/VfrCompile/Pccts/antlr/makefile.cygwin
BaseTools/Source/C/VfrCompile/Pccts/dlg/makefile
BaseTools/Source/C/VfrCompile/Pccts/dlg/makefile.cygwin
BaseTools/Source/C/Makefiles/header.makefile

and tell me the result of:
find -name \*.bswap -exec grep -H "found at" {} \;
?

[dann frazier (dannf)]

On Tue, Sep 1, 2015 at 1:56 AM, Thomas Preud'homme
<email address hidden> wrote:
> grep -E '[cC]{2}[ "].*' ../edk2.log | grep -Ev " (-E|(-x assembler
> (-with-cpp)?)) " | grep -v -- "-fdump-tree-bswap" shows that all
> invocation of cc or gcc for compiling had a -fdump-tree-bswap in it.
> This confuses me because the only "found at" I could find in the bswap
> dumps are for:
>
> ./Build/Shell/RELEASE_ARMGCC/AARCH64/MdePkg/Library/BaseLib/BaseLib/OUTPUT/SwapBytes16.c.090t.bswap:16 bit bswap implementation found at: _7 = _4 | _6;
> ./Build/ArmVirtualizationQemu-AARCH64/RELEASE_ARMGCC/AARCH64/MdePkg/Library/BaseLib/BaseLib/OUTPUT/SwapBytes16.c.090t.bswap:16 bit bswap implementation found at: _7 = _4 | _6;
> ./Build/Fat/RELEASE_ARMGCC/AARCH64/MdePkg/Library/BaseLib/BaseLib/OUTPUT/SwapBytes16.c.090t.bswap:16 bit bswap implementation found at: _7 = _4 | _6;
>
> All these conversion are fine and should have triggered with GCC 4.9 as
> well.
>
> Can you add -fdump-tree-bswap to CFLAGS in:
> BaseTools/Conf/tools_def.template
> BaseTools/Source/C/VfrCompile/Pccts/antlr/makefile
> BaseTools/Source/C/VfrCompile/Pccts/antlr/makefile.cygwin
> BaseTools/Source/C/VfrCompile/Pccts/dlg/makefile
> BaseTools/Source/C/VfrCompile/Pccts/dlg/makefile.cygwin
> BaseTools/Source/C/Makefiles/header.makefile
>
> and tell me the result of:
> find -name \*.bswap -exec grep -H "found at" {} \;
> ?

dannf@mustang:~/edk2-0~20150106.5c2d456b$ gcc --version
gcc (Ubuntu 5.2.1-15ubuntu2) 4.10.0 20140523 (experimental)
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

dannf@mustang:~/edk2-0~20150106.5c2d456b$ find -name \*.bswap -exec
grep -H "found at" {} \;
./Build/ArmVirtualizationQemu-AARCH64/RELEASE_GCC49/AARCH64/MdePkg/Library/BaseLib/BaseLib/OUTPUT/SwapBytes16.c.091t.bswap:16
bit bswap implementation found at: _7 = _4 | _6;
./Build/Shell/RELEASE_GCC49/AARCH64/MdePkg/Library/BaseLib/BaseLib/OUTPUT/SwapBytes16.c.091t.bswap:16
bit bswap implementation found at: _7 = _4 | _6;
./Build/Fat/RELEASE_GCC49/AARCH64/MdePkg/Library/BaseLib/BaseLib/OUTPUT/SwapBytes16.c.091t.bswap:16
bit bswap implementation found at: _7 = _4 | _6;

[dann frazier (dannf)]

On Mon, Aug 31, 2015 at 8:24 PM, Thomas Preud'homme
<email address hidden> wrote:
> The commit mentionned in the description concern the bswap pass which is
> enabled at O2 and Os. No need to resort to -O1 though, -fno-expensive-
> optimizations should be enough to disable it.

I tested that and can confirm it is sufficient (see attached patch).

[Steve Langasek (vorlon)]

On Wed, Sep 02, 2015 at 04:44:35PM -0000, dann frazier wrote:

> I tested that and can confirm it is sufficient (see attached patch).

This adjusts the build flags for all platforms using gcc, for gcc 4.4 and
above, which is overbroad.

Could you test adding this as a patch to the ARMGCC build flags instead?

[dann frazier (dannf)]

On Wed, Sep 2, 2015 at 5:57 PM, Steve Langasek
<email address hidden> wrote:
> On Wed, Sep 02, 2015 at 04:44:35PM -0000, dann frazier wrote:
>
>> I tested that and can confirm it is sufficient (see attached patch).
>
> This adjusts the build flags for all platforms using gcc, for gcc 4.4 and
> above, which is overbroad.
>
> Could you test adding this as a patch to the ARMGCC build flags instead?

Yep, here you go.

[Thomas Preud'homme (thomas-preudhomme)]

I built edk2 with and without the arm64-no-expensive-optimizations patch, dumping the trees in both case (with -fdump-tree-bswap with the patch and -fdump-tree-sincos without). Fortunately, the diff is quite small and I could review it all. Transformations all look sound and indeed there is a few transformations that wasn't done before the patch mentionned in the initial bug report.

One thing that it does and could pose problem is transforming a serie of byte load into a single unaligned 32bit load. This is allowed for this target by default so nothing wrong here but maybe in this case unaligned load should not be done. If that is the case, -mstrict-align should be used when compiling all files. Dann, could you try compiling with this flag instead of -mno-expensive-optimizations and see if you still hit the bug? Unfortunately I don't have access to an aarch64 machine with KVM available.

Best regards.

[dann frazier (dannf)]

@Thomas: I can confirm that replacing -fno-expensive-optimizations with -mstrict-align does work.

[Thomas Preud'homme (thomas-preudhomme)]

Great. Then I think the bug should be closed against GCC and the edk2 patch updated.

Best regards.

[Steve Langasek (vorlon)]

The updated version of this patch has landed in xenial (but synced, so no automatic bug closure).