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:ler...@redhat.com] > Sent: Tuesday, March 31, 2020 3:05 AM > To: devel@edk2.groups.io; ard.biesheu...@linaro.org; Jiang, Guomin > <guomin.ji...@intel.com> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX > <xiaoyux...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Sean Brogan > <sean.bro...@microsoft.com>; mac...@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.ji...@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.w...@intel.com> > >> Cc: Xiaoyu Lu <xiaoyux...@intel.com> > >> Signed-off-by: Guomin Jiang <guomin.ji...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
