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

["Laszlo Ersek" <lersek@redhat.com>]

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