Re: [edk2-devel] [Patch 11/12] OvmfPkg: Enable CLANG9 tool chain -

["Laszlo Ersek" <lersek@redhat.com>]

On 10/11/19 03:47, Liming Gao wrote:
> Laszlo:
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, October 11, 2019 12:43 AM
>> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Andrew Fish
>> <afish@apple.com>
>> Cc: Justen, Jordan L <jordan.l.justen@intel.com>
>> Subject: Re: [edk2-devel] [Patch 11/12] OvmfPkg: Enable CLANG9 tool chain -
>>
>> Hi Liming,
>>
>> On 10/10/19 14:18, Liming Gao wrote:
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
>> Ersek
>>>> Sent: Thursday, October 10, 2019 3:35 PM
>>>> To: Andrew Fish <afish@apple.com>; Gao, Liming <liming.gao@intel.com>
>>>> Cc: devel@edk2.groups.io
>>>> Subject: Re: [edk2-devel] [Patch 11/12] OvmfPkg: Enable CLANG9 tool
>> chain -
>>>>
>>>> Hi Andrew,
>>>>
>>>> On 10/09/19 18:22, Andrew Fish wrote:
>>>>
>>>>> I thought the thing we were discussing was compiler flags.
>>>>> Specifically -mno-mmx -mno-sse. It seems to me if OVMF requires
>>>>> -mno-mmx -mno-sse then it is a bug in the tools_def.txt definition
>>>>> for those compilers?  As far as I can tell -mno-implicit-float should
>>>>> prevent the optimizer from using floating point. The -mno-mmx
>>>>> -mno-sse flags most change how floating point code gets compiled [1].
>>>>> it looks like -mno-mmx -mno-sse just down grade floating point
>>>>> instructions that get used. Thus it seems like either we have some
>>>>> code doing float and that code should set -mno-mmx -mno-sse, or the
>>>>> -mno-mmx -mno-sse should be set generically.
>>>>
>>>> The origin of those build flags in OVMF is commit 4a8266f570ef
>>>> ("OvmfPkg: Work around issue seen with kvm + grub2 (efi)", 2010-12-31):
>>>>
>>>>     OvmfPkg: Work around issue seen with kvm + grub2 (efi)
>>>>
>>>>     When OVMF is run with kvm and grub2 (efi), an exception
>>>>     occurs when mmx/sse registers are accessed.
>>>>
>>>>     As a work around, this change eliminates firmware usage
>>>>     of these register types.
>>>>
>>>>     First, only the BaseMemoryLib implementation
>>>>     MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
>>>>     is used.
>>>>
>>>>     Second, the GCC compiler is passes the additional
>>>>     '-mno-mmx -mno-sse' parameters.
>>>>
>>>> The eternal problem with this kind of workaround is that we never know
>>>> when it becomes safe to remove.
>>>>
>>>> It would be nice to understand the exact details around the problem.
>>>> Given that the commit is from 2010, I assume the issue is difficult to
>>>> reproduce with recent components (KVM, grub2). On the other hand, if
>> we
>>>> just remove the flags (which we should, in a perfect world), someone
>>>> could report a bug in three months: "my host distro upgraded the OVMF
>>>> package to the next edk2-stableYYYYMM tag, and now my virtual machine,
>>>> which runs a distro from 2009, no longer boots". (We've seen similar
>>>> reports before.)
>>>
>>> Yes. I agree it is hard to decide to remove this option, because we don't
>> know its impact.
>>> With the option -mno-mmx -mno-sse, it will cause CLANG9 linker failure like
>> below. So, I say
>>> this patch is to support the different linker.
>>>
>>> 0.      Program arguments: C:\Program Files\LLVM\bin\lld-link.EXE
>> /OUT:d:\allpkg\edk2\Build\Ovmf3264\DEBUG_CLANG9\X64\Se
>>>
>> curityPkg\VariableAuthenticated\SecureBootConfigDxe\SecureBootConfigDx
>> e\DEBUG\SecureBootConfigDxe.dll /NOLOGO /NODEFAULT
>>> LIB /IGNORE:4001 /OPT:REF /OPT:ICF=10 /ALIGN:32 /FILEALIGN:32
>> /SECTION:.xdata,D /SECTION:.pdata,D /Machine:X64 /DLL /ENT
>>> RY:_ModuleEntryPoint /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER
>> /SAFESEH:NO /BASE:0 /DEBUG:GHASH /lldmap @d:\allpkg\edk2\Build\O
>>>
>> vmf3264\DEBUG_CLANG9\X64\SecurityPkg\VariableAuthenticated\SecureBo
>> otConfigDxe\SecureBootConfigDxe\OUTPUT\static_library
>>> _files.lst
>>> 1.      Running pass 'Function Pass Manager' on module 'ld-temp.o'.
>>> 2.      Running pass 'X86 FP Stackifier' on function '@drbg_add'
>>>  #0 0x00007ff696bad7f8 C:\Program Files\LLVM\bin\lld-link.EXE 0x148d7f8
>> C:\Program Files\LLVM\bin\lld-link.EXE 0x142ba7f
>>
>> can you please try the following:
>>
>> (a) replace this patch with two patches, as follows
>>
>> (b) in the first patch, only rework
>>
>> !if $(TOOL_CHAIN_TAG) != "XCODE5"
>>  GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
>> !endif
>>
>> into:
>>
>>  GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
>>  XCODE:*_*_*_CC_FLAGS                 =
>>
>> (c) in the second patch, append:
>>
>>  CLANGPE:*_*_*_CC_FLAGS               =
>>
> This way clear CLANG9 CC flags. It forbids CLANG9 to inherit other GCC flags. 
> Below flags are not applied into CLANG9. The purpose is not to include 
> specific -mno-mmx -mno-sse option, and still inherit other GCC flags. 
> So, I have to use $(TOOL_CHAIN_TAG) check. 
> 
>   MSFT:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
>   INTEL:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
>   GCC:*_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
> 
>> and also introduce
>>
>>  CLANGPE: *_*_*_DLINK_FLAGS = /ALIGN:4096
>>
>>
>> If (b) preserves the intended effect for the XCODE5 toolchain, and (c)
>> does the right thing for the CLANG9 toolchain as well, then that
>> approach would be preferable to the currently proposed patch#11.
>>
>> If the above does *not* work, then I'm fine with the currently proposed
>> patch, but then please update the commit message; it should say that
>> "-mno-mmx -mno-sse" is *excluded* for CLANG9.
>>
> Yes. I will update the commit message. 

OK. With the commit message updated, for this patch:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

For later, please consider the GENUINE_GCC suggestion (for
BUILDRULEFAMILY) in my other message in the present thread:

http://mid.mail-archive.com/43b303eb-fd74-7480-aa6d-6907300af3e0@redhat.com
https://edk2.groups.io/g/devel/message/48808

If you think that the "BaseTools/Conf/tools_def.template" patch that I
suggested there is acceptable, then I would like to update the OVMF DSC
files (later) as follows:

  GENUINE_GCC:*_*_*_CC_FLAGS              = -mno-mmx -mno-sse
  CLANG:*_*_*_CC_FLAGS                    = -mno-mmx -mno-sse
  /* say nothing about XCODE */
  /* say nothing CLANGPE */

Thanks
Laszlo