From: "Guomin Jiang" <guomin.jiang@intel.com>
To: Laszlo Ersek <lersek@redhat.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>,
"ard.biesheuvel@linaro.org" <ard.biesheuvel@linaro.org>
Cc: "Wang, Jian J" <jian.j.wang@intel.com>,
"Lu, XiaoyuX" <xiaoyux.lu@intel.com>,
"Yao, Jiewen" <jiewen.yao@intel.com>,
Sean Brogan <sean.brogan@microsoft.com>,
"macarl@microsoft.com" <macarl@microsoft.com>
Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.
Date: Tue, 31 Mar 2020 01:40:18 +0000 [thread overview]
Message-ID: <B1F5B0856690F44595CF70FED755C939552611@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <03f18a49-6bd6-e753-e1bf-7e061d13f200@redhat.com>
Hi Laszlo,
Thanks for you spending time review the changes.
And I just want to present how to reproduce the build error.
When build OvmfPkgX64, you can encounter this issue with your local change. The error as below:
TcgPei.lib(AutoGen.obj) : error LNK2001: unresolved external symbol _fltused
c:\sourcecodes\tiano\Build\OvmfX64\DEBUG_VS2019\X64\SecurityPkg\Tcg\TcgPei\TcgPei\DEBUG\TcgPei.dll : fatal error LNK1120: 1 unresolved externals
The build command: build -p OvmfPkg\OvmfPkgX64.dsc -b DEBUG -t VS2019 -a X64 -D TPM_ENABLE
The code changes for reproducing this symptom:
- int GLOBAL_USED _fltused = 1;
+ //int GLOBAL_USED _fltused = 1;
The machine: WIN10
The branch: edk2 master
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, March 31, 2020 3:05 AM
> To: devel@edk2.groups.io; ard.biesheuvel@linaro.org; Jiang, Guomin
> <guomin.jiang@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> <xiaoyux.lu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>; macarl@microsoft.com
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> float.
>
> On 03/30/20 11:02, Ard Biesheuvel wrote:
> > On Mon, 30 Mar 2020 at 10:52, Guomin Jiang <guomin.jiang@intel.com>
> > 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 <jian.j.wang@intel.com>
> >> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> >> Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
> >
> > 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 <https://bugzilla.tianocore.org/show_bug.cgi?id=2596#c0>
> 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 <https://bugzilla.tianocore.org/show_bug.cgi?id=2596#c5>, 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
next prev parent reply other threads:[~2020-03-31 1:40 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-30 8:52 [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float Guomin Jiang
2020-03-30 9:02 ` [edk2-devel] " Ard Biesheuvel
2020-03-30 19:04 ` Laszlo Ersek
2020-03-30 21:27 ` Matthew Carlson
2020-03-30 21:41 ` Ard Biesheuvel
2020-03-31 12:42 ` Laszlo Ersek
2020-03-31 14:36 ` Michael D Kinney
2020-03-31 22:29 ` Laszlo Ersek
2020-03-31 22:57 ` Sean
2020-03-31 23:36 ` Michael D Kinney
2020-04-01 6:42 ` Ard Biesheuvel
2020-04-01 16:38 ` Michael D Kinney
2020-04-14 5:02 ` Ni, Ray
2020-04-14 7:01 ` Guomin Jiang
2020-04-17 8:15 ` Ard Biesheuvel
2020-04-23 2:36 ` Guomin Jiang
[not found] ` <16059D94172527B2.17445@groups.io>
2020-04-23 1:33 ` Guomin Jiang
2020-04-23 3:31 ` Ni, Ray
2020-04-23 4:04 ` Guomin Jiang
2020-04-23 5:49 ` Liming Gao
2020-04-24 5:07 ` Guomin Jiang
2020-04-26 15:32 ` Liming Gao
2020-04-27 2:32 ` Ni, Ray
2020-03-31 1:40 ` Guomin Jiang [this message]
2020-03-31 7:13 ` Ard Biesheuvel
2020-03-31 12:32 ` Laszlo Ersek
2020-03-30 16:55 ` [EXTERNAL] " Bret Barkelew
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B1F5B0856690F44595CF70FED755C939552611@SHSMSX101.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox