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

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

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.

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