[edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.

Tue Mar 31 01:40:18 UTC 2020

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 at redhat.com]
> Sent: Tuesday, March 31, 2020 3:05 AM
> To: devel at edk2.groups.io; ard.biesheuvel at linaro.org; Jiang, Guomin
> <guomin.jiang at intel.com>
> Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu, XiaoyuX
> <xiaoyux.lu at intel.com>; Yao, Jiewen <jiewen.yao at intel.com>; Sean Brogan
> <sean.brogan at microsoft.com>; macarl at 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 at 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 at intel.com>
> >> Cc: Xiaoyu Lu <xiaoyux.lu at intel.com>
> >> Signed-off-by: Guomin Jiang <guomin.jiang at 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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56704): https://edk2.groups.io/g/devel/message/56704
Mute This Topic: https://groups.io/mt/72648022/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-