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

[Laszlo Ersek <lersek@redhat.com>]

On 12/05/16 03:55, Gao, Liming wrote:
> Laszlo:
> Thanks for your investigation. In tools_def.txt, there are two
> chains: GCC49 and GCC5. GCC49 enables Os without lto, GCC5 enables Os
> and lto. If GCC version supports lto well, it can use GCC5 tool
> chain. Otherwise, it can use GCC49 tool chain. I suggest to add
> comments in GCC5 tool chain to document the known workable GCC
> version. From below comments, only GCC5.3 and GCC5.4 can work with
> GCC5 tool chain with lto enable.

Anthony, I suggest that you please send a new patch series, with two
patches:

- The first patch should be the same as your current patch #1, but it
should have a non-empty commit message. Please state that EFIAPI is
necessary for functions declared in library class header files.

- The second patch should be for "OvmfPkg/build.sh". gcc versions 6.0,
6.1, and 6.2 should be mapped to the GCC49 toolchain:

      4.9.*|6.[0-2].*)
        TARGET_TOOLS=GCC49

Jordan, do you agree?

Thanks
Laszlo

> 
> Thanks
> Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Sunday, December 04, 2016 1:59 AM
> To: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@ml01.01.org; Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
> 
> On 12/02/16 20:26, Laszlo Ersek wrote:
>> On 12/02/16 17:02, Anthony PERARD wrote:
>>> On Thu, Dec 01, 2016 at 07:43:24PM +0100, Laszlo Ersek wrote:
>>>> On 12/01/16 16:28, Anthony PERARD wrote:
>>>>> Hi,
>>>>>
>>>>> That might be only with the Xen part of OVMF but now that the GCC5 
>>>>> toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF 
>>>>> fail to boot in Xen guests.
>>>>>
>>> [...]
>>>>>
>>>>> Removing the gcc option -flto in only the XenBusDxe module makes 
>>>>> OVMF boot.
>>>>>
>>>>> While trying to debug that, I've added some debug prints (in this 
>>>>> module and in XenPvBlkDxe), and the exception could change and 
>>>>> become a "page fault" instead, or even an assert failure in the 
>>>>> PrintLib, that was the ASSERT(Buffer != NULL) at I think
>>>>> MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
>>>>>
>>>>> Adding EFIAPI to internal functions in XenBusDxe makes things work 
>>>>> again.  My guest is that gcc would bypass (optimise) an exported 
>>>>> functions and call directly an internal one but without reordering 
>>>>> the arguments (EFIAPI vs nothing).
>>>>>
>>>>> Does that make sense?
>>>>
>>>> If "-b NOOPT" works for you, I'd prefer that as a temporary solution 
>>>> (until the root cause is found and addressed) to the XenBusDxe patches.
>>>
>>> That works, using GCC49 (with gcc 6.2.1) works as well.
>>>
>>>> Hrpmf, wait a second, I do see something interesting: in this series 
>>>> you
>>>> *are* modifying APIs declared in a library class header (namely 
>>>> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
>>>> libraries) *are* required to specify EFIAPI.
>>>>
>>>> What happens if you apply patch #1 only?
>>>
>>> With only XenHypercallLib changes, the error is the same.
>>>
>>> But I did find the minimum change needed, it envolve a function with 
>>> a VA_LIST as argument.
>>>
>>> With only the following patch, OVMF works again.
>>>
>>> diff --git a/OvmfPkg/XenBusDxe/XenStore.c 
>>> b/OvmfPkg/XenBusDxe/XenStore.c index 1666c4b..85b0956 100644
>>> --- a/OvmfPkg/XenBusDxe/XenStore.c
>>> +++ b/OvmfPkg/XenBusDxe/XenStore.c
>>> @@ -1307,6 +1307,7 @@ XenStoreTransactionEnd (  }
>>>  
>>>  XENSTORE_STATUS
>>> +EFIAPI
>>>  XenStoreVSPrint (
>>>    IN CONST XENSTORE_TRANSACTION *Transaction,
>>>    IN CONST CHAR8           *DirectoryPath,
>>> diff --git a/OvmfPkg/XenBusDxe/XenStore.h 
>>> b/OvmfPkg/XenBusDxe/XenStore.h index c9d4c65..33bb647 100644
>>> --- a/OvmfPkg/XenBusDxe/XenStore.h
>>> +++ b/OvmfPkg/XenBusDxe/XenStore.h
>>> @@ -209,6 +209,7 @@ XenStoreSPrint (
>>>             indicating the type of write failure.
>>>  **/
>>>  XENSTORE_STATUS
>>> +EFIAPI
>>>  XenStoreVSPrint (
>>>    IN CONST XENSTORE_TRANSACTION *Transaction,
>>>    IN CONST CHAR8           *DirectoryPath,
>>>    IN CONST CHAR8           *Node,
>>>    IN CONST CHAR8           *FormatString,
>>>    IN VA_LIST               Marker
>>>    );
>>>
>>>
>>> I think the exception happen when this function is called via
>>> XENBUS_PROTOCOL->XsPrintf() from XenPvBlockFrontInitialization() in 
>>> OvmfPkg/XenPvBlkDxe/BlockFront.c
>>>
>>
>> It used to be a known requirement / limitation that all functions with 
>> variable argument lists had to be EFIAPI, regardless of cross-module 
>> use. However, commit 48d5f9a551a93acb45f272dda879b0ab5a504e36 changed 
>> that, and varargs should "just work" now. I suspect this is a
>> __builtin_ms_va_* regression in gcc-6. Thank you for narrowing it down.
>> It might make sense to report a bug in the upstream gcc tracker.
>>
>> ... Oh wow, this is a known gcc bug! See:
>>
>> https://lists.01.org/pipermail/edk2-devel/2016-August/001018.html
>>
>> Upstream gcc BZ <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955> 
>> was apparently solved for "Target Milestone: 6.3" (your version is 6.2.1).
>> So we'll either need a GCC6 toolchain in BaseTools that drops -flto, 
>> in order to work around this gcc issue, or we'll have to ask gcc-6 
>> users to use at least gcc-6.3.
>>
>> Oh wait, gcc-6.3 hasn't been released yet. We need the BaseTools 
>> workaround then.
> 
> I think I got confused in parts of the above; I got some details wrong.
> Namely, commit 48d5f9a551a9 did not remove the requirement/limitation that all varargs functions have to be EFIAPI. Said commit only changed how the VA_*() macros would be implemented.
> 
> The two caller functions of XenStoreVSPrint(), namely XenStoreSPrint() and XenBusXenStoreSPrint(), are varargs functions, but they are already EFIAPI. So the requirement/limitation (which was unaffected by
> 48d5f9a551a9) is actually satisfied / considered in XenBusDxe.
> 
> The XenStoreVSPrint() function, which you identified as the breaking part, is *not* a varargs function itself, so it needn't be EFIAPI. It simply receives a VA_LIST parameter (which is "__builtin_ms_va_list", from commit 48d5f9a551a9), and (a) copies it with VA_COPY() for passing the copy to SPrintLengthAsciiFormat(), (b) passes the original parameter to AsciiVSPrint(). In turn both of those functions call the common
> BasePrintLibSPrintMarker() function.
> 
> Comment <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c6> says,
> 
>> This is bug report that the specialized 
>> __builtin_ms_va_{list,start,end,copy} builtins have stopped working 
>> when -flto is used.  They worked until gcc 5.3, both with or without 
>> -flto.  In gcc 6.1 with -flto, the canonical iterator __builtin_va_arg 
>> ignores them and works on a sysv_va_list.  To be precise, it's 
>> __builtin_va_arg in the context of -flto that's broken, not the 
>> specialized builtins.  __builtin_va_arg has always been a polymorphic 
>> builtin that changes its behavior based on the type of va_list it's 
>> given as an argument.  Without this polymorphic behavior, there's no 
>> way to iterate over an ms_va_list.
> 
> Apparently, when BasePrintLibSPrintMarker() finally calls VA_ARG() (== __builtin_va_arg(), from commit 48d5f9a551a9) on Marker / Marker2, with LTO enabled, __builtin_va_arg() fails to notice what context VaListMarker comes from:
> - __builtin_ms_va_start() in XenStoreSPrint() and XenBusXenStoreSPrint(), or
> - __builtin_ms_va_copy() in XenStoreVSPrint().
> 
> So I think we *are* being hit by gcc BZ#70955, and making
> XenStoreVSPrint() EFIAPI only masks the issue. Comment <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c7> seems relevant:
> 
>> The change with GCC 6 is that the builtins are now lowered during 
>> link-time optimization rather than at compile-time.  Thus the abi 
>> selection bits are possibly not transfered correctly (type merging?).
>> I remember the business was quite ugly, but eventually we just miss to 
>> properly transfer the function attribute.
> 
> The end result for edk2 remains the same (= BaseTools should work around this gcc issue with a new GCC6 toolchain that drops -flto, unless
> gcc-6.3 is about to become available to users real quick). I just wanted to point out that my earlier statement "commit 48d5f9a551a9 had removed the need for varargs functions to be EFIAPI" was incorrect -- varargs functions still must be EFIAPI (and XenBusDxe conforms, see
> XenStoreSPrint() and XenBusXenStoreSPrint()).
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>