From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (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 129E782226 for ; Wed, 22 Feb 2017 00:54:23 -0800 (PST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Feb 2017 00:54:22 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,193,1484035200"; d="scan'208";a="827117379" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by FMSMGA003.fm.intel.com with ESMTP; 22 Feb 2017 00:54:22 -0800 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 22 Feb 2017 00:54:22 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx115.amr.corp.intel.com (10.18.116.19) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 22 Feb 2017 00:54:21 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.88]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.204]) with mapi id 14.03.0248.002; Wed, 22 Feb 2017 16:54:18 +0800 From: "Gao, Liming" To: Laszlo Ersek , Anthony PERARD CC: Ard Biesheuvel , "Justen, Jordan L" , "edk2-devel@ml01.01.org" Thread-Topic: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 Thread-Index: AQHSjGthWJiwml7su06VLG9E0bLQvKFzSzsAgAFpD7A= Date: Wed, 22 Feb 2017 08:54:17 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D6E2BC3@shsmsx102.ccr.corp.intel.com> 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> <2a9ad7dd-fbeb-5a36-091f-d8592e95c509@redhat.com> In-Reply-To: <2a9ad7dd-fbeb-5a36-091f-d8592e95c509@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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: Wed, 22 Feb 2017 08:54:23 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo: In edk2, I find the several functions with VA_LIST have no EFIAPI. They m= ay use VA_ARG() or call other functions, but they don't use VA_COPY(). In B= ase.h, VA_ARG() is defined as __builtin_va_arg(), which is same to native o= ne. VA_COPY() is defined as __builtin_ms_va_copy(). So, I also think this i= s MS ABI request. That means only if the function implementation uses VA_ST= ART(),VA_END() or VA_COPY(), it must be declared with EFIAPI. MdePkg\Library\BasePrintLib\PrintLibInternal.c BasePrintLibSPrintMarker() ShellPkg\Library\UefiShellLib\UefiShellLib.c InternalShellPrintWorker() Thanks Liming >-----Original Message----- >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >Laszlo Ersek >Sent: Wednesday, February 22, 2017 3:03 AM >To: Anthony PERARD >Cc: Ard Biesheuvel ; Justen, Jordan L >; edk2-devel@ml01.01.org; Gao, Liming > >Subject: Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when >compiled with GCC5 > >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 suppresse= d 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=3D70955 >>>> >>>> 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=3DGCC48 >>>> ;; >>>> - 4.9.*) >>>> + 4.9.*|6.[0-2].*) >>>> TARGET_TOOLS=3DGCC49 >>>> ;; >>>> *) >>> >>> 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 >_______________________________________________ >edk2-devel mailing list >edk2-devel@lists.01.org >https://lists.01.org/mailman/listinfo/edk2-devel