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

["Zurcher, Christopher J" <christopher.j.zurcher@intel.com>]

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.

Thanks,
Christopher Zurcher

> -----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
> >
> 
> 
> 
> 
>