From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [216.205.24.74]) by mx.groups.io with SMTP id smtpd.web11.1694.1585595088803126460 for ; Mon, 30 Mar 2020 12:04:49 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=BhJYdBAT; spf=pass (domain: redhat.com, ip: 216.205.24.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585595087; 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=WgwBDPZDcfjlW8zL4C8RglNwJlGrBFwncqpBgzfLMMU=; b=BhJYdBATQT1DwEx3s8N1Vadp+RLnXxgmQHbp8+sPc/9lJ2sAP2ivcNH/CaEwlrAmOZLMQC Psj/tS5qOzpeZYwqBYFm/UHuAwHatv8mu8+/MgFXWbBooVQZ+/H4JGbNT0uufAOPOc8pef op2bgYNCNj9l6Q/+GqufYKCqgaiyf3M= 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-203-TGEzlf6wNyCFH06ZHUnl9g-1; Mon, 30 Mar 2020 15:04:36 -0400 X-MC-Unique: TGEzlf6wNyCFH06ZHUnl9g-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E2E5A13FA; Mon, 30 Mar 2020 19:04:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-191.ams2.redhat.com [10.36.112.191]) by smtp.corp.redhat.com (Postfix) with ESMTP id EC53E100EBB6; Mon, 30 Mar 2020 19:04:32 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float. To: devel@edk2.groups.io, ard.biesheuvel@linaro.org, guomin.jiang@intel.com Cc: Jian J Wang , Xiaoyu Lu , Jiewen Yao , Sean Brogan , macarl@microsoft.com References: <20200330085247.2415-1-guomin.jiang@intel.com> From: "Laszlo Ersek" Message-ID: <03f18a49-6bd6-e753-e1bf-7e061d13f200@redhat.com> Date: Mon, 30 Mar 2020 21:04:32 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 03/30/20 11:02, Ard Biesheuvel wrote: > On Mon, 30 Mar 2020 at 10:52, Guomin Jiang > wrote: >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2596 >> >> OpenSSL requires _fltused to be defined as a constant anywhere >> floating point is used. >> This is to satisfy the linker, however, it is possible to compile a >> module with multiple definitions of fltused. This causes the >> MSVC compiler to fail the build. >> To solve this problem, the FltUsedLib was created that is one spot >> that the global static can exist. >> >> Cc: Jian J Wang >> Cc: Xiaoyu Lu >> Signed-off-by: Guomin Jiang > > Doesn't this affect *every* platform? Isn't there a better way to do > this? E.g., using weak linkage? We already have manually added files under "CryptoPkg/Library/OpensslLib": - ossl_store.c - rand_pool.c - rand_pool_noise.c - rand_pool_noise_tsc.c These files are then referenced in both OpensslLib.inf and OpensslLibCrypto.inf, outside of the "Autogenerated files list". In particular "ossl_store.c" looks like a good example -- it does nothing, just provides a needed symbol. The comment at states that _fltused is an OpenSSL requirement -- so why not move _fltused into the edk2 openssl library instances, and even then, only when building with MSVC? BTW I don't think I understand the actual problem, from the bug report. Matthew wrote, "it is possible to compile a module with multiple definitions of fltused" -- I don't see how (and no example is provided), assuming the module in question already uses IntrinsicLib. In , Sean writes, "the reason we moved to a library to define this symbol was to deal with two libraries within the same module. If both libs defined it then there were problems". -- And I don't understand why *either* of those libraries defined _fltused at all; I think they should have only dependend on InstrinsicLib, which already ensures there's exactly one external definition of _fltused. I just applied the following patch locally: > diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c > index 94fe341bec9d..6ae4c4c82ecf 100644 > --- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c > +++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c > @@ -21,7 +21,9 @@ typedef UINTN size_t; > > /* OpenSSL will use floating point support, and C compiler produces the _fltused > symbol by default. Simply define this symbol here to satisfy the linker. */ > +#if 0 > int GLOBAL_USED _fltused = 1; > +#endif > > /* Sets buffers to a specified character */ > void * memset (void *dest, int ch, size_t count) and witnessed no build failures in my environment. This external definition of "_fltused" comes from historical commit 97f98500c1d4 ("Add CryptoPkg (from UDK2010.UP3)", 2010-11-01), and has been updated (tweaked) once since, in commit 933681b20844 ("CryptoPkg IntrinsicLib: Make _fltused always be used", 2019-10-24), for TianoCore#1603. To me, even the initial addition (from 2010) seems incorrect. Summary: - I don't understand the problem. Please state it clearly, including build platform, target (firmware) platform, toolchain, modules, libraries, and so on. - Assuming the _fltused external definition is needed in fact, I think it should be moved into a C source file referenced by the OpenSSL INF files. This will solve problems where some module depends on the OpensslLib class, but not the IntrinsicLib class. - And, this reference in the OpensslLib INF files should be toolchain specific. Adding a new lib *class* dependency to the CryptLib instances will break *every* platform out there, which is especially incomprehensible given that some platforms don't need _fltused *at all*. Please let's not make this 9+ years old hack worse. Thanks Laszlo