Re: [edk2-devel] [PATCH v2 1/2] CryptoPkg/OpensslLib: Add native instruction support for X64

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

On 10/09/20 21:27, Zurcher, Christopher J wrote:
> Here is the error message:
> [...]/OpensslLibX64/OUTPUT/X64/crypto/aes/aesni-mb-x86_64.iii:1746: error: symbol `..imagebase' undefined
> [cut 18 lines]
> [...]/OpensslLibX64/OUTPUT/X64/crypto/aes/aesni-mb-x86_64.iii:1775: error: symbol `..imagebase' undefined
> GNUmakefile:3390: recipe for target '[...]OpensslLibX64/OUTPUT/X64/crypto/aes/aesni-mb-x86_64.obj' failed
> make: *** [[...]/OpensslLibX64/OUTPUT/X64/crypto/aes/aesni-mb-x86_64.obj] Error 1
> 
> The functionality is described here in "7.6.1 win64: Writing Position-Independent Code" and "7.6.2 win64: Structured Exception Handling"
> https://www.nasm.us/xdoc/2.13.02rc3/html/nasmdoc7.html
> 
> The x86_64 implementation in OpenSSL seems to assume that building with NASM guarantees a Windows toolchain and Windows execution environment.

Thank you. I have no more ideas or questions. Please proceed as you
suggested.

Laszlo

>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Friday, October 9, 2020 04:37
>> To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>;
>> devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Jiang, Guomin
>> <guomin.jiang@intel.com>
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>;
>> Ard Biesheuvel (ARM address) <ard.biesheuvel@arm.com>
>> Subject: Re: [edk2-devel] [PATCH v2 1/2] CryptoPkg/OpensslLib: Add native
>> instruction support for X64
>>
>> On 10/08/20 21:56, Zurcher, Christopher J wrote:
>>> Laszlo, thanks for sharing this explanation and history. I have found that
>> in addition to the "common" declaration, OpenSSL's Structured Exception
>> Handling functionality also breaks the GCC build by including "wrt
>> ..imagebase" statements. Since we cannot implement functional changes in the
>> current 1.1.1x versions of OpenSSL, my proposal is to go ahead with this
>> patch only supporting VS and LLVM toolchains for now.
>>
>> Could you include the error message with the "wrt ..imagebase" string?
>>
>> I found the string in "crypto/perlasm/x86_64-xlate.pl" but don't really
>> understand what it's about.
>>
>> I'd just like us to make one attempt to resolve that problem; otherwise
>> personally I'm OK if this new feature is not enabled for GCC at first.
>>
>> Thanks
>> Laszlo
>>
>>>
>>> Thanks,
>>> Christopher Zurcher
>>>
>>>> -----Original Message-----
>>>> From: Laszlo Ersek <lersek@redhat.com>
>>>> Sent: Thursday, October 1, 2020 05:58
>>>> To: devel@edk2.groups.io; Zurcher, Christopher J
>>>> <christopher.j.zurcher@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
>> Jiang,
>>>> Guomin <guomin.jiang@intel.com>
>>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
>> <xiaoyux.lu@intel.com>;
>>>> Ard Biesheuvel (ARM address) <ard.biesheuvel@arm.com>
>>>> Subject: Re: [edk2-devel] [PATCH v2 1/2] CryptoPkg/OpensslLib: Add native
>>>> instruction support for X64
>>>>
>>>> (refreshing Ard's address, comments below)
>>>>
>>>> On 09/29/20 23:08, Zurcher, Christopher J wrote:
>>>>> The GCC build fails with this error:
>>>>>
>>>>> `OPENSSL_ia32cap_P' referenced in section `.text.OPENSSL_cpuid_setup'
>>>>> of /tmp/ccIIRAYs.ltrans20.ltrans.o: defined in discarded section
>>>>> `COMMON' of
>>>>>
>>>>
>> /mnt/c/mssql/tiano/Build/OvmfX64/DEBUG_GCC5/X64/CryptoPkg/Library/OpensslLib/
>>>> OpensslLibX64/OUTPUT/OpensslLibX64.lib(x86_64cpuid.obj)
>>>>>
>>>>> The code in question is here:
>>>>>> section .CRT$XCU rdata align=8
>>>>>>                 DQ      OPENSSL_cpuid_setup
>>>>>>
>>>>>> common  OPENSSL_ia32cap_P 16
>>>>
>>>> For the X64 arch, OPENSSL_cpuid_setup() is implemented in
>>>>
>>>>   CryptoPkg/Library/OpensslLib/openssl/crypto/cryptlib.c
>>>>
>>>> It makes references to:
>>>>
>>>>   extern unsigned int OPENSSL_ia32cap_P[4];
>>>>
>>>> The variable is defined in generated assembly source code.
>>>>
>>>> There seem to be multiple generators (for various assemblers):
>>>>
>>>> (1) crypto/perlasm/x86gas.pl -- likely for the GNU assembler:
>>>>
>>>>>        my $tmp=".comm\t${nmdecor}OPENSSL_ia32cap_P,16";
>>>>
>>>> (2) crypto/perlasm/x86nasm.pl -- likely for NASM:
>>>>
>>>>> ${drdecor}common      ${nmdecor}OPENSSL_ia32cap_P 16
>>>>
>>>> (3) crypto/x86_64cpuid.pl -- likely for... ???
>>>>
>>>>> .comm     OPENSSL_ia32cap_P,16,4
>>>>
>>>> They all put the variable in the "common" section.
>>>>
>>>> Tracking the NASM generator through a number of "git blame" commands,
>>>> I've ended up at historical commit 10e7d6d52650 ("Support for IA-32 SSE2
>>>> instruction set.", 2004-05-06). This commit introduced "OPENSSL_ia32cap"
>>>> at once in the common section -- see "crypto/perlasm/x86unix.pl".
>>>>
>>>> Now, the NASM manual says the following about the common section:
>>>>
>>>>> 6.7. 'COMMON': Defining Common Data Areas
>>>>> =========================================
>>>>>
>>>>> The 'COMMON' directive is used to declare _common variables_.  A common
>>>>> variable is much like a global variable declared in the uninitialized
>>>>> data section, so that
>>>>>
>>>>>      common  intvar  4
>>>>>
>>>>>    is similar in function to
>>>>>
>>>>>      global  intvar
>>>>>      section .bss
>>>>>
>>>>>      intvar  resd    1
>>>>>
>>>>>    The difference is that if more than one module defines the same
>>>>> common variable, then at link time those variables will be _merged_, and
>>>>> references to 'intvar' in all modules will point at the same piece of
>>>>> memory.
>>>>
>>>> The common section is a *really* bad idea for C language projects,
>>>> because if there are multiple external definitions of an object in a
>>>> program, then that should (per C language standard) prevent the
>>>> successful linking of the program, rather than undergo silent definition
>>>> merging.
>>>>
>>>> This has caused actual, inexplicable bugs in edk2 -- identically named,
>>>> but differently sized, and entirely independently inteded, variables
>>>> with external linkage and static storage duration got silently merged,
>>>> rather than breaking the build. In the end, we tracked those down and
>>>> marked them all STATIC. But in order to prevent such nonsense in the
>>>> future, we also forbade the common section altogether. Let me find that
>>>> commit...
>>>>
>>>> Yes, please see 214a3b79417f ("BaseTools GCC: avoid the use of COMMON
>>>> symbols", 2015-12-08).
>>>>
>>>> So, my guess is that this interferes with OpenSSL's placing of
>>>> "OPENSSL_ia32cap_P" in the common section.
>>>>
>>>> Without knowing more, I'd hazard that this is a bug in OpenSSL. Unless
>>>> they have a strong reason for it, I think we should try to contribute a
>>>> patch that removes "common".
>>>>
>>>> The code should provide exactly one definition (in the generated
>>>> assembly source), provide one central (extern) declaration too, in a
>>>> header file, then let all users include the declaration via the header
>>>> file. The object file built from the generated assembly source should be
>>>> linked into each final executable.
>>>>
>>>> For example, "CryptoPkg/Library/OpensslLib/openssl/crypto/cryptlib.c"
>>>> already correctly declares the variable as "extern".
>>>>
>>>> Otherwise, as last resort, I guess we could attempt working it around by
>>>> adding back "-fcommon" to the OpensslLib build flags. :/
>>>>
>>>> Thanks,
>>>> Laszlo
>>>
>>
>>
>>
>> 
>>
>