Vitaly:
  We also find _MSC_VER is defined in Windows, but not in Linux. Your analysis 
explains it. When use i686-unknown-windows-gnu option, __GNUC__ macro will be 
defined. If so, we don’t need to append the check for defined (__clang__) in 
Base.h. And, this change can remove -fno-ms-extensions and -fms-compatibility 
option. Then, CLANGPDB can keep the same behavior in Windows/Linux/MacOs host 
OS.

#if defined (__GNUC__) || defined (__clang__)
==>
#if defined (__GNUC__)

Thanks
Liming
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov 
via Groups.Io
Sent: Thursday, February 6, 2020 8:17 AM
To: Gao, Liming <liming....@intel.com>
Cc: devel@edk2.groups.io; Marvin Häuser <marvin.haeu...@outlook.com>; Laszlo 
Ersek <ler...@redhat.com>; Kinney, Michael D <michael.d.kin...@intel.com>
Subject: Re: [edk2-devel] [PATCH 0/1] Use _MSC_VER to determine MSVC compiler

Liming,

We performed the initial exploration of CLANGPDB toolchain issue on our end and 
believe we can suggest a solid solution.

In addition to all the issues I mentioned in the BZ[1] there are several more.

1. CLANGPDB uses -target x86_64-unknown-windows, and this basically means 
different behaviour for Windows and other operating systems:
- On Windows it will "attach" to installed Visual Studio and will gather the 
parameters from this installation, i.e. it will define _MSC_VER to installed 
Visual Studio version. For example, for me it is implicitly passing 
-fms-compatibility-version=19.16.27026 and setting full triple to 
x86_64-unknown-windows-msvc19.16.27026.
- On Mac and Linux it will obviously not find Visual Studio, and as a result 
the full triple will be x86_64-unknown-windows-msvc with _MSC_VER macro not 
being defined.

There basically is no control to it except -U_MSC_VER, which is ugly, as 
different include directories, other defines will still happen between 
installations.

2. EDK II relies on UINT32_MAX being a valid value for enum. This is not the 
case in the specification, as it requires enum to be either INT32 or UINT32:

Element of a standard ANSI C enum type declaration. Type INT32.or UINT32. ANSI 
C does not define the size of sign of an enum so they should never be used in 
structures. ANSI C integer promotion rules make INT32 or UINT32 interchangeable 
when passed as an argument to a function.

However, since I am not positive that no existing code relies on this, it is 
best to preserve the current behaviour. Supporting this is valid for GNU 
flavour or as a Microsoft Extension. Disabling -fms-compatibility will result 
in a compile error for enums having 0xFFFFFFFF values, like in Base.h.

All in all, we believe that CLANGPDB simply has an overlook in the -target 
argument due to a misconsideration of the clang triple implementation. Normally 
for target only 3 words are provided, but for Windows it is crucial to have 4, 
as there are different drivers with different automatics. To resolve the 
problem, we should use GNU targets i686-unknown-windows-gnu and 
x86_64-unknown-windows-gnu. This is basically the only and the least hurtful 
solution, as using MSVC mode will define _MSC_EXTENSIONS, which already breaks 
many places and will require a heavy codebase refactoring, and randomly define 
_MSC_VER and use Visual Studio headers and configuration, which makes 
reproducible builds on different operating systems questionable if not 
impossible.


I will submit another patch that will replace this one later this week. In 
addition to GNU targets I additionally pass -nostdlib and -nostdlibinc so that 
a freestanding target is used and only builtin headers are accessible (like 
stdint.h, stddef.h, and stdbool.h). This is not required but an extra safety 
measure. Our initial validation of the changes found no issues with our 
projects. We also performed building of most common EDK II packages like 
CryptoPkg, MdePkg, and MdeModulePkg. While the change is fairly minor, I will 
still request others to perform a brief check of their packages in case they 
want to use CLANGPDB. For a quick test I include the diff of the patch 
beforehand.

Best wishes,
Vitaly

[1] https://bugzilla.tianocore.org/show_bug.cgi?id=2397




4 февр. 2020 г., в 09:56, Gao, Liming 
<liming....@intel.com<mailto:liming....@intel.com>> написал(а):

Vitaly:
  Yes. I think we should have better solution in OpenSSL to support EDK2 usage. 
But, this is a long term solution. For now, I will try the solution to remove 
-fms-compatibility option in CLANGPDB tool chain.

Thanks
Liming
From: vit9696 <vit9...@protonmail.com<mailto:vit9...@protonmail.com>>
Sent: Monday, February 3, 2020 7:29 PM
To: Gao, Liming <liming....@intel.com<mailto:liming....@intel.com>>; 
devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Marvin Häuser 
<marvin.haeu...@outlook.com<mailto:marvin.haeu...@outlook.com>>
Subject: RE: [edk2-devel] [PATCH 0/1] Use _MSC_VER to determine MSVC compiler

Liming,

I believe it is reasonable to fix OpenSSL, but most likely it is faster to 
address EDK II code at first. In future we should still stick to _MSC_VER, but 
for now just ensuring that any toolchain by MSVC does not define 
_MSC_EXTENSIONS should work too.

I believe that -fms-compatibility option is not needed for CLANGPDB, as 
CLANGPDB is literally using clang, and that worked fine before in CLANG38 and 
XCODE5. We may still have to update some preprocessor conditions to include 
__clang__ near __GNUC__ if __GNUC__ is left undefined even when we remove 
-fms-compatibility, but that sounds fine for me.

We will investigate that on our own and submit a new patch, but help from other 
parties is strongly appreciated. We mostly use XCODE5 and are not well aware of 
other platforms.

Best wishes,
Vitaly

On Mon, Feb 3, 2020 at 11:00, Gao, Liming 
<liming....@intel.com<mailto:liming....@intel.com>> wrote:
Vitaly:
This change will cause CryptoPkg openssl build failure, because OpensslLib.inf 
undefines _MSC_VER macro to make sure openssl source code be built in edk2 
project without using MS VS functions.

Now, I have no good solution to fix this issue. One way is to use 
defined(_MSC_EXTENSIONS) && !defined(__clang__), another way is to investigate 
whether we can remove -fms-compatibility option in CLANGPDB.

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> 
> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly 
> Cheptsov via Groups.Io
> Sent: Saturday, February 1, 2020 5:17 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Subject: [edk2-devel] [PATCH 0/1] Use _MSC_VER to determine MSVC compiler
>
> Patch details are explained in 
> https://bugzilla.tianocore.org/show_bug.cgi?id=2397.
> We request this to be merged in edk2-stable202002.
>
> Vitaly Cheptsov (1):
> MdePkg: Use _MSC_VER to determine MSVC compiler
>
> MdePkg/Include/AArch64/ProcessorBind.h | 2 +-
> MdePkg/Include/Arm/ProcessorBind.h | 8 ++++----
> MdePkg/Include/Base.h | 10 +++++-----
> MdePkg/Include/Ia32/ProcessorBind.h | 6 +++---
> MdePkg/Include/X64/ProcessorBind.h | 6 +++---
> 5 files changed, 16 insertions(+), 16 deletions(-)
>
> --
> 2.21.1 (Apple Git-122.3)
>
>
> -=-=-=-=-=-=
> Groups.io<http://Groups.io> Links: You receive all messages sent to this 
> group.
>
> View/Reply Online (#53623): https://edk2.groups.io/g/devel/message/53623
> Mute This Topic: https://groups.io/mt/70882954/1759384
> Group Owner: devel+ow...@edk2.groups.io<mailto:devel+ow...@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub 
> [liming....@intel.com<mailto:liming....@intel.com>]
> -=-=-=-=-=-=



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53846): https://edk2.groups.io/g/devel/message/53846
Mute This Topic: https://groups.io/mt/70882954/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Reply via email to