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

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

Laszlo:
  
- VariableGetBestLanguage() [SecurityPkg/VariableAuthenticated/EsalVariableDxeSal/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. 

Thanks
Liming
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Thursday, February 23, 2017 6:19 PM
To: Gao, Liming <liming.gao@intel.com>; Anthony PERARD <anthony.perard@citrix.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Wei, David <david.wei@intel.com>; Guo, Mang <mang.guo@intel.com>
Cc: Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@ml01.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled 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_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 <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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel