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

["Liming Gao" <liming.gao@intel.com>]

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. 

Thanks
Liming

>Thanks
>Laszlo