Liming, Correct, I believe it is not just Base.h but several other files. I plan to include the removal of __clang__ references in my patchset as well, since after the target change all the use of clang will be in GNU mode.
In addition to that, I believe that in GNU mode it should be also possible to support ARM and AARCH64 in CLANGPDB, but I would rather not work on this as I do not have the hardware for validation. Best wishes, Vitaly > 6 февр. 2020 г., в 11:22, Gao, Liming <liming....@intel.com> написал(а): > > 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 <mailto:devel@edk2.groups.io> > <devel@edk2.groups.io <mailto: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 <mailto:liming....@intel.com>> > Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Marvin Häuser > <marvin.haeu...@outlook.com <mailto:marvin.haeu...@outlook.com>>; Laszlo > Ersek <ler...@redhat.com <mailto:ler...@redhat.com>>; Kinney, Michael D > <michael.d.kin...@intel.com <mailto: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 > <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 > > <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 > > <https://edk2.groups.io/g/devel/message/53623> > > Mute This Topic: https://groups.io/mt/70882954/1759384 > > <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 > > <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 (#53860): https://edk2.groups.io/g/devel/message/53860 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] -=-=-=-=-=-=-=-=-=-=-=-
signature.asc
Description: OpenPGP digital signature
