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

[Laszlo Ersek <lersek@redhat.com>]

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.

Thanks!
Laszlo