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 9ABC882184 for ; Thu, 23 Feb 2017 02:19:07 -0800 (PST) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (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 1BC8C80467; Thu, 23 Feb 2017 10:19:08 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-58.phx2.redhat.com [10.3.116.58]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1NAJ5fr009041; Thu, 23 Feb 2017 05:19:06 -0500 To: "Gao, Liming" , Anthony PERARD , Chao Zhang , David Wei , "Guo, Mang" 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> Cc: Ard Biesheuvel , "Justen, Jordan L" , "edk2-devel@ml01.01.org" From: Laszlo Ersek Message-ID: <3250749d-e8b3-3143-734f-a922e15e3c91@redhat.com> Date: Thu, 23 Feb 2017 11:19:03 +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: <4A89E2EF3DFEDB4C8BFDE51014F606A14D6E2BC3@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Thu, 23 Feb 2017 10:19:08 +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: Thu, 23 Feb 2017 10:19:07 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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_list 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. > > 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 EdkCompatibilityPkg -- I indeed found only a handful of affected functions: - XenStoreVSPrint() [OvmfPkg/XenBusDxe/XenStore.c] - VariableGetBestLanguage() [SecurityPkg/VariableAuthenticated/EsalVariableDxeSal/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 SmmBootScriptWrite()? Thanks, Laszlo > > 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 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 >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel