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-boun...@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Wednesday, February 22, 2017 3:03 AM >> To: Anthony PERARD <anthony.per...@citrix.com> >> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>; Justen, Jordan L >> <jordan.l.jus...@intel.com>; edk2-de...@ml01.01.org; Gao, Liming >> <liming....@intel.com> >> 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 <anthony.per...@citrix.com> >>>>> 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 <anthony.per...@citrix.com> >>>>> Reviewed-by: Laszlo Ersek <ler...@redhat.com> >>>>> Regression-tested-by: Laszlo Ersek <ler...@redhat.com> >>>>> >>>>> 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
