Re: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for CLANGPDB

["Vitaly Cheptsov" <vit9696@protonmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7627 bytes --]

Liming,

Yes, perhaps that was unclear from my message. From what I understand __builtin_va_start behaves the same way as __builtin_ms_va__start for EFIAPI functions, and these are currently the only functions where variadic arguments are allowed.

For non-EFIAPI functions the calling conventions may be different from Microsoft 64-bit, i.e. they are System V for CLANG, CLANGPDB, and GCC. If you use variadic arguments in non-EFIAPI functions (which I believe is currently unsupported), you will end up with different calling conventions for variadic arguments as well. In this case the use of __builtin_va_start will work fine here, as System V variadic arguments will be used for non-EFIAPI, and Microsoft 64-bit for EFIAPI, while __builtin_ms_va_start may technically break non-EFIAPI.

Perhaps I do not fully understand the underlying mechanism, but that is how I have always thought it works.

Best wishes,
Vitaly

On Wed, Feb 12, 2020 at 17:01, Gao, Liming <liming.gao@intel.com> wrote:

> Vitaly:
>
>   With this change, X64 GCC and CLANG tool chain will use below VA_START definition.
>
> #define VA_START(Marker, Parameter)  __builtin_ms_va_start (Marker, Parameter)
>
> Thanks
>
> Liming
>
> From: devel@edk2.groups.io <devel@edk2.groups.io>  On Behalf Of Vitaly Cheptsov via Groups.Io
> Sent: Wednesday, February 12, 2020 4:08 PM
> To: Liu, Zhiguang <zhiguang.liu@intel.com>
> Cc: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Shi, Steven <steven.shi@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for CLANGPDB
>
> Liu,
>
> Thanks for explanation, it does make sense now. As for no need to -DNO_MSABI_VA_FUNCS I agree, but it will not make much difference, because from what I understand the VA_ARG implementation is chosen based on EFIAPI presence when generic __builtin’s are used.
>
> Best,
>
> Vitaly
>
>> 12 февр. 2020 г., в 04:38, Liu, Zhiguang <zhiguang.liu@intel.com> написал(а):
>>
>> Hi Vitaly,
>>
>> After your patch to Switch to GNU mode for CLANGPDB, the build option -DNO_MSABI_VA_FUNCS is not required. I will send another patch to remove it.
>>
>> And for you question, this is a patch set that resolves BZ 2415, and the second patch 21821933aea284cd3dfea6994bd4b83bd9739fc9 has direct influence to CLANG38.
>>
>> Thanks
>>
>> Zhiguang
>>
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov via Groups.Io
>> Sent: Tuesday, February 11, 2020 3:09 PM
>> To: Gao, Liming <liming.gao@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>; Shi, Steven <steven.shi@intel.com>
>> Cc: devel@edk2.groups.io
>> Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for CLANGPDB
>>
>> Liming,
>>
>> Done. As a side note, I am not positive how can 7990438f1437f47990a8890dee51978cb8dbc25c[1] resolve BZ 2415[2]. The bug was about CLANG38, and the toolchain updated was CLANGPDB. While it makes sense to update CLANGPDB with this flag to stay clean (it will not make a difference for clang in GNU mode), CLANGPDB has nothing to do to CLANG38.
>>
>> Best wishes,
>>
>> Vitaly
>>
>> [1] https://bugzilla.tianocore.org/show_bug.cgi?id=2415
>>
>> [2] https://github.com/tianocore/edk2/commit/7990438f1437f47990a8890dee51978cb8dbc25c
>>
>>> 11 февр. 2020 г., в 09:02, Gao, Liming <liming.gao@intel.com> написал(а):
>>>
>>> Vitaly:
>>>
>>>   Can you update this patch based on the latest edk2 trunk? I will catch it for edk2 Q1 stable tag.
>>>
>>> Thanks
>>>
>>> Liming
>>>
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Liming Gao
>>> Sent: Tuesday, February 11, 2020 1:34 PM
>>> To: vit9696 <vit9696@protonmail.com>; devel@edk2.groups.io
>>> Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for CLANGPDB
>>>
>>> Reviewed-by: Liming Gao <liming.gao@intel.com>
>>>
>>> From: vit9696 <vit9696@protonmail.com>
>>> Sent: Tuesday, February 11, 2020 3:23 AM
>>> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io
>>> Subject: RE: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for CLANGPDB
>>>
>>> Liming,
>>>
>>> We did run several of our projects based on EDK II in X64 mode, DEBUG, RELEASE, NOOPT. Noticed no change from XCODE5.
>>>
>>> We also tried building several EDK builtin packages like CryptoPkg, MdePkg, MdeModulePkg.
>>>
>>> Best wishes,
>>>
>>> Vitaly
>>>
>>> В пн, февр. 10, 2020 в 16:47, Gao, Liming <liming.gao@intel.com> пишет:
>>>
>>>> Vitaly:
>>>> This change is good. Can you your test for it? I verify this patch for Ovmf platform on Windows. It can make ovmf pass build with CLANGPDB.
>>>>
>>>> Thanks
>>>> Liming
>>>>> -----Original Message-----
>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov via Groups.Io
>>>>> Sent: Monday, February 10, 2020 6:59 PM
>>>>> To: devel@edk2.groups.io
>>>>> Subject: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for CLANGPDB
>>>>>
>>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2397
>>>>>
>>>>> Signed-off-by: Vitaly Cheptsov <vit9696@protonmail.com>
>>>>> ---
>>>>> BaseTools/Conf/tools_def.template | 6 +++---
>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
>>>>> index feee2bbf16..6bf6c5768e 100755
>>>>> --- a/BaseTools/Conf/tools_def.template
>>>>> +++ b/BaseTools/Conf/tools_def.template
>>>>> @@ -2755,11 +2755,11 @@ RELEASE_CLANG38_AARCH64_DLINK_FLAGS = DEF(CLANG38_AARCH64_DLINK_FLAGS) -flto -Wl
>>>>> DEFINE CLANGPDB_IA32_PREFIX = ENV(CLANG_BIN)
>>>>> DEFINE CLANGPDB_X64_PREFIX = ENV(CLANG_BIN)
>>>>>
>>>>> -DEFINE CLANGPDB_IA32_TARGET = -target i686-unknown-windows
>>>>> -DEFINE CLANGPDB_X64_TARGET = -target x86_64-unknown-windows
>>>>> +DEFINE CLANGPDB_IA32_TARGET = -target i686-unknown-windows-gnu
>>>>> +DEFINE CLANGPDB_X64_TARGET = -target x86_64-unknown-windows-gnu
>>>>>
>>>>> DEFINE CLANGPDB_WARNING_OVERRIDES = -Wno-parentheses-equality -Wno-tautological-compare -Wno-tautological-constant-out-
>>>>> of-range-compare -Wno-empty-body -Wno-unused-const-variable -Wno-varargs -Wno-unknown-warning-option -Wno-microsoft-enum-
>>>>> forward-reference
>>>>> -DEFINE CLANGPDB_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector -
>>>>> mms-bitfields -Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas -Wno-incompatible-library-redeclaration -fno-
>>>>> asynchronous-unwind-tables -mno-implicit-float -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang -
>>>>> funsigned-char -fno-ms-extensions -Wno-null-dereference -fms-compatibility -mno-stack-arg-probe
>>>>> +DEFINE CLANGPDB_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector -
>>>>> fno-asynchronous-unwind-tables -funsigned-char -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang -Wno-
>>>>> address -Wno-shift-negative-value -Wno-unknown-pragmas -Wno-incompatible-library-redeclaration -Wno-null-dereference -mno-
>>>>> implicit-float -mms-bitfields -mno-stack-arg-probe -nostdlib -nostdlibinc
>>>>>
>>>>> ###########################
>>>>> # CLANGPDB IA32 definitions
>>>>> --
>>>>> 2.21.1 (Apple Git-122.3)
>>>>>
>>>>>
>>>>> -=-=-=-=-=-=
>>>>> [Groups.io](http://groups.io/) Links: You receive all messages sent to this group.
>>>>>
>>>>> View/Reply Online (#54130): https://edk2.groups.io/g/devel/message/54130
>>>>> Mute This Topic: https://groups.io/mt/71134286/1759384
>>>>> Group Owner: devel+owner@edk2.groups.io
>>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [liming.gao@intel.com]
>>>>> -=-=-=-=-=-=
>
> 

[-- Attachment #2: Type: text/html, Size: 16720 bytes --]