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

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

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\SecureBootConfigDxe\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\SecureBootConfigDxe\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               =

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.

Thanks
Laszlo