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 (#56679): https://edk2.groups.io/g/devel/message/56679 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] -=-=-=-=-=-=-=-=-=-=-=-
