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

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

(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