From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web12.11964.1601557119243327880 for ; Thu, 01 Oct 2020 05:58:39 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=TSVhPoh6; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1601557118; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Qj2LktLoKx7qh8tp9B8LGxmOYpCy3LlUtnxxKaTjBDI=; b=TSVhPoh6SwXADu7MfwRf8UpzTAznG3POhj//X/nIx3M3l4oYy6oif7rSgRzX1bYRRu174F g3smsD6hkSwAuiCBmtL5MvIxhlEpL++XO0ZdNm/JUKdJM518GbYXLg4CoonhdHdz5dID7M 2wJQkovV29PSww+wqe6nqINvUhQLB9Y= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-76-uAUmjxHBOyCPQdAvFNhrgg-1; Thu, 01 Oct 2020 08:58:35 -0400 X-MC-Unique: uAUmjxHBOyCPQdAvFNhrgg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 954BE1017886; Thu, 1 Oct 2020 12:58:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-110.ams2.redhat.com [10.36.113.110]) by smtp.corp.redhat.com (Postfix) with ESMTP id B4C0960BF1; Thu, 1 Oct 2020 12:58:11 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 1/2] CryptoPkg/OpensslLib: Add native instruction support for X64 To: devel@edk2.groups.io, christopher.j.zurcher@intel.com, "Yao, Jiewen" , "Jiang, Guomin" Cc: "Wang, Jian J" , "Lu, XiaoyuX" , "Ard Biesheuvel (ARM address)" References: <20200804002429.3897-1-christopher.j.zurcher@intel.com> <20200804002429.3897-2-christopher.j.zurcher@intel.com> <162C7E6ED8CEF542.12673@groups.io> From: "Laszlo Ersek" Message-ID: <1ce6123c-7616-30ad-07a0-30b6a5b51dec@redhat.com> Date: Thu, 1 Oct 2020 14:58:10 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit (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