Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5

["Gao, Liming" <liming.gao@intel.com>]

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. 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.

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 <anthony.perard@citrix.com>
>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Justen, Jordan L
><jordan.l.justen@intel.com>; edk2-devel@ml01.01.org; Gao, Liming
><liming.gao@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.perard@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.perard@citrix.com>
>>>>     Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>>     Regression-tested-by: Laszlo Ersek <lersek@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