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


Guomin Jiang
 

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

Join devel@edk2.groups.io to automatically receive all group messages.