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

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

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.  

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