Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h

[Laszlo Ersek <lersek@redhat.com>]

On 12/12/17 02:32, Gao, Liming wrote:
> Laszlo:
>   Have you any comments for this patch? Seemly, you discussed this topic in bugzillar. 

Thanks for the ping, I'll comment under the thread starter message.

Thanks!
Laszlo

>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Zenith432
>> Sent: Monday, December 11, 2017 10:58 PM
>> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
>> Subject: Re: [edk2] [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h
>>
>> Yes, that's right, in mixed ABI on GCC 7.2 (which is what GCC5 toolchain in tools_def.sample creates...), VA_COPY when
>> used inside native (non-EFIAPI) function causes incorrect code generation that leads to crash (it mistreats the
>> __builtin_ms_va_list as a __builtin_sysv_va_list).
>>
>> The same source code works in current versions of LLVM and Apple clang - so it's a compiler-specific bug.
>>
>> The text I wrote
>>  >> +//  -- VA_END, VA_ARG, VA_COPY, va_end, va_arg, and va_copy may be used
>>  >> +//     in functions of either calling conventions.
>>
>> is the hypothetical desired condition, but it depends on compilers generating proper code in mixed ABI.  It obviously
>> does not hold for current GCC.
>>
>> However, the part of the documentation to add EFIAPI to ExampleVarArg should definitely be fixed.  Using
>> __builtin_ms_va_list (=VA_LIST) in a sysv_abi (native) function probably yields a compile-time diagnostic, and if not -
>> generates code that crashes for sure.
>>
>> On 11/12/2017 04:35 PM, Gao, Liming wrote:
>>> Hi,
>>>    I see you and Laszlo are still in discussion on GCC behavior. Because there is one bug in GCC compiler, the following functions in
>> edk2 must be EFIAPI. Right?
>>>
>>> - functions that have a variable argument list and call VA_START / VA_END
>>> - functions that call VA_COPY
>>>
>>> Thanks
>>> Liming
>>>> -----Original Message-----
>>>> From: Zenith432 [mailto:zenith432@users.sourceforge.net]
>>>> Sent: Sunday, December 10, 2017 11:43 PM
>>>> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>>>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
>>>> Subject: Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h
>>>>
>>>> On 10/12/2017 03:53 PM, Gao, Liming wrote:
>>>>> Could you add bug 457 link in the commit message?
>>>> ---
>>>> Subject: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h
>>>>
>>>> This is to resolve bug 457.
>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=457
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Zenith432 <zenith432 at users.sourceforge.net>
>>>> ---
>>>>     MdePkg/Include/Base.h | 24 ++++++++++++++++++++----
>>>>     1 file changed, 20 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
>>>> index 02140a5a..19f36872 100644
>>>> --- a/MdePkg/Include/Base.h
>>>> +++ b/MdePkg/Include/Base.h
>>>> @@ -560,13 +560,14 @@ struct _LIST_ENTRY {
>>>>     //  VA_LIST  - typedef for argument list.
>>>>     //  VA_START (VA_LIST Marker, argument before the ...) - Init Marker for use.
>>>>     //  VA_END (VA_LIST Marker) - Clear Marker
>>>> -//  VA_ARG (VA_LIST Marker, var arg size) - Use Marker to get an argument from
>>>> -//    the ... list. You must know the size and pass it in this macro.
>>>> +//  VA_ARG (VA_LIST Marker, var arg type) - Use Marker to get an argument from
>>>> +//    the ... list. You must know the type and pass it in this macro.
>>>>     //  VA_COPY (VA_LIST Dest, VA_LIST Start) - Initialize Dest as a copy of Start.
>>>>     //
>>>> -//  example:
>>>> +//  Example:
>>>>     //
>>>>     //  UINTN
>>>> +//  EFIAPI
>>>>     //  ExampleVarArg (
>>>>     //    IN UINTN  NumberOfArgs,
>>>>     //    ...
>>>> @@ -582,7 +583,7 @@ struct _LIST_ENTRY {
>>>>     //    VA_START (Marker, NumberOfArgs);
>>>>     //    for (Index = 0, Result = 0; Index < NumberOfArgs; Index++) {
>>>>     //      //
>>>> -//      // The ... list is a series of UINTN values, so average them up.
>>>> +//      // The ... list is a series of UINTN values, so sum them up.
>>>>     //      //
>>>>     //      Result += VA_ARG (Marker, UINTN);
>>>>     //    }
>>>> @@ -591,6 +592,21 @@ struct _LIST_ENTRY {
>>>>     //    return Result
>>>>     //  }
>>>>     //
>>>> +//  Notes:
>>>> +//
>>>> +//  This set of macros is intended to support variadic functions that
>>>> +//  use the EFIAPI calling convention.  Variadic functions that use a
>>>> +//  native calling convention should use stdarg.h.
>>>> +//  In particular:
>>>> +//  -- VA_START may only be used in a variadic EFIAPI function.
>>>> +//  -- va_start may only be used in a variadic native function.
>>>> +//  -- VA_START, VA_END, VA_ARG and VA_COPY may only be used on a VA_LIST.
>>>> +//  -- va_start, va_end, va_arg and va_copy may only be used on a va_list.
>>>> +//  -- Both VA_LIST or va_list may be passed as arguments to functions
>>>> +//     of either EFIAPI or native calling conventions.
>>>> +//  -- VA_END, VA_ARG, VA_COPY, va_end, va_arg, and va_copy may be used
>>>> +//     in functions of either calling conventions.
>>>> +//
>>>>
>>>>     /**
>>>>       Return the size of argument that has been aligned to sizeof (UINTN).
>>>> --
>>>> 2.14.3
>>>
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel