From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 9540382152 for ; Thu, 23 Feb 2017 05:00:20 -0800 (PST) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Feb 2017 05:00:20 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,197,1484035200"; d="scan'208";a="69269587" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga006.fm.intel.com with ESMTP; 23 Feb 2017 05:00:19 -0800 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 23 Feb 2017 05:00:19 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 23 Feb 2017 05:00:19 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.88]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.177]) with mapi id 14.03.0248.002; Thu, 23 Feb 2017 21:00:17 +0800 From: "Gao, Liming" To: Laszlo Ersek , Anthony PERARD , "Zhang, Chao B" , "Wei, David" , "Guo, Mang" CC: "Justen, Jordan L" , "edk2-devel@ml01.01.org" , Ard Biesheuvel Thread-Topic: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 Thread-Index: AQHSjGthWJiwml7su06VLG9E0bLQvKFzSzsAgAFpD7CAASlGgIAAsoqg Date: Thu, 23 Feb 2017 13:00:16 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D6E376F@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> <4A89E2EF3DFEDB4C8BFDE51014F606A14D6E2BC3@shsmsx102.ccr.corp.intel.com> <3250749d-e8b3-3143-734f-a922e15e3c91@redhat.com> In-Reply-To: <3250749d-e8b3-3143-734f-a922e15e3c91@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: Thu, 23 Feb 2017 13:00:20 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo: =20 - VariableGetBestLanguage() [SecurityPkg/VariableAuthenticated/EsalVariable= DxeSal/Variable.c] [Liming] It is for IPF arch only. We don't support IPF any longer. So, keep= it as-is - SmmBootScriptWrite() [Vlv2TbltDevicePkg/PlatformSmm/SmmScriptSave.c] [Liming] I am not sure MinnowMax platform supports GCC build or not. If it = supports GCC build, I agree to fix it.=20 Thanks Liming -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Lasz= lo Ersek Sent: Thursday, February 23, 2017 6:19 PM To: Gao, Liming ; Anthony PERARD ; Zhang, Chao B ; Wei, David ; Guo, Mang Cc: Justen, Jordan L ; edk2-devel@ml01.01.org; A= rd Biesheuvel Subject: Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compile= d with GCC5 On 02/22/17 09:54, Gao, Liming wrote: > Laszlo: > In edk2, I find the several functions with VA_LIST have no EFIAPI. > They may use VA_ARG() or call other functions, but they don't use > VA_COPY(). In Base.h, VA_ARG() is defined as __builtin_va_arg(), > which is same to native one. You are right; apparently __builtin_va_arg() works with __builtin_ms_va_lis= t and __builtin_va_list alike. However, as you say, > VA_COPY() is defined as > __builtin_ms_va_copy(). So, I also think this is MS ABI request. That > means only if the function implementation uses VA_START(),VA_END() or > VA_COPY(), it must be declared with EFIAPI. - __builtin_va_start / __builtin_ms_va_start, - __builtin_va_end / __builtin_ms_va_end, - __builtin_va_copy / __builtin_ms_va_copy must be matched to __builtin_va_list vs. __builtin_ms_va_list. >=20 > MdePkg\Library\BasePrintLib\PrintLibInternal.c BasePrintLibSPrintMarker() > ShellPkg\Library\UefiShellLib\UefiShellLib.c InternalShellPrintWorker() Yes, you are right -- when looking for functions that should be made EFIAPI= , we shouldn't search for VA_LIST, but VA_START|VA_END|VA_COPY. Thanks for the correction. The following command is a good start: git grep -E -n '\<(VA_START|VA_END|VA_COPY)\>|^[A-Za-z0-9_]' \ | grep -E -B 3 '\<(VA_START|VA_END|VA_COPY)\>' I just went over the output (it was gruesome), and -- outside of EdkCompati= bilityPkg -- I indeed found only a handful of affected functions: - XenStoreVSPrint() [OvmfPkg/XenBusDxe/XenStore.c] - VariableGetBestLanguage() [SecurityPkg/VariableAuthenticated/EsalVariable= DxeSal/Variable.c] - SmmBootScriptWrite() [Vlv2TbltDevicePkg/PlatformSmm/SmmScriptSave.c] Anthony, can you please submit the patch for XenStoreVSPrint()? Chao Zhang, can you please submit a patch for VariableGetBestLanguage()? David Wei or Mang Guo, can one of you guys please submit a patch for SmmBoo= tScriptWrite()? Thanks, Laszlo >=20 > 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 suppress= ed 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel