From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 25D8F82212 for ; Tue, 21 Feb 2017 11:02:52 -0800 (PST) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 950ED3B3C7; Tue, 21 Feb 2017 19:02:52 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-47.phx2.redhat.com [10.3.116.47]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1LJ2nKp014830; Tue, 21 Feb 2017 14:02:50 -0500 To: Anthony PERARD References: <20161201152819.8341-1-anthony.perard@citrix.com> <53c67cb5-e947-8979-7738-288cc83f374b@redhat.com> <20161202160201.GA1848@perard.uk.xensource.com> <9c0c9f43-a297-179f-2d57-fa5d8fab3763@redhat.com> <20170221163922.GC1867@perard.uk.xensource.com> <97214320-6054-8034-7667-6edde4debd80@redhat.com> <20170221175303.GD1867@perard.uk.xensource.com> Cc: Jordan Justen , "Gao, Liming" , "Zhu, Yonghong" , Ard Biesheuvel , edk2-devel@ml01.01.org, Rebecca Cran , Konrad Rzeszutek Wilk From: Laszlo Ersek Message-ID: <2a9ad7dd-fbeb-5a36-091f-d8592e95c509@redhat.com> Date: Tue, 21 Feb 2017 20:02:48 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <20170221175303.GD1867@perard.uk.xensource.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Tue, 21 Feb 2017 19:02:52 +0000 (UTC) Subject: Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 21 Feb 2017 19:02:52 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 02/21/17 18:53, Anthony PERARD wrote: > On Tue, Feb 21, 2017 at 06:07:15PM +0100, Laszlo Ersek wrote: >> CC Rebecca & Konrad >> >> On 02/21/17 17:39, Anthony PERARD wrote: [snip] >>> So, should I add EFIAPI to XenStoreVSPrint, as it is using VA_COPY? >>> >> >> Hm, please help me jog my memory... >> >> If I remember correctly, this is still a GCC bug, one that we suppressed for gcc-6.2 with your patch as follows: > > Yes. > >>> commit 432f1d83f77acf92d52ef18d2cee6dbf7c5b9b86 >>> Author: Anthony PERARD >>> Date: Tue Dec 6 12:03:25 2016 +0000 >>> >>> OvmfPkg/build.sh: Use GCC49 toolchains with GCC 6.[0-2] >>> >>> The goal of the patch is to avoid using -flto with GCC 6.0 to 6.2. >>> >>> This is to workaround a GCC bug: >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Anthony PERARD >>> Reviewed-by: Laszlo Ersek >>> Regression-tested-by: Laszlo Ersek >>> >>> diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh >>> index 95fe8fb07647..b6e936056ca0 100755 >>> --- a/OvmfPkg/build.sh >>> +++ b/OvmfPkg/build.sh >>> @@ -102,7 +102,7 @@ case `uname` in >>> 4.8.*) >>> TARGET_TOOLS=GCC48 >>> ;; >>> - 4.9.*) >>> + 4.9.*|6.[0-2].*) >>> TARGET_TOOLS=GCC49 >>> ;; >>> *) >> >> Do I understand correctly that the gcc bug has not been fixed in >> gcc-6.3, and -- because we don't suppress it for gcc-6.3 as the >> above expression does not match -- it causes problems again? > > The bug describe in the GCC bugzilla is probably fix, but the > test-case does not make use of __builtin_ms_va_copy. :/ > >> You also mention gcc-5.4 as problematic. I think we haven't >> received such reports about gcc-5 versions up to and including >> gcc-5.3 (that's why GCC5 is the default selection in >> "OvmfPkg/build.sh"). Do you mean that the gcc bug has now been >> "backported" from the gcc-6 series to the gcc-5 series (starting >> with gcc-5.4)? > > I don't know the state of gcc-5.0 to gcc-5.3, I have never tested -flto > with gcc-5.x (until now), I would say they are also problematic until > proven otherwise. When we enabled GCC5, it definitely worked for at least one gcc release, with -flto. (-flto is the default for DEBUG and RELEASE builds with GCC5; NOOPT disables -Os and -flto.) > >> If that's the case, then I suggest flipping "OvmfPkg/build.sh" from >> black-listing gcc versions for -flto to white-listing. In other >> words, assume that -flto is generally broken with GCC, except for a >> few known versions: 5.0 through 5.3 inclusive. Those versions >> should trigger the use of the GCC5 toolchain, and everything else >> (5.4+, 6.*, 4.9.*) should use GCC49. >> >> I don't feel comfortable about adding EFIAPI to XenStoreVSPrint >> just because it takes a VA_LIST parameter -- note: it is *not* a >> varargs function itself! --; the same issue might hit elsewhere in >> the edk2 tree at any time, outside of OvmfPkg too. > > From the different tests I've done, I feel more like VA_COPY might be > the issue, but I don't know how __builtin_ms_va_* are supposed to be > used. If I recall correctly, from the upstream GCC bug, the problem is that __builtin_va_list does not track internally whether it was created in an msabi or sysvabi function, and therefore the va_* functions cannot be used transparently on it. Instead, when va_list is accessed, the accessor builtins seem to apply the currently executing function's calling convetion to va_list. (Even if the creation context of va_list was different.) > >> Would the gcc white-listing work for you? >> >> Note that the white-listing would practically undo Konrad's commit >> 2667ad40919a ("OvmfPkg/build.sh: Make GCC5 the default toolchain, >> catch GCC43 and earlier", 2016-11-23), but given the recent gcc >> developments (gcc-6.3 has not fixed the gcc bug, and the bug has >> even surfaced in gcc-5.4), I think it would be justified. > > Do be honnest, I don't think the toolchain GCC5 has ever been tested > with gcc-5.x and the module XenBusDxe. I think most people that want to > start OVMF under Xen are likely to build it with gcc-4.9 or already had > gcc-6.x when OVMF switch to the GCC5 toolchain by default. > Okay... I'm equally fine if we just say "given that GCC is broken like this, we hereby require all functions that take a variable argument list, *or* a VA_LIST parameter, to be EFIAPI". (The first part of the requirement already exists.) But in this case, the full edk2 codebase has to be grepped for VA_LIST-taking functions, and all of them must be flipped to EFIAPI, if they currently aren't EFIAPI. Covering just XenStoreVSPrint() seems incomplete. (Note: CryptoPkg/Library/OpensslLib is an exception.) Also, in this case, your commit 432f1d83f77a should likely be reverted. (Because we are ultimately giving in to the gcc bug.) Thanks Laszlo