From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web11.11616.1602243434204109297 for ; Fri, 09 Oct 2020 04:37:14 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ACcSwzQQ; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1602243433; 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=wI/ouucPsIxMil81Zfq/tIzyvbRlS1N26RP9Pr81Ric=; b=ACcSwzQQX0x9e2opnJ/IzKDnzgHZPdMW3Ef/CutwNHhNXNIaUtyaALEp1X1MrB5+4vKtU/ P15UhgHjM0KY5dmomciTdE+R1FTw/7zd/AetoPI45CN1GWpJ6pwr18lxAIEKQeOVX5nuPX m4pk1JF4JM/rDzdvXQReZb9gDEdzMHg= 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-5-uOTB2ACLP1aabl_J5Ic5qA-1; Fri, 09 Oct 2020 07:37:11 -0400 X-MC-Unique: uOTB2ACLP1aabl_J5Ic5qA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 75B4A10866A8; Fri, 9 Oct 2020 11:37:09 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-57.ams2.redhat.com [10.36.113.57]) by smtp.corp.redhat.com (Postfix) with ESMTP id 752E76EF6D; Fri, 9 Oct 2020 11:37:07 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 1/2] CryptoPkg/OpensslLib: Add native instruction support for X64 To: "Zurcher, Christopher J" , "devel@edk2.groups.io" , "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> <1ce6123c-7616-30ad-07a0-30b6a5b51dec@redhat.com> From: "Laszlo Ersek" Message-ID: <18e31a95-85e4-9c76-bdc1-5ffa0d32d9cd@redhat.com> Date: Fri, 9 Oct 2020 13:37:06 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 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 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 >> Sent: Thursday, October 1, 2020 05:58 >> To: devel@edk2.groups.io; Zurcher, Christopher J >> ; Yao, Jiewen ; Jiang, >> Guomin >> Cc: Wang, Jian J ; Lu, XiaoyuX ; >> Ard Biesheuvel (ARM address) >> 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 >