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

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

[Guomin Jiang]

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>
---
 CryptoPkg/CryptoPkg.dsc                       |  2 ++
 .../Library/BaseCryptLib/BaseCryptLib.inf     |  1 +
 .../Library/BaseCryptLib/PeiCryptLib.inf      |  1 +
 .../Library/BaseCryptLib/RuntimeCryptLib.inf  |  1 +
 .../Library/BaseCryptLib/SmmCryptLib.inf      |  1 +
 CryptoPkg/Library/FltUsedLib/FltUsedLib.c     | 14 ++++++++
 CryptoPkg/Library/FltUsedLib/FltUsedLib.inf   | 33 +++++++++++++++++++
 CryptoPkg/Library/TlsLib/TlsLib.inf           |  1 +
 8 files changed, 54 insertions(+)
 create mode 100644 CryptoPkg/Library/FltUsedLib/FltUsedLib.c
 create mode 100644 CryptoPkg/Library/FltUsedLib/FltUsedLib.inf

diff --git a/CryptoPkg/CryptoPkg.dsc b/CryptoPkg/CryptoPkg.dsc
index 4cb37b1349..e9854087b5 100644
--- a/CryptoPkg/CryptoPkg.dsc
+++ b/CryptoPkg/CryptoPkg.dsc
@@ -97,6 +97,7 @@
   IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf                                          #???
   OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
   IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
+  FltUsedLib|CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
   SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
 
 [LibraryClasses.ARM]
@@ -232,6 +233,7 @@
   CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
   CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
   CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
+  CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
   CryptoPkg/Library/TlsLib/TlsLib.inf
   CryptoPkg/Library/TlsLibNull/TlsLibNull.inf
   CryptoPkg/Library/OpensslLib/OpensslLib.inf
diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
index 1bbe4f435a..c82e703aa0 100644
--- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -84,6 +84,7 @@
   DebugLib
   OpensslLib
   IntrinsicLib
+  FltUsedLib
   PrintLib
 
 #
diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
index c836c257f8..342aad008c 100644
--- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
@@ -78,6 +78,7 @@
   DebugLib
   OpensslLib
   IntrinsicLib
+  FltUsedLib
 
 #
 # Remove these [BuildOptions] after this library is cleaned up
diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
index bff308a4f5..dcf209d7f5 100644
--- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
@@ -89,6 +89,7 @@
   DebugLib
   OpensslLib
   IntrinsicLib
+  FltUsedLib
   PrintLib
 
 #
diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
index cc0b65fd25..7fdaaa48a6 100644
--- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
@@ -88,6 +88,7 @@
   MemoryAllocationLib
   OpensslLib
   IntrinsicLib
+  FltUsedLib
   PrintLib
 
 #
diff --git a/CryptoPkg/Library/FltUsedLib/FltUsedLib.c b/CryptoPkg/Library/FltUsedLib/FltUsedLib.c
new file mode 100644
index 0000000000..8f2487ea13
--- /dev/null
+++ b/CryptoPkg/Library/FltUsedLib/FltUsedLib.c
@@ -0,0 +1,14 @@
+/** @file
+  Need include this when use floats.
+
+  Copyright (C) Microsoft Corporation. All rights reserved.<BR>
+  Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+//
+// You need to include this to let the compiler know we are going to use
+// floating point
+//
+int _fltused = 0x9875;
diff --git a/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf b/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
new file mode 100644
index 0000000000..8d67eadfa5
--- /dev/null
+++ b/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
@@ -0,0 +1,33 @@
+## @file
+# It is depent on when using floats
+#
+# Copyright (C) Microsoft Corporation. All rights reserved.<BR>
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = FltUsedLib
+  FILE_GUID                      = C004F180-9FE2-4D2B-8318-BADC2A231774
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = FltUsedLib
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+#  VALID_ARCHITECTURES           = IA32 X64 AARCH64
+#
+
+[Sources]
+  FltUsedLib.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+
+[BuildOptions]
+  # Disable GL due to linker error LNK1237
+  # https://docs.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-error-lnk1237?view=vs-2017
+  MSFT:*_*_*_CC_FLAGS = /GL-
diff --git a/CryptoPkg/Library/TlsLib/TlsLib.inf b/CryptoPkg/Library/TlsLib/TlsLib.inf
index 2f3ce695c3..febbdf5149 100644
--- a/CryptoPkg/Library/TlsLib/TlsLib.inf
+++ b/CryptoPkg/Library/TlsLib/TlsLib.inf
@@ -37,6 +37,7 @@
   BaseMemoryLib
   DebugLib
   IntrinsicLib
+  FltUsedLib
   MemoryAllocationLib
   OpensslLib
   SafeIntLib
-- 
2.25.1.windows.1

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

[Ard Biesheuvel]

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?

> ---
>  CryptoPkg/CryptoPkg.dsc                       |  2 ++
>  .../Library/BaseCryptLib/BaseCryptLib.inf     |  1 +
>  .../Library/BaseCryptLib/PeiCryptLib.inf      |  1 +
>  .../Library/BaseCryptLib/RuntimeCryptLib.inf  |  1 +
>  .../Library/BaseCryptLib/SmmCryptLib.inf      |  1 +
>  CryptoPkg/Library/FltUsedLib/FltUsedLib.c     | 14 ++++++++
>  CryptoPkg/Library/FltUsedLib/FltUsedLib.inf   | 33 +++++++++++++++++++
>  CryptoPkg/Library/TlsLib/TlsLib.inf           |  1 +
>  8 files changed, 54 insertions(+)
>  create mode 100644 CryptoPkg/Library/FltUsedLib/FltUsedLib.c
>  create mode 100644 CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
>
> diff --git a/CryptoPkg/CryptoPkg.dsc b/CryptoPkg/CryptoPkg.dsc
> index 4cb37b1349..e9854087b5 100644
> --- a/CryptoPkg/CryptoPkg.dsc
> +++ b/CryptoPkg/CryptoPkg.dsc
> @@ -97,6 +97,7 @@
>    IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf                                          #???
>    OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
>    IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> +  FltUsedLib|CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
>    SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
>
>  [LibraryClasses.ARM]
> @@ -232,6 +233,7 @@
>    CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>    CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
>    CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> +  CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
>    CryptoPkg/Library/TlsLib/TlsLib.inf
>    CryptoPkg/Library/TlsLibNull/TlsLibNull.inf
>    CryptoPkg/Library/OpensslLib/OpensslLib.inf
> diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> index 1bbe4f435a..c82e703aa0 100644
> --- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> @@ -84,6 +84,7 @@
>    DebugLib
>    OpensslLib
>    IntrinsicLib
> +  FltUsedLib
>    PrintLib
>
>  #
> diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> index c836c257f8..342aad008c 100644
> --- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> @@ -78,6 +78,7 @@
>    DebugLib
>    OpensslLib
>    IntrinsicLib
> +  FltUsedLib
>
>  #
>  # Remove these [BuildOptions] after this library is cleaned up
> diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> index bff308a4f5..dcf209d7f5 100644
> --- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> @@ -89,6 +89,7 @@
>    DebugLib
>    OpensslLib
>    IntrinsicLib
> +  FltUsedLib
>    PrintLib
>
>  #
> diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> index cc0b65fd25..7fdaaa48a6 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> @@ -88,6 +88,7 @@
>    MemoryAllocationLib
>    OpensslLib
>    IntrinsicLib
> +  FltUsedLib
>    PrintLib
>
>  #
> diff --git a/CryptoPkg/Library/FltUsedLib/FltUsedLib.c b/CryptoPkg/Library/FltUsedLib/FltUsedLib.c
> new file mode 100644
> index 0000000000..8f2487ea13
> --- /dev/null
> +++ b/CryptoPkg/Library/FltUsedLib/FltUsedLib.c
> @@ -0,0 +1,14 @@
> +/** @file
> +  Need include this when use floats.
> +
> +  Copyright (C) Microsoft Corporation. All rights reserved.<BR>
> +  Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +//
> +// You need to include this to let the compiler know we are going to use
> +// floating point
> +//
> +int _fltused = 0x9875;
> diff --git a/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf b/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
> new file mode 100644
> index 0000000000..8d67eadfa5
> --- /dev/null
> +++ b/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
> @@ -0,0 +1,33 @@
> +## @file
> +# It is depent on when using floats
> +#
> +# Copyright (C) Microsoft Corporation. All rights reserved.<BR>
> +# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = FltUsedLib
> +  FILE_GUID                      = C004F180-9FE2-4D2B-8318-BADC2A231774
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = FltUsedLib
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64 AARCH64
> +#
> +
> +[Sources]
> +  FltUsedLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +
> +[BuildOptions]
> +  # Disable GL due to linker error LNK1237
> +  # https://docs.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-error-lnk1237?view=vs-2017
> +  MSFT:*_*_*_CC_FLAGS = /GL-
> diff --git a/CryptoPkg/Library/TlsLib/TlsLib.inf b/CryptoPkg/Library/TlsLib/TlsLib.inf
> index 2f3ce695c3..febbdf5149 100644
> --- a/CryptoPkg/Library/TlsLib/TlsLib.inf
> +++ b/CryptoPkg/Library/TlsLib/TlsLib.inf
> @@ -37,6 +37,7 @@
>    BaseMemoryLib
>    DebugLib
>    IntrinsicLib
> +  FltUsedLib
>    MemoryAllocationLib
>    OpensslLib
>    SafeIntLib
> --
> 2.25.1.windows.1
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#56613): https://edk2.groups.io/g/devel/message/56613
> Mute This Topic: https://groups.io/mt/72648022/1761188
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [ard.biesheuvel@linaro.org]
> ------------
>

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

[Laszlo Ersek]

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

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

[Matthew Carlson]

[-- Attachment #1: Type: text/plain, Size: 660 bytes --]

So it's not required by OpenSSL, it's required by the compiler whenever floating point is used, which can be in multiple places. For example, this is used in mu_plus (the Microsoft UEFI value add to EDK2) by the OnScreenKeyboard driver as well as the UiToolKit driver. If you happen to use BaseCryptLib or the intrinsic in a driver that happens to need crypto as well, it can break due to multiple.

I do agree, this only applies to MSVC and requiring every platform to add a line in their DSC would be a situation I would prefer to avoid if possible. Is there a way to specify a library dependency only if a toolchain is being used?
--
- Matthew Carlson

[-- Attachment #2: Type: text/html, Size: 677 bytes --]

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

[Ard Biesheuvel]

On Mon, 30 Mar 2020 at 23:29, Matthew Carlson via Groups.Io
<macarl=microsoft.com@groups.io> wrote:
>
> So it's not required by OpenSSL, it's required by the compiler whenever floating point is used, which can be in multiple places. For example, this is used in mu_plus (the Microsoft UEFI value add to EDK2) by the OnScreenKeyboard driver as well as the UiToolKit driver. If you happen to use BaseCryptLib or the intrinsic in a driver that happens to need crypto as well, it can break due to multiple.
>
> I do agree, this only applies to MSVC and requiring every platform to add a line in their DSC would be a situation I would prefer to avoid if possible. Is there a way to specify a library dependency only if a toolchain is being used?

Yes, so this either belongs in one of the IntrinsicsLibs we have, or
in the various Entrypoint libraries we have for PEIMs, DXEs, etc.

However, given that we are talking about static libraries here, adding
a source file that *only* defines __fltused (and nothing else) to any
library should also work, as the resulting object file will only be
incorporated by the linker if it is needed to satisfy a symbol
dependency, and so it can never cause a conflict if it is the only
symbol in the object.

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

[Laszlo Ersek]

On 03/30/20 23:41, Ard Biesheuvel wrote:
> On Mon, 30 Mar 2020 at 23:29, Matthew Carlson via Groups.Io
> <macarl=microsoft.com@groups.io> wrote:
>>
>> So it's not required by OpenSSL, it's required by the compiler whenever floating point is used, which can be in multiple places. For example, this is used in mu_plus (the Microsoft UEFI value add to EDK2) by the OnScreenKeyboard driver as well as the UiToolKit driver.

OK, so this is the part that was not obvious to me. This is basically
code that exists on top of edk2 (consumes edk2) *AND* uses floating
point *internally*.

> If you happen to use BaseCryptLib or the intrinsic in a driver that happens to need crypto as well, it can break due to multiple.
>>
>> I do agree, this only applies to MSVC and requiring every platform to add a line in their DSC would be a situation I would prefer to avoid if possible. Is there a way to specify a library dependency only if a toolchain is being used?
> 
> Yes, so this either belongs in one of the IntrinsicsLibs we have, or
> in the various Entrypoint libraries we have for PEIMs, DXEs, etc.
> 
> However, given that we are talking about static libraries here, adding
> a source file that *only* defines __fltused (and nothing else) to any
> library should also work, as the resulting object file will only be
> incorporated by the linker if it is needed to satisfy a symbol
> dependency, and so it can never cause a conflict if it is the only
> symbol in the object.

IMO: if edk2 intends to support out-of-tree modules that themselves
utilize floating point, *with or without* a dependency on OpensslLib,
then we should add Ard's suggestion:

 [Sources]
+  FloatUsed.c         | MSFT

to some of the "most core" edk2 library instances, i.e. those that *all*
modules of the given module type inevitably depend on. The entry point
libs look like a great idea to me.

That should link the _fltused external definition exactly once into all
modules built with MSVC, regardless of whether each such module uses
floating point or not.

Thanks
Laszlo

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

[Michael D Kinney]

ARM and AARCH64 have a compiler intrinsic lib that is linked against all modules.

[LibraryClasses.ARM, LibraryClasses.AARCH64]
  #
  # It is not possible to prevent ARM compiler calls to generic intrinsic functions.
  # This library provides the instrinsic functions generated by a given compiler.
  # [LibraryClasses.ARM] and NULL mean link this library into all ARM images.
  #
  NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf

Can we use this same technique for IA32/X64 VS builds?

Mike


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Laszlo Ersek
> Sent: Tuesday, March 31, 2020 5:42 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-
> devel-groups-io <devel@edk2.groups.io>;
> macarl@microsoft.com
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
> Add FltUsedLib for float.
> 
> On 03/30/20 23:41, Ard Biesheuvel wrote:
> > On Mon, 30 Mar 2020 at 23:29, Matthew Carlson via
> Groups.Io
> > <macarl=microsoft.com@groups.io> wrote:
> >>
> >> So it's not required by OpenSSL, it's required by the
> compiler whenever floating point is used, which can be in
> multiple places. For example, this is used in mu_plus
> (the Microsoft UEFI value add to EDK2) by the
> OnScreenKeyboard driver as well as the UiToolKit driver.
> 
> OK, so this is the part that was not obvious to me. This
> is basically
> code that exists on top of edk2 (consumes edk2) *AND*
> uses floating
> point *internally*.
> 
> > If you happen to use BaseCryptLib or the intrinsic in a
> driver that happens to need crypto as well, it can break
> due to multiple.
> >>
> >> I do agree, this only applies to MSVC and requiring
> every platform to add a line in their DSC would be a
> situation I would prefer to avoid if possible. Is there a
> way to specify a library dependency only if a toolchain
> is being used?
> >
> > Yes, so this either belongs in one of the
> IntrinsicsLibs we have, or
> > in the various Entrypoint libraries we have for PEIMs,
> DXEs, etc.
> >
> > However, given that we are talking about static
> libraries here, adding
> > a source file that *only* defines __fltused (and
> nothing else) to any
> > library should also work, as the resulting object file
> will only be
> > incorporated by the linker if it is needed to satisfy a
> symbol
> > dependency, and so it can never cause a conflict if it
> is the only
> > symbol in the object.
> 
> IMO: if edk2 intends to support out-of-tree modules that
> themselves
> utilize floating point, *with or without* a dependency on
> OpensslLib,
> then we should add Ard's suggestion:
> 
>  [Sources]
> +  FloatUsed.c         | MSFT
> 
> to some of the "most core" edk2 library instances, i.e.
> those that *all*
> modules of the given module type inevitably depend on.
> The entry point
> libs look like a great idea to me.
> 
> That should link the _fltused external definition exactly
> once into all
> modules built with MSVC, regardless of whether each such
> module uses
> floating point or not.
> 
> Thanks
> Laszlo
> 
> 
> 

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

[Laszlo Ersek]

On 03/31/20 16:36, Kinney, Michael D wrote:
> ARM and AARCH64 have a compiler intrinsic lib that is linked against all modules.
> 
> [LibraryClasses.ARM, LibraryClasses.AARCH64]
>   #
>   # It is not possible to prevent ARM compiler calls to generic intrinsic functions.
>   # This library provides the instrinsic functions generated by a given compiler.
>   # [LibraryClasses.ARM] and NULL mean link this library into all ARM images.
>   #
>   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> 
> Can we use this same technique for IA32/X64

It's not a problem for in-tree platforms (I do hope the edk2 patch
series will include the patch for OvmfPkg), but it will require all
out-of-tree platforms to be updated.

> VS builds?

Yes; I think the library instance should consist of one totally empty C
file, and an |MSFT specific C file providing the external definition of
_fltused. When building for GCC, the lib instance should compile to an
empty object (archive) file.

In my opinion, of course.

Thanks
Laszlo

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

[Sean]

[-- Attachment #1: Type: text/plain, Size: 266 bytes --]

Does anyone know off hand if defining this and enabling floating point has any negative side effects if you don't need it?  Size? Optimization? Other?   That is my only concern for enabling in all modules which is why the initial proposal was for a new library.

[-- Attachment #2: Type: text/html, Size: 290 bytes --]

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

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

Hi Sean,

This lib defines a global variable that is referenced when a compiler detects use of float/double types.  If the global is not referenced, then it should be optimized away, so the size impact should be zero.  That can be verified as part of the review of this feature.

Mike



From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean via Groups.Io
Sent: Tuesday, March 31, 2020 3:58 PM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.

Does anyone know off hand if defining this and enabling floating point has any negative side effects if you don't need it?  Size? Optimization? Other?   That is my only concern for enabling in all modules which is why the initial proposal was for a new library.


[-- Attachment #2: Type: text/html, Size: 40431 bytes --]

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

[Ard Biesheuvel]

On Tue, 31 Mar 2020 at 16:36, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> ARM and AARCH64 have a compiler intrinsic lib that is linked against all modules.
>
> [LibraryClasses.ARM, LibraryClasses.AARCH64]
>   #
>   # It is not possible to prevent ARM compiler calls to generic intrinsic functions.
>   # This library provides the instrinsic functions generated by a given compiler.
>   # [LibraryClasses.ARM] and NULL mean link this library into all ARM images.
>   #
>   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
>
> Can we use this same technique for IA32/X64 VS builds?
>

In my opinion, having these intrinsics libraries added via NULL
library class resolution was a mistake to begin with.

Every component that we build incorporates some kind of startup code
library that defines the _ModuleEntryPoint symbol, and it would be
much better to make those libraries include an IntrinsicsLibrary
dependency that can be satisfied by arch specific versions that also
encapsulate the toolchain dependencies (such as this _fltused symbol,
or memcpy/memset on ARM/GCC).

On another note, it is still deeply disappointing that we need to jump
through all of these hoops because the *only* single use of floating
point in OpenSSL is the entropy estimate of an RNG, which is in the
range of 0..1023 to begin with [IIRC). Remember that we also have a
softfloat submodule for 32-bit ARM, for the same stupid reason. If we
could stop pulling in that part of the code (or replace it with an
UINT16 when building for the UEFI target), the whole floating point
issue would mostly go away AFAICT.

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

[Michael D Kinney]

Hi Ard,

I think adding a dependency in the module entry point
libs is also good way to guarantee an intrinsic lib
is available.  I agree that it can provide module type
and arch specific mappings.  The NULL lib instance can
do the same if the NULL instance is listed in the 
module/arch specific library classes sections. a few
example section names.

[LibraryClasses.ARM.PEIM, LibraryClasses.AARCH64.PEIM]

[LibraryClasses.IA32.UEFI_DRIVER]

[LibraryClasses.X64.DXE_DRIVER]

So either way, the DSC files need to provide the
library mapping.  The only difference between these
2 approaches is that adding a dependency to the
module entry point libs uses a formally defined
library class name for the intrinsic functions vs.
the un-named NULL library instance.  A formally
defined library class name is better supported for
things like unit tests from the UnitTestFrameworkPkg.

The fltused issue would not go away of we removed the 
float usage for OpenSSL.  One or more other libs or the
module could use float/double types and this issue
would pop up again.  I do agree it would be cleaner
if we could use OpenSSL without floating point.

I think adding an intrinsic lib for IA32/X64 for VS20xx
generation of intrinsic functions would address fltused
and other common C implementation styles that generate
intrinsic functions (e.g. 64-bit int math on 32-bit,
structure variable assignments, and loops that fill a buffer
with a const value) by VS20xx compilers.  An intrinsic
lib for GCC would also help with these same common C
implementation styles that generate intrinsic functions.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Ard Biesheuvel
> Sent: Tuesday, March 31, 2020 11:43 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; lersek@redhat.com;
> macarl@microsoft.com
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
> Add FltUsedLib for float.
> 
> On Tue, 31 Mar 2020 at 16:36, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> > ARM and AARCH64 have a compiler intrinsic lib that is
> linked against all modules.
> >
> > [LibraryClasses.ARM, LibraryClasses.AARCH64]
> >   #
> >   # It is not possible to prevent ARM compiler calls to
> generic intrinsic functions.
> >   # This library provides the instrinsic functions
> generated by a given compiler.
> >   # [LibraryClasses.ARM] and NULL mean link this
> library into all ARM images.
> >   #
> >
> NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrins
> icsLib.inf
> >
> > Can we use this same technique for IA32/X64 VS builds?
> >
> 
> In my opinion, having these intrinsics libraries added
> via NULL
> library class resolution was a mistake to begin with.
> 
> Every component that we build incorporates some kind of
> startup code
> library that defines the _ModuleEntryPoint symbol, and it
> would be
> much better to make those libraries include an
> IntrinsicsLibrary
> dependency that can be satisfied by arch specific
> versions that also
> encapsulate the toolchain dependencies (such as this
> _fltused symbol,
> or memcpy/memset on ARM/GCC).
> 
> On another note, it is still deeply disappointing that we
> need to jump
> through all of these hoops because the *only* single use
> of floating
> point in OpenSSL is the entropy estimate of an RNG, which
> is in the
> range of 0..1023 to begin with [IIRC). Remember that we
> also have a
> softfloat submodule for 32-bit ARM, for the same stupid
> reason. If we
> could stop pulling in that part of the code (or replace
> it with an
> UINT16 when building for the UEFI target), the whole
> floating point
> issue would mostly go away AFAICT.
> 
> 

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

[Ni, Ray]

UEFI spec allows using float operation so asking module developers avoid using
float may not make sense. Even UefiCpuPkg\Library\BaseUefiCpuLib\ provides
routine to initialize float support for X86.

Given ARM already uses NULL library class mechanism to supply the compiler stub,
I prefer X86 aligns to the ARM style.

The only unsure thing is the size impact when a module is not using float. We expect
there is no size impact when a module is not using float.

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D
> Kinney
> Sent: Thursday, April 2, 2020 12:38 AM
> To: devel@edk2.groups.io; ard.biesheuvel@linaro.org; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: lersek@redhat.com; macarl@microsoft.com
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> float.
> 
> Hi Ard,
> 
> I think adding a dependency in the module entry point
> libs is also good way to guarantee an intrinsic lib
> is available.  I agree that it can provide module type
> and arch specific mappings.  The NULL lib instance can
> do the same if the NULL instance is listed in the
> module/arch specific library classes sections. a few
> example section names.
> 
> [LibraryClasses.ARM.PEIM, LibraryClasses.AARCH64.PEIM]
> 
> [LibraryClasses.IA32.UEFI_DRIVER]
> 
> [LibraryClasses.X64.DXE_DRIVER]
> 
> So either way, the DSC files need to provide the
> library mapping.  The only difference between these
> 2 approaches is that adding a dependency to the
> module entry point libs uses a formally defined
> library class name for the intrinsic functions vs.
> the un-named NULL library instance.  A formally
> defined library class name is better supported for
> things like unit tests from the UnitTestFrameworkPkg.
> 
> The fltused issue would not go away of we removed the
> float usage for OpenSSL.  One or more other libs or the
> module could use float/double types and this issue
> would pop up again.  I do agree it would be cleaner
> if we could use OpenSSL without floating point.
> 
> I think adding an intrinsic lib for IA32/X64 for VS20xx
> generation of intrinsic functions would address fltused
> and other common C implementation styles that generate
> intrinsic functions (e.g. 64-bit int math on 32-bit,
> structure variable assignments, and loops that fill a buffer
> with a const value) by VS20xx compilers.  An intrinsic
> lib for GCC would also help with these same common C
> implementation styles that generate intrinsic functions.
> 
> Mike
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On
> > Behalf Of Ard Biesheuvel
> > Sent: Tuesday, March 31, 2020 11:43 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: devel@edk2.groups.io; lersek@redhat.com;
> > macarl@microsoft.com
> > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
> > Add FltUsedLib for float.
> >
> > On Tue, 31 Mar 2020 at 16:36, Kinney, Michael D
> > <michael.d.kinney@intel.com> wrote:
> > >
> > > ARM and AARCH64 have a compiler intrinsic lib that is
> > linked against all modules.
> > >
> > > [LibraryClasses.ARM, LibraryClasses.AARCH64]
> > >   #
> > >   # It is not possible to prevent ARM compiler calls to
> > generic intrinsic functions.
> > >   # This library provides the instrinsic functions
> > generated by a given compiler.
> > >   # [LibraryClasses.ARM] and NULL mean link this
> > library into all ARM images.
> > >   #
> > >
> > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrins
> > icsLib.inf
> > >
> > > Can we use this same technique for IA32/X64 VS builds?
> > >
> >
> > In my opinion, having these intrinsics libraries added
> > via NULL
> > library class resolution was a mistake to begin with.
> >
> > Every component that we build incorporates some kind of
> > startup code
> > library that defines the _ModuleEntryPoint symbol, and it
> > would be
> > much better to make those libraries include an
> > IntrinsicsLibrary
> > dependency that can be satisfied by arch specific
> > versions that also
> > encapsulate the toolchain dependencies (such as this
> > _fltused symbol,
> > or memcpy/memset on ARM/GCC).
> >
> > On another note, it is still deeply disappointing that we
> > need to jump
> > through all of these hoops because the *only* single use
> > of floating
> > point in OpenSSL is the entropy estimate of an RNG, which
> > is in the
> > range of 0..1023 to begin with [IIRC). Remember that we
> > also have a
> > softfloat submodule for 32-bit ARM, for the same stupid
> > reason. If we
> > could stop pulling in that part of the code (or replace
> > it with an
> > UINT16 when building for the UEFI target), the whole
> > floating point
> > issue would mostly go away AFAICT.
> >
> >
> 
> 
> 

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

[Guomin Jiang]

Summarize current status:

Problem Statement:
Openssl require _fltused to be defined as a constant anywhere floating point is used.
It may use float out of edk2 tree and need _fltused, for example, Microsoft’s OnScreenKeyboard and UiToolKit.

Current Proposal as below:

Proposal 1: Add FltUsed.c into exist library
Detail: Add FltUsed.c into EntryPointLib(PEIM, DXE).
Recommend: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Approve: Laszlo Ersek lersek@redhat.com
Netual: Michael D Kinney <michael.d.kinney@intel.com>
Benefit: Doesn’t need modify every .dsc description file.
Defection: I test that it will fail because of /GL option, the error show fatal error LNK1237: during code generation, compiler introduced reference to symbol '_fltused' defined in module 'UefiApplicationEntryPoint.lib(FltUsed.obj)' compiled with /GL

Proposal 2: Define NULL library
Recommend: Michael D Kinney michael.d.kinney@intel.com
Oppose: Sean sean.brogan=microsoft.com@groups.io , Ard Biesheuvel <ard.biesheuvel@linaro.org>
Benefit: I test it and prove that it is executable.
Defection: It is required that modify every description file.
Defection: It need be very careful that it should only apply some module type(PEIM, DXE_DRIVER, UEFI_APPLICATION) rather than all.
Defection: Build break up.
Action Required: Need evaluate the affection on size.
Consideration: Add PCD to control the feature

Proposal 3: Define FltUsedLib
Detail: Define FltUsedLib and add dependence
Oppose: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Benefit: Doesn’t need care that which module will use it, we will explicitly point it in component file.
Defection: More complex, It is required that modify every description file and modify component meanwhile.
Defection: Build breakup

Thanks
guomin

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Tuesday, April 14, 2020 1:03 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>;
> ard.biesheuvel@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: lersek@redhat.com; macarl@microsoft.com
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> float.
> 
> UEFI spec allows using float operation so asking module developers avoid
> using float may not make sense. Even UefiCpuPkg\Library\BaseUefiCpuLib\
> provides routine to initialize float support for X86.
> 
> Given ARM already uses NULL library class mechanism to supply the compiler
> stub, I prefer X86 aligns to the ARM style.
> 
> The only unsure thing is the size impact when a module is not using float. We
> expect there is no size impact when a module is not using float.
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Michael
> > D Kinney
> > Sent: Thursday, April 2, 2020 12:38 AM
> > To: devel@edk2.groups.io; ard.biesheuvel@linaro.org; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Cc: lersek@redhat.com; macarl@microsoft.com
> > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib
> > for float.
> >
> > Hi Ard,
> >
> > I think adding a dependency in the module entry point libs is also
> > good way to guarantee an intrinsic lib is available.  I agree that it
> > can provide module type and arch specific mappings.  The NULL lib
> > instance can do the same if the NULL instance is listed in the
> > module/arch specific library classes sections. a few example section
> > names.
> >
> > [LibraryClasses.ARM.PEIM, LibraryClasses.AARCH64.PEIM]
> >
> > [LibraryClasses.IA32.UEFI_DRIVER]
> >
> > [LibraryClasses.X64.DXE_DRIVER]
> >
> > So either way, the DSC files need to provide the library mapping.  The
> > only difference between these
> > 2 approaches is that adding a dependency to the module entry point
> > libs uses a formally defined library class name for the intrinsic
> > functions vs.
> > the un-named NULL library instance.  A formally defined library class
> > name is better supported for things like unit tests from the
> > UnitTestFrameworkPkg.
> >
> > The fltused issue would not go away of we removed the float usage for
> > OpenSSL.  One or more other libs or the module could use float/double
> > types and this issue would pop up again.  I do agree it would be
> > cleaner if we could use OpenSSL without floating point.
> >
> > I think adding an intrinsic lib for IA32/X64 for VS20xx generation of
> > intrinsic functions would address fltused and other common C
> > implementation styles that generate intrinsic functions (e.g. 64-bit
> > int math on 32-bit, structure variable assignments, and loops that
> > fill a buffer with a const value) by VS20xx compilers.  An intrinsic
> > lib for GCC would also help with these same common C implementation
> > styles that generate intrinsic functions.
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> > > Biesheuvel
> > > Sent: Tuesday, March 31, 2020 11:43 PM
> > > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > > Cc: devel@edk2.groups.io; lersek@redhat.com; macarl@microsoft.com
> > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
> > > Add FltUsedLib for float.
> > >
> > > On Tue, 31 Mar 2020 at 16:36, Kinney, Michael D
> > > <michael.d.kinney@intel.com> wrote:
> > > >
> > > > ARM and AARCH64 have a compiler intrinsic lib that is
> > > linked against all modules.
> > > >
> > > > [LibraryClasses.ARM, LibraryClasses.AARCH64]
> > > >   #
> > > >   # It is not possible to prevent ARM compiler calls to
> > > generic intrinsic functions.
> > > >   # This library provides the instrinsic functions
> > > generated by a given compiler.
> > > >   # [LibraryClasses.ARM] and NULL mean link this
> > > library into all ARM images.
> > > >   #
> > > >
> > > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrins
> > > icsLib.inf
> > > >
> > > > Can we use this same technique for IA32/X64 VS builds?
> > > >
> > >
> > > In my opinion, having these intrinsics libraries added via NULL
> > > library class resolution was a mistake to begin with.
> > >
> > > Every component that we build incorporates some kind of startup code
> > > library that defines the _ModuleEntryPoint symbol, and it would be
> > > much better to make those libraries include an IntrinsicsLibrary
> > > dependency that can be satisfied by arch specific versions that also
> > > encapsulate the toolchain dependencies (such as this _fltused
> > > symbol, or memcpy/memset on ARM/GCC).
> > >
> > > On another note, it is still deeply disappointing that we need to
> > > jump through all of these hoops because the *only* single use of
> > > floating point in OpenSSL is the entropy estimate of an RNG, which
> > > is in the range of 0..1023 to begin with [IIRC). Remember that we
> > > also have a softfloat submodule for 32-bit ARM, for the same stupid
> > > reason. If we could stop pulling in that part of the code (or
> > > replace it with an
> > > UINT16 when building for the UEFI target), the whole floating point
> > > issue would mostly go away AFAICT.
> > >
> > >
> >
> >
> >
> 
> 
> 

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

[Ard Biesheuvel]

On Tue, 14 Apr 2020 at 09:01, Jiang, Guomin <guomin.jiang@intel.com> wrote:
>
> Summarize current status:
>
> Problem Statement:
> Openssl require _fltused to be defined as a constant anywhere floating point is used.
> It may use float out of edk2 tree and need _fltused, for example, Microsoft’s OnScreenKeyboard and UiToolKit.
>
> Current Proposal as below:
>
> Proposal 1: Add FltUsed.c into exist library
> Detail: Add FltUsed.c into EntryPointLib(PEIM, DXE).
> Recommend: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Approve: Laszlo Ersek lersek@redhat.com
> Netual: Michael D Kinney <michael.d.kinney@intel.com>
> Benefit: Doesn’t need modify every .dsc description file.
> Defection: I test that it will fail because of /GL option, the error show fatal error LNK1237: during code generation, compiler introduced reference to symbol '_fltused' defined in module 'UefiApplicationEntryPoint.lib(FltUsed.obj)' compiled with /GL
>

Can you elaborate on this issue? What does /GL do, and why is it
throwing this error?

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

[Guomin Jiang]

Hi Ard,

it explain the reason at https://docs.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-error-lnk1237?view=vs-2019.
It can use ```MSFT:*_*_*_DLINK_FLAGS = /include:_fltused``` to resolve the error, but it is complex.

Best Regards
Guomin

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Friday, April 17, 2020 4:15 PM
> To: Jiang, Guomin <guomin.jiang@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@arm.com>
> Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; lersek@redhat.com; macarl@microsoft.com
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> float.
> 
> On Tue, 14 Apr 2020 at 09:01, Jiang, Guomin <guomin.jiang@intel.com>
> wrote:
> >
> > Summarize current status:
> >
> > Problem Statement:
> > Openssl require _fltused to be defined as a constant anywhere floating
> point is used.
> > It may use float out of edk2 tree and need _fltused, for example,
> Microsoft’s OnScreenKeyboard and UiToolKit.
> >
> > Current Proposal as below:
> >
> > Proposal 1: Add FltUsed.c into exist library
> > Detail: Add FltUsed.c into EntryPointLib(PEIM, DXE).
> > Recommend: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Approve: Laszlo Ersek lersek@redhat.com
> > Netual: Michael D Kinney <michael.d.kinney@intel.com>
> > Benefit: Doesn’t need modify every .dsc description file.
> > Defection: I test that it will fail because of /GL option, the error
> > show fatal error LNK1237: during code generation, compiler introduced
> > reference to symbol '_fltused' defined in module
> > 'UefiApplicationEntryPoint.lib(FltUsed.obj)' compiled with /GL
> >
> 
> Can you elaborate on this issue? What does /GL do, and why is it throwing
> this error?
> 
> 

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

[Guomin Jiang]

The size comparation between with _fltused and without _fltused, use OvmfPkgX64 as build target
OvmfX64  EFI_FV_TAKEN_SIZE  |   Without _fltused       | With _fltused
DXEFV                                             |  0x46bbe0                     | 0x46bbc0
FVMAIN_COMPACT                     |  0x12c8a0                      | 0x12c7f0
PEIFV                                              |  0x4cae8                         | 0x4ca68
From the result, it will occupy less size rather than more size.

Code Change as below and build command is ```build -p OvmfPkg/OvmfPkgX64 -b DEBUG -t VS2019 -a X64 -DTPM_ENABLE=TRUE```

diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
index 94fe341bec..c4136916d0 100644
--- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
+++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
@@ -2,7 +2,7 @@
   Intrinsic Memory Routines Wrapper Implementation for OpenSSL-based
   Cryptographic Library.

-Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2010 - 2020, Intel Corporation. All rights reserved.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent

 **/
@@ -13,16 +13,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent

 typedef UINTN  size_t;

-#if defined(__GNUC__) || defined(__clang__)
-  #define GLOBAL_USED __attribute__((used))
-#else
-  #define GLOBAL_USED
-#endif
-
-/* OpenSSL will use floating point support, and C compiler produces the _fltused
-   symbol by default. Simply define this symbol here to satisfy the linker. */
-int  GLOBAL_USED _fltused = 1;
-
 /* Sets buffers to a specified character */
 void * memset (void *dest, int ch, size_t count)
 {
diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
index 3586beb0ab..bab31c07c7 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -1,7 +1,7 @@
 ## @file
 #  Base Library implementation.
 #
-#  Copyright (c) 2007 - 2019, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.<BR>
 #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
 #  Portions copyright (c) 2011 - 2013, ARM Ltd. All rights reserved.<BR>
 #
@@ -60,6 +60,7 @@
   String.c
   FilePaths.c
   BaseLibInternals.h
+  FltUsed.c               | MSFT

 [Sources.Ia32]
   Ia32/WriteTr.nasm
diff --git a/MdePkg/Library/BaseLib/FltUsed.c b/MdePkg/Library/BaseLib/FltUsed.c
new file mode 100644
index 0000000000..c065594266
--- /dev/null
+++ b/MdePkg/Library/BaseLib/FltUsed.c
@@ -0,0 +1,14 @@
+/** @file
+  Declare _fltused symbol for MSVC
+
+  MSVC need this symbol for float, andd it here to feed MSVC. it may remove
+  if MSVC not need it any more.
+
+  Copyright (c) 2020 - 2020, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+//
+// Just for MSVC float
+//
+int _fltused = 0x1;

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin
> Jiang
> Sent: Tuesday, April 14, 2020 3:01 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> Cc: lersek@redhat.com; macarl@microsoft.com
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> float.
> 
> Summarize current status:
> 
> Problem Statement:
> Openssl require _fltused to be defined as a constant anywhere floating point
> is used.
> It may use float out of edk2 tree and need _fltused, for example, Microsoft’s
> OnScreenKeyboard and UiToolKit.
> 
> Current Proposal as below:
> 
> Proposal 1: Add FltUsed.c into exist library
> Detail: Add FltUsed.c into EntryPointLib(PEIM, DXE).
> Recommend: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Approve: Laszlo Ersek lersek@redhat.com
> Netual: Michael D Kinney <michael.d.kinney@intel.com>
> Benefit: Doesn’t need modify every .dsc description file.
> Defection: I test that it will fail because of /GL option, the error show fatal
> error LNK1237: during code generation, compiler introduced reference to
> symbol '_fltused' defined in module
> 'UefiApplicationEntryPoint.lib(FltUsed.obj)' compiled with /GL
> 
> Proposal 2: Define NULL library
> Recommend: Michael D Kinney michael.d.kinney@intel.com
> Oppose: Sean sean.brogan=microsoft.com@groups.io , Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Benefit: I test it and prove that it is executable.
> Defection: It is required that modify every description file.
> Defection: It need be very careful that it should only apply some module
> type(PEIM, DXE_DRIVER, UEFI_APPLICATION) rather than all.
> Defection: Build break up.
> Action Required: Need evaluate the affection on size.
> Consideration: Add PCD to control the feature
> 
> Proposal 3: Define FltUsedLib
> Detail: Define FltUsedLib and add dependence
> Oppose: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Benefit: Doesn’t need care that which module will use it, we will explicitly
> point it in component file.
> Defection: More complex, It is required that modify every description file
> and modify component meanwhile.
> Defection: Build breakup
> 
> Thanks
> guomin
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni,
> Ray
> > Sent: Tuesday, April 14, 2020 1:03 PM
> > To: devel@edk2.groups.io; Kinney, Michael D
> > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org; Kinney,
> > Michael D <michael.d.kinney@intel.com>
> > Cc: lersek@redhat.com; macarl@microsoft.com
> > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib
> > for float.
> >
> > UEFI spec allows using float operation so asking module developers
> > avoid using float may not make sense. Even
> > UefiCpuPkg\Library\BaseUefiCpuLib\
> > provides routine to initialize float support for X86.
> >
> > Given ARM already uses NULL library class mechanism to supply the
> > compiler stub, I prefer X86 aligns to the ARM style.
> >
> > The only unsure thing is the size impact when a module is not using
> > float. We expect there is no size impact when a module is not using float.
> >
> > Thanks,
> > Ray
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Michael
> > > D Kinney
> > > Sent: Thursday, April 2, 2020 12:38 AM
> > > To: devel@edk2.groups.io; ard.biesheuvel@linaro.org; Kinney, Michael
> > > D <michael.d.kinney@intel.com>
> > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > FltUsedLib for float.
> > >
> > > Hi Ard,
> > >
> > > I think adding a dependency in the module entry point libs is also
> > > good way to guarantee an intrinsic lib is available.  I agree that
> > > it can provide module type and arch specific mappings.  The NULL lib
> > > instance can do the same if the NULL instance is listed in the
> > > module/arch specific library classes sections. a few example section
> > > names.
> > >
> > > [LibraryClasses.ARM.PEIM, LibraryClasses.AARCH64.PEIM]
> > >
> > > [LibraryClasses.IA32.UEFI_DRIVER]
> > >
> > > [LibraryClasses.X64.DXE_DRIVER]
> > >
> > > So either way, the DSC files need to provide the library mapping.
> > > The only difference between these
> > > 2 approaches is that adding a dependency to the module entry point
> > > libs uses a formally defined library class name for the intrinsic
> > > functions vs.
> > > the un-named NULL library instance.  A formally defined library
> > > class name is better supported for things like unit tests from the
> > > UnitTestFrameworkPkg.
> > >
> > > The fltused issue would not go away of we removed the float usage
> > > for OpenSSL.  One or more other libs or the module could use
> > > float/double types and this issue would pop up again.  I do agree it
> > > would be cleaner if we could use OpenSSL without floating point.
> > >
> > > I think adding an intrinsic lib for IA32/X64 for VS20xx generation
> > > of intrinsic functions would address fltused and other common C
> > > implementation styles that generate intrinsic functions (e.g. 64-bit
> > > int math on 32-bit, structure variable assignments, and loops that
> > > fill a buffer with a const value) by VS20xx compilers.  An intrinsic
> > > lib for GCC would also help with these same common C implementation
> > > styles that generate intrinsic functions.
> > >
> > > Mike
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Ard
> > > > Biesheuvel
> > > > Sent: Tuesday, March 31, 2020 11:43 PM
> > > > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > > > Cc: devel@edk2.groups.io; lersek@redhat.com; macarl@microsoft.com
> > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
> > > > Add FltUsedLib for float.
> > > >
> > > > On Tue, 31 Mar 2020 at 16:36, Kinney, Michael D
> > > > <michael.d.kinney@intel.com> wrote:
> > > > >
> > > > > ARM and AARCH64 have a compiler intrinsic lib that is
> > > > linked against all modules.
> > > > >
> > > > > [LibraryClasses.ARM, LibraryClasses.AARCH64]
> > > > >   #
> > > > >   # It is not possible to prevent ARM compiler calls to
> > > > generic intrinsic functions.
> > > > >   # This library provides the instrinsic functions
> > > > generated by a given compiler.
> > > > >   # [LibraryClasses.ARM] and NULL mean link this
> > > > library into all ARM images.
> > > > >   #
> > > > >
> > > > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrins
> > > > icsLib.inf
> > > > >
> > > > > Can we use this same technique for IA32/X64 VS builds?
> > > > >
> > > >
> > > > In my opinion, having these intrinsics libraries added via NULL
> > > > library class resolution was a mistake to begin with.
> > > >
> > > > Every component that we build incorporates some kind of startup
> > > > code library that defines the _ModuleEntryPoint symbol, and it
> > > > would be much better to make those libraries include an
> > > > IntrinsicsLibrary dependency that can be satisfied by arch
> > > > specific versions that also encapsulate the toolchain dependencies
> > > > (such as this _fltused symbol, or memcpy/memset on ARM/GCC).
> > > >
> > > > On another note, it is still deeply disappointing that we need to
> > > > jump through all of these hoops because the *only* single use of
> > > > floating point in OpenSSL is the entropy estimate of an RNG, which
> > > > is in the range of 0..1023 to begin with [IIRC). Remember that we
> > > > also have a softfloat submodule for 32-bit ARM, for the same
> > > > stupid reason. If we could stop pulling in that part of the code
> > > > (or replace it with an
> > > > UINT16 when building for the UEFI target), the whole floating
> > > > point issue would mostly go away AFAICT.
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
> 
> 
> 

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

[Ni, Ray]

Guomin,
Can you investigate why moving _fltused from CryptoPkg/IntrinsicLib to MdePkg/BaseLib saves size?

Thanks,
Ray

> -----Original Message-----
> From: Jiang, Guomin <guomin.jiang@intel.com>
> Sent: Thursday, April 23, 2020 9:34 AM
> To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>; Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> Cc: lersek@redhat.com; macarl@microsoft.com
> Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.
> 
> The size comparation between with _fltused and without _fltused, use OvmfPkgX64 as build target
> OvmfX64  EFI_FV_TAKEN_SIZE  |   Without _fltused       | With _fltused
> DXEFV                                             |  0x46bbe0                     | 0x46bbc0
> FVMAIN_COMPACT                     |  0x12c8a0                      | 0x12c7f0
> PEIFV                                              |  0x4cae8                         | 0x4ca68
> From the result, it will occupy less size rather than more size.
> 
> Code Change as below and build command is ```build -p OvmfPkg/OvmfPkgX64 -b DEBUG -t VS2019 -a X64 -
> DTPM_ENABLE=TRUE```
> 
> diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> index 94fe341bec..c4136916d0 100644
> --- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> +++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> @@ -2,7 +2,7 @@
>    Intrinsic Memory Routines Wrapper Implementation for OpenSSL-based
>    Cryptographic Library.
> 
> -Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2010 - 2020, Intel Corporation. All rights reserved.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -13,16 +13,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  typedef UINTN  size_t;
> 
> -#if defined(__GNUC__) || defined(__clang__)
> -  #define GLOBAL_USED __attribute__((used))
> -#else
> -  #define GLOBAL_USED
> -#endif
> -
> -/* OpenSSL will use floating point support, and C compiler produces the _fltused
> -   symbol by default. Simply define this symbol here to satisfy the linker. */
> -int  GLOBAL_USED _fltused = 1;
> -
>  /* Sets buffers to a specified character */
>  void * memset (void *dest, int ch, size_t count)
>  {
> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
> index 3586beb0ab..bab31c07c7 100644
> --- a/MdePkg/Library/BaseLib/BaseLib.inf
> +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  #  Base Library implementation.
>  #
> -#  Copyright (c) 2007 - 2019, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.<BR>
>  #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
>  #  Portions copyright (c) 2011 - 2013, ARM Ltd. All rights reserved.<BR>
>  #
> @@ -60,6 +60,7 @@
>    String.c
>    FilePaths.c
>    BaseLibInternals.h
> +  FltUsed.c               | MSFT
> 
>  [Sources.Ia32]
>    Ia32/WriteTr.nasm
> diff --git a/MdePkg/Library/BaseLib/FltUsed.c b/MdePkg/Library/BaseLib/FltUsed.c
> new file mode 100644
> index 0000000000..c065594266
> --- /dev/null
> +++ b/MdePkg/Library/BaseLib/FltUsed.c
> @@ -0,0 +1,14 @@
> +/** @file
> +  Declare _fltused symbol for MSVC
> +
> +  MSVC need this symbol for float, andd it here to feed MSVC. it may remove
> +  if MSVC not need it any more.
> +
> +  Copyright (c) 2020 - 2020, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +//
> +// Just for MSVC float
> +//
> +int _fltused = 0x1;
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin
> > Jiang
> > Sent: Tuesday, April 14, 2020 3:01 PM
> > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > Cc: lersek@redhat.com; macarl@microsoft.com
> > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> > float.
> >
> > Summarize current status:
> >
> > Problem Statement:
> > Openssl require _fltused to be defined as a constant anywhere floating point
> > is used.
> > It may use float out of edk2 tree and need _fltused, for example, Microsoft’s
> > OnScreenKeyboard and UiToolKit.
> >
> > Current Proposal as below:
> >
> > Proposal 1: Add FltUsed.c into exist library
> > Detail: Add FltUsed.c into EntryPointLib(PEIM, DXE).
> > Recommend: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Approve: Laszlo Ersek lersek@redhat.com
> > Netual: Michael D Kinney <michael.d.kinney@intel.com>
> > Benefit: Doesn’t need modify every .dsc description file.
> > Defection: I test that it will fail because of /GL option, the error show fatal
> > error LNK1237: during code generation, compiler introduced reference to
> > symbol '_fltused' defined in module
> > 'UefiApplicationEntryPoint.lib(FltUsed.obj)' compiled with /GL
> >
> > Proposal 2: Define NULL library
> > Recommend: Michael D Kinney michael.d.kinney@intel.com
> > Oppose: Sean sean.brogan=microsoft.com@groups.io , Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>
> > Benefit: I test it and prove that it is executable.
> > Defection: It is required that modify every description file.
> > Defection: It need be very careful that it should only apply some module
> > type(PEIM, DXE_DRIVER, UEFI_APPLICATION) rather than all.
> > Defection: Build break up.
> > Action Required: Need evaluate the affection on size.
> > Consideration: Add PCD to control the feature
> >
> > Proposal 3: Define FltUsedLib
> > Detail: Define FltUsedLib and add dependence
> > Oppose: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Benefit: Doesn’t need care that which module will use it, we will explicitly
> > point it in component file.
> > Defection: More complex, It is required that modify every description file
> > and modify component meanwhile.
> > Defection: Build breakup
> >
> > Thanks
> > guomin
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni,
> > Ray
> > > Sent: Tuesday, April 14, 2020 1:03 PM
> > > To: devel@edk2.groups.io; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org; Kinney,
> > > Michael D <michael.d.kinney@intel.com>
> > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib
> > > for float.
> > >
> > > UEFI spec allows using float operation so asking module developers
> > > avoid using float may not make sense. Even
> > > UefiCpuPkg\Library\BaseUefiCpuLib\
> > > provides routine to initialize float support for X86.
> > >
> > > Given ARM already uses NULL library class mechanism to supply the
> > > compiler stub, I prefer X86 aligns to the ARM style.
> > >
> > > The only unsure thing is the size impact when a module is not using
> > > float. We expect there is no size impact when a module is not using float.
> > >
> > > Thanks,
> > > Ray
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Michael
> > > > D Kinney
> > > > Sent: Thursday, April 2, 2020 12:38 AM
> > > > To: devel@edk2.groups.io; ard.biesheuvel@linaro.org; Kinney, Michael
> > > > D <michael.d.kinney@intel.com>
> > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > FltUsedLib for float.
> > > >
> > > > Hi Ard,
> > > >
> > > > I think adding a dependency in the module entry point libs is also
> > > > good way to guarantee an intrinsic lib is available.  I agree that
> > > > it can provide module type and arch specific mappings.  The NULL lib
> > > > instance can do the same if the NULL instance is listed in the
> > > > module/arch specific library classes sections. a few example section
> > > > names.
> > > >
> > > > [LibraryClasses.ARM.PEIM, LibraryClasses.AARCH64.PEIM]
> > > >
> > > > [LibraryClasses.IA32.UEFI_DRIVER]
> > > >
> > > > [LibraryClasses.X64.DXE_DRIVER]
> > > >
> > > > So either way, the DSC files need to provide the library mapping.
> > > > The only difference between these
> > > > 2 approaches is that adding a dependency to the module entry point
> > > > libs uses a formally defined library class name for the intrinsic
> > > > functions vs.
> > > > the un-named NULL library instance.  A formally defined library
> > > > class name is better supported for things like unit tests from the
> > > > UnitTestFrameworkPkg.
> > > >
> > > > The fltused issue would not go away of we removed the float usage
> > > > for OpenSSL.  One or more other libs or the module could use
> > > > float/double types and this issue would pop up again.  I do agree it
> > > > would be cleaner if we could use OpenSSL without floating point.
> > > >
> > > > I think adding an intrinsic lib for IA32/X64 for VS20xx generation
> > > > of intrinsic functions would address fltused and other common C
> > > > implementation styles that generate intrinsic functions (e.g. 64-bit
> > > > int math on 32-bit, structure variable assignments, and loops that
> > > > fill a buffer with a const value) by VS20xx compilers.  An intrinsic
> > > > lib for GCC would also help with these same common C implementation
> > > > styles that generate intrinsic functions.
> > > >
> > > > Mike
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Ard
> > > > > Biesheuvel
> > > > > Sent: Tuesday, March 31, 2020 11:43 PM
> > > > > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > > > > Cc: devel@edk2.groups.io; lersek@redhat.com; macarl@microsoft.com
> > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
> > > > > Add FltUsedLib for float.
> > > > >
> > > > > On Tue, 31 Mar 2020 at 16:36, Kinney, Michael D
> > > > > <michael.d.kinney@intel.com> wrote:
> > > > > >
> > > > > > ARM and AARCH64 have a compiler intrinsic lib that is
> > > > > linked against all modules.
> > > > > >
> > > > > > [LibraryClasses.ARM, LibraryClasses.AARCH64]
> > > > > >   #
> > > > > >   # It is not possible to prevent ARM compiler calls to
> > > > > generic intrinsic functions.
> > > > > >   # This library provides the instrinsic functions
> > > > > generated by a given compiler.
> > > > > >   # [LibraryClasses.ARM] and NULL mean link this
> > > > > library into all ARM images.
> > > > > >   #
> > > > > >
> > > > > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrins
> > > > > icsLib.inf
> > > > > >
> > > > > > Can we use this same technique for IA32/X64 VS builds?
> > > > > >
> > > > >
> > > > > In my opinion, having these intrinsics libraries added via NULL
> > > > > library class resolution was a mistake to begin with.
> > > > >
> > > > > Every component that we build incorporates some kind of startup
> > > > > code library that defines the _ModuleEntryPoint symbol, and it
> > > > > would be much better to make those libraries include an
> > > > > IntrinsicsLibrary dependency that can be satisfied by arch
> > > > > specific versions that also encapsulate the toolchain dependencies
> > > > > (such as this _fltused symbol, or memcpy/memset on ARM/GCC).
> > > > >
> > > > > On another note, it is still deeply disappointing that we need to
> > > > > jump through all of these hoops because the *only* single use of
> > > > > floating point in OpenSSL is the entropy estimate of an RNG, which
> > > > > is in the range of 0..1023 to begin with [IIRC). Remember that we
> > > > > also have a softfloat submodule for 32-bit ARM, for the same
> > > > > stupid reason. If we could stop pulling in that part of the code
> > > > > (or replace it with an
> > > > > UINT16 when building for the UEFI target), the whole floating
> > > > > point issue would mostly go away AFAICT.
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> > 
> 

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

[Guomin Jiang]

I guess the key point is /GL option. IntrinsicLib omit the /GL option to avoid the build error, but it abandon the optimization meanwhile.
I will do a test: create a simplest application and just depend IntrinsicLib and add /GL option to compare the different with and without /GL.

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Thursday, April 23, 2020 11:32 AM
> To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io; Kinney,
> Michael D <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> Cc: lersek@redhat.com; macarl@microsoft.com
> Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> float.
> 
> Guomin,
> Can you investigate why moving _fltused from CryptoPkg/IntrinsicLib to
> MdePkg/BaseLib saves size?
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Jiang, Guomin <guomin.jiang@intel.com>
> > Sent: Thursday, April 23, 2020 9:34 AM
> > To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>; Ni,
> > Ray <ray.ni@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > Cc: lersek@redhat.com; macarl@microsoft.com
> > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib
> for float.
> >
> > The size comparation between with _fltused and without _fltused, use
> OvmfPkgX64 as build target
> > OvmfX64  EFI_FV_TAKEN_SIZE  |   Without _fltused       | With _fltused
> > DXEFV                                             |  0x46bbe0                     | 0x46bbc0
> > FVMAIN_COMPACT                     |  0x12c8a0                      | 0x12c7f0
> > PEIFV                                              |  0x4cae8                         | 0x4ca68
> > From the result, it will occupy less size rather than more size.
> >
> > Code Change as below and build command is ```build -p
> > OvmfPkg/OvmfPkgX64 -b DEBUG -t VS2019 -a X64 - DTPM_ENABLE=TRUE```
> >
> > diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > index 94fe341bec..c4136916d0 100644
> > --- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > +++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > @@ -2,7 +2,7 @@
> >    Intrinsic Memory Routines Wrapper Implementation for OpenSSL-based
> >    Cryptographic Library.
> >
> > -Copyright (c) 2010 - 2019, Intel Corporation. All rights
> > reserved.<BR>
> > +Copyright (c) 2010 - 2020, Intel Corporation. All rights
> > +reserved.<BR>
> >  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  **/
> > @@ -13,16 +13,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  typedef UINTN  size_t;
> >
> > -#if defined(__GNUC__) || defined(__clang__)
> > -  #define GLOBAL_USED __attribute__((used)) -#else
> > -  #define GLOBAL_USED
> > -#endif
> > -
> > -/* OpenSSL will use floating point support, and C compiler produces the
> _fltused
> > -   symbol by default. Simply define this symbol here to satisfy the linker. */
> > -int  GLOBAL_USED _fltused = 1;
> > -
> >  /* Sets buffers to a specified character */  void * memset (void
> > *dest, int ch, size_t count)  { diff --git
> > a/MdePkg/Library/BaseLib/BaseLib.inf
> > b/MdePkg/Library/BaseLib/BaseLib.inf
> > index 3586beb0ab..bab31c07c7 100644
> > --- a/MdePkg/Library/BaseLib/BaseLib.inf
> > +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> > @@ -1,7 +1,7 @@
> >  ## @file
> >  #  Base Library implementation.
> >  #
> > -#  Copyright (c) 2007 - 2019, Intel Corporation. All rights
> > reserved.<BR>
> > +#  Copyright (c) 2007 - 2020, Intel Corporation. All rights
> > +reserved.<BR>
> >  #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights
> > reserved.<BR>  #  Portions copyright (c) 2011 - 2013, ARM Ltd. All
> > rights reserved.<BR>  # @@ -60,6 +60,7 @@
> >    String.c
> >    FilePaths.c
> >    BaseLibInternals.h
> > +  FltUsed.c               | MSFT
> >
> >  [Sources.Ia32]
> >    Ia32/WriteTr.nasm
> > diff --git a/MdePkg/Library/BaseLib/FltUsed.c
> > b/MdePkg/Library/BaseLib/FltUsed.c
> > new file mode 100644
> > index 0000000000..c065594266
> > --- /dev/null
> > +++ b/MdePkg/Library/BaseLib/FltUsed.c
> > @@ -0,0 +1,14 @@
> > +/** @file
> > +  Declare _fltused symbol for MSVC
> > +
> > +  MSVC need this symbol for float, andd it here to feed MSVC. it may
> > + remove  if MSVC not need it any more.
> > +
> > +  Copyright (c) 2020 - 2020, Intel Corporation. All rights
> > +reserved.<BR>
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> > +
> > +//
> > +// Just for MSVC float
> > +//
> > +int _fltused = 0x1;
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Guomin Jiang
> > > Sent: Tuesday, April 14, 2020 3:01 PM
> > > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Kinney,
> > > Michael D <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > FltUsedLib for float.
> > >
> > > Summarize current status:
> > >
> > > Problem Statement:
> > > Openssl require _fltused to be defined as a constant anywhere
> > > floating point is used.
> > > It may use float out of edk2 tree and need _fltused, for example,
> > > Microsoft’s OnScreenKeyboard and UiToolKit.
> > >
> > > Current Proposal as below:
> > >
> > > Proposal 1: Add FltUsed.c into exist library
> > > Detail: Add FltUsed.c into EntryPointLib(PEIM, DXE).
> > > Recommend: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Approve: Laszlo Ersek lersek@redhat.com
> > > Netual: Michael D Kinney <michael.d.kinney@intel.com>
> > > Benefit: Doesn’t need modify every .dsc description file.
> > > Defection: I test that it will fail because of /GL option, the error
> > > show fatal error LNK1237: during code generation, compiler
> > > introduced reference to symbol '_fltused' defined in module
> > > 'UefiApplicationEntryPoint.lib(FltUsed.obj)' compiled with /GL
> > >
> > > Proposal 2: Define NULL library
> > > Recommend: Michael D Kinney michael.d.kinney@intel.com
> > > Oppose: Sean sean.brogan=microsoft.com@groups.io , Ard Biesheuvel
> > > <ard.biesheuvel@linaro.org>
> > > Benefit: I test it and prove that it is executable.
> > > Defection: It is required that modify every description file.
> > > Defection: It need be very careful that it should only apply some
> > > module type(PEIM, DXE_DRIVER, UEFI_APPLICATION) rather than all.
> > > Defection: Build break up.
> > > Action Required: Need evaluate the affection on size.
> > > Consideration: Add PCD to control the feature
> > >
> > > Proposal 3: Define FltUsedLib
> > > Detail: Define FltUsedLib and add dependence
> > > Oppose: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Benefit: Doesn’t need care that which module will use it, we will
> > > explicitly point it in component file.
> > > Defection: More complex, It is required that modify every
> > > description file and modify component meanwhile.
> > > Defection: Build breakup
> > >
> > > Thanks
> > > guomin
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni,
> > > Ray
> > > > Sent: Tuesday, April 14, 2020 1:03 PM
> > > > To: devel@edk2.groups.io; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org; Kinney,
> > > > Michael D <michael.d.kinney@intel.com>
> > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > FltUsedLib for float.
> > > >
> > > > UEFI spec allows using float operation so asking module developers
> > > > avoid using float may not make sense. Even
> > > > UefiCpuPkg\Library\BaseUefiCpuLib\
> > > > provides routine to initialize float support for X86.
> > > >
> > > > Given ARM already uses NULL library class mechanism to supply the
> > > > compiler stub, I prefer X86 aligns to the ARM style.
> > > >
> > > > The only unsure thing is the size impact when a module is not
> > > > using float. We expect there is no size impact when a module is not
> using float.
> > > >
> > > > Thanks,
> > > > Ray
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Michael
> > > > > D Kinney
> > > > > Sent: Thursday, April 2, 2020 12:38 AM
> > > > > To: devel@edk2.groups.io; ard.biesheuvel@linaro.org; Kinney,
> > > > > Michael D <michael.d.kinney@intel.com>
> > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > FltUsedLib for float.
> > > > >
> > > > > Hi Ard,
> > > > >
> > > > > I think adding a dependency in the module entry point libs is
> > > > > also good way to guarantee an intrinsic lib is available.  I
> > > > > agree that it can provide module type and arch specific
> > > > > mappings.  The NULL lib instance can do the same if the NULL
> > > > > instance is listed in the module/arch specific library classes
> > > > > sections. a few example section names.
> > > > >
> > > > > [LibraryClasses.ARM.PEIM, LibraryClasses.AARCH64.PEIM]
> > > > >
> > > > > [LibraryClasses.IA32.UEFI_DRIVER]
> > > > >
> > > > > [LibraryClasses.X64.DXE_DRIVER]
> > > > >
> > > > > So either way, the DSC files need to provide the library mapping.
> > > > > The only difference between these
> > > > > 2 approaches is that adding a dependency to the module entry
> > > > > point libs uses a formally defined library class name for the
> > > > > intrinsic functions vs.
> > > > > the un-named NULL library instance.  A formally defined library
> > > > > class name is better supported for things like unit tests from
> > > > > the UnitTestFrameworkPkg.
> > > > >
> > > > > The fltused issue would not go away of we removed the float
> > > > > usage for OpenSSL.  One or more other libs or the module could
> > > > > use float/double types and this issue would pop up again.  I do
> > > > > agree it would be cleaner if we could use OpenSSL without floating
> point.
> > > > >
> > > > > I think adding an intrinsic lib for IA32/X64 for VS20xx
> > > > > generation of intrinsic functions would address fltused and
> > > > > other common C implementation styles that generate intrinsic
> > > > > functions (e.g. 64-bit int math on 32-bit, structure variable
> > > > > assignments, and loops that fill a buffer with a const value) by
> > > > > VS20xx compilers.  An intrinsic lib for GCC would also help with
> > > > > these same common C implementation styles that generate intrinsic
> functions.
> > > > >
> > > > > Mike
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Ard
> > > > > > Biesheuvel
> > > > > > Sent: Tuesday, March 31, 2020 11:43 PM
> > > > > > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > > > > > Cc: devel@edk2.groups.io; lersek@redhat.com;
> > > > > > macarl@microsoft.com
> > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
> > > > > > Add FltUsedLib for float.
> > > > > >
> > > > > > On Tue, 31 Mar 2020 at 16:36, Kinney, Michael D
> > > > > > <michael.d.kinney@intel.com> wrote:
> > > > > > >
> > > > > > > ARM and AARCH64 have a compiler intrinsic lib that is
> > > > > > linked against all modules.
> > > > > > >
> > > > > > > [LibraryClasses.ARM, LibraryClasses.AARCH64]
> > > > > > >   #
> > > > > > >   # It is not possible to prevent ARM compiler calls to
> > > > > > generic intrinsic functions.
> > > > > > >   # This library provides the instrinsic functions
> > > > > > generated by a given compiler.
> > > > > > >   # [LibraryClasses.ARM] and NULL mean link this
> > > > > > library into all ARM images.
> > > > > > >   #
> > > > > > >
> > > > > > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrins
> > > > > > icsLib.inf
> > > > > > >
> > > > > > > Can we use this same technique for IA32/X64 VS builds?
> > > > > > >
> > > > > >
> > > > > > In my opinion, having these intrinsics libraries added via
> > > > > > NULL library class resolution was a mistake to begin with.
> > > > > >
> > > > > > Every component that we build incorporates some kind of
> > > > > > startup code library that defines the _ModuleEntryPoint
> > > > > > symbol, and it would be much better to make those libraries
> > > > > > include an IntrinsicsLibrary dependency that can be satisfied
> > > > > > by arch specific versions that also encapsulate the toolchain
> > > > > > dependencies (such as this _fltused symbol, or memcpy/memset
> on ARM/GCC).
> > > > > >
> > > > > > On another note, it is still deeply disappointing that we need
> > > > > > to jump through all of these hoops because the *only* single
> > > > > > use of floating point in OpenSSL is the entropy estimate of an
> > > > > > RNG, which is in the range of 0..1023 to begin with [IIRC).
> > > > > > Remember that we also have a softfloat submodule for 32-bit
> > > > > > ARM, for the same stupid reason. If we could stop pulling in
> > > > > > that part of the code (or replace it with an
> > > > > > UINT16 when building for the UEFI target), the whole floating
> > > > > > point issue would mostly go away AFAICT.
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > >
> > >
> > > 
> >
> 

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

[Liming Gao]

Guomin:
  You can count the size of all generated EFI images for IA32 and X64, then compare them between two builds. This change should impact EFI image only. Based on this data, we can know the image size impact. 

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin Jiang
> Sent: Thursday, April 23, 2020 12:04 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> Cc: lersek@redhat.com; macarl@microsoft.com
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.
> 
> I guess the key point is /GL option. IntrinsicLib omit the /GL option to avoid the build error, but it abandon the optimization
> meanwhile.
> I will do a test: create a simplest application and just depend IntrinsicLib and add /GL option to compare the different with and
> without /GL.
> 
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Thursday, April 23, 2020 11:32 AM
> > To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io; Kinney,
> > Michael D <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > Cc: lersek@redhat.com; macarl@microsoft.com
> > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> > float.
> >
> > Guomin,
> > Can you investigate why moving _fltused from CryptoPkg/IntrinsicLib to
> > MdePkg/BaseLib saves size?
> >
> > Thanks,
> > Ray
> >
> > > -----Original Message-----
> > > From: Jiang, Guomin <guomin.jiang@intel.com>
> > > Sent: Thursday, April 23, 2020 9:34 AM
> > > To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>; Ni,
> > > Ray <ray.ni@intel.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib
> > for float.
> > >
> > > The size comparation between with _fltused and without _fltused, use
> > OvmfPkgX64 as build target
> > > OvmfX64  EFI_FV_TAKEN_SIZE  |   Without _fltused       | With _fltused
> > > DXEFV                                             |  0x46bbe0                     | 0x46bbc0
> > > FVMAIN_COMPACT                     |  0x12c8a0                      | 0x12c7f0
> > > PEIFV                                              |  0x4cae8                         | 0x4ca68
> > > From the result, it will occupy less size rather than more size.
> > >
> > > Code Change as below and build command is ```build -p
> > > OvmfPkg/OvmfPkgX64 -b DEBUG -t VS2019 -a X64 - DTPM_ENABLE=TRUE```
> > >
> > > diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > index 94fe341bec..c4136916d0 100644
> > > --- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > +++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > @@ -2,7 +2,7 @@
> > >    Intrinsic Memory Routines Wrapper Implementation for OpenSSL-based
> > >    Cryptographic Library.
> > >
> > > -Copyright (c) 2010 - 2019, Intel Corporation. All rights
> > > reserved.<BR>
> > > +Copyright (c) 2010 - 2020, Intel Corporation. All rights
> > > +reserved.<BR>
> > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > >  **/
> > > @@ -13,16 +13,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > >  typedef UINTN  size_t;
> > >
> > > -#if defined(__GNUC__) || defined(__clang__)
> > > -  #define GLOBAL_USED __attribute__((used)) -#else
> > > -  #define GLOBAL_USED
> > > -#endif
> > > -
> > > -/* OpenSSL will use floating point support, and C compiler produces the
> > _fltused
> > > -   symbol by default. Simply define this symbol here to satisfy the linker. */
> > > -int  GLOBAL_USED _fltused = 1;
> > > -
> > >  /* Sets buffers to a specified character */  void * memset (void
> > > *dest, int ch, size_t count)  { diff --git
> > > a/MdePkg/Library/BaseLib/BaseLib.inf
> > > b/MdePkg/Library/BaseLib/BaseLib.inf
> > > index 3586beb0ab..bab31c07c7 100644
> > > --- a/MdePkg/Library/BaseLib/BaseLib.inf
> > > +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> > > @@ -1,7 +1,7 @@
> > >  ## @file
> > >  #  Base Library implementation.
> > >  #
> > > -#  Copyright (c) 2007 - 2019, Intel Corporation. All rights
> > > reserved.<BR>
> > > +#  Copyright (c) 2007 - 2020, Intel Corporation. All rights
> > > +reserved.<BR>
> > >  #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights
> > > reserved.<BR>  #  Portions copyright (c) 2011 - 2013, ARM Ltd. All
> > > rights reserved.<BR>  # @@ -60,6 +60,7 @@
> > >    String.c
> > >    FilePaths.c
> > >    BaseLibInternals.h
> > > +  FltUsed.c               | MSFT
> > >
> > >  [Sources.Ia32]
> > >    Ia32/WriteTr.nasm
> > > diff --git a/MdePkg/Library/BaseLib/FltUsed.c
> > > b/MdePkg/Library/BaseLib/FltUsed.c
> > > new file mode 100644
> > > index 0000000000..c065594266
> > > --- /dev/null
> > > +++ b/MdePkg/Library/BaseLib/FltUsed.c
> > > @@ -0,0 +1,14 @@
> > > +/** @file
> > > +  Declare _fltused symbol for MSVC
> > > +
> > > +  MSVC need this symbol for float, andd it here to feed MSVC. it may
> > > + remove  if MSVC not need it any more.
> > > +
> > > +  Copyright (c) 2020 - 2020, Intel Corporation. All rights
> > > +reserved.<BR>
> > > +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> > > +
> > > +//
> > > +// Just for MSVC float
> > > +//
> > > +int _fltused = 0x1;
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Guomin Jiang
> > > > Sent: Tuesday, April 14, 2020 3:01 PM
> > > > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Kinney,
> > > > Michael D <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > FltUsedLib for float.
> > > >
> > > > Summarize current status:
> > > >
> > > > Problem Statement:
> > > > Openssl require _fltused to be defined as a constant anywhere
> > > > floating point is used.
> > > > It may use float out of edk2 tree and need _fltused, for example,
> > > > Microsoft’s OnScreenKeyboard and UiToolKit.
> > > >
> > > > Current Proposal as below:
> > > >
> > > > Proposal 1: Add FltUsed.c into exist library
> > > > Detail: Add FltUsed.c into EntryPointLib(PEIM, DXE).
> > > > Recommend: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > Approve: Laszlo Ersek lersek@redhat.com
> > > > Netual: Michael D Kinney <michael.d.kinney@intel.com>
> > > > Benefit: Doesn’t need modify every .dsc description file.
> > > > Defection: I test that it will fail because of /GL option, the error
> > > > show fatal error LNK1237: during code generation, compiler
> > > > introduced reference to symbol '_fltused' defined in module
> > > > 'UefiApplicationEntryPoint.lib(FltUsed.obj)' compiled with /GL
> > > >
> > > > Proposal 2: Define NULL library
> > > > Recommend: Michael D Kinney michael.d.kinney@intel.com
> > > > Oppose: Sean sean.brogan=microsoft.com@groups.io , Ard Biesheuvel
> > > > <ard.biesheuvel@linaro.org>
> > > > Benefit: I test it and prove that it is executable.
> > > > Defection: It is required that modify every description file.
> > > > Defection: It need be very careful that it should only apply some
> > > > module type(PEIM, DXE_DRIVER, UEFI_APPLICATION) rather than all.
> > > > Defection: Build break up.
> > > > Action Required: Need evaluate the affection on size.
> > > > Consideration: Add PCD to control the feature
> > > >
> > > > Proposal 3: Define FltUsedLib
> > > > Detail: Define FltUsedLib and add dependence
> > > > Oppose: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > Benefit: Doesn’t need care that which module will use it, we will
> > > > explicitly point it in component file.
> > > > Defection: More complex, It is required that modify every
> > > > description file and modify component meanwhile.
> > > > Defection: Build breakup
> > > >
> > > > Thanks
> > > > guomin
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni,
> > > > Ray
> > > > > Sent: Tuesday, April 14, 2020 1:03 PM
> > > > > To: devel@edk2.groups.io; Kinney, Michael D
> > > > > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org; Kinney,
> > > > > Michael D <michael.d.kinney@intel.com>
> > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > FltUsedLib for float.
> > > > >
> > > > > UEFI spec allows using float operation so asking module developers
> > > > > avoid using float may not make sense. Even
> > > > > UefiCpuPkg\Library\BaseUefiCpuLib\
> > > > > provides routine to initialize float support for X86.
> > > > >
> > > > > Given ARM already uses NULL library class mechanism to supply the
> > > > > compiler stub, I prefer X86 aligns to the ARM style.
> > > > >
> > > > > The only unsure thing is the size impact when a module is not
> > > > > using float. We expect there is no size impact when a module is not
> > using float.
> > > > >
> > > > > Thanks,
> > > > > Ray
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > Michael
> > > > > > D Kinney
> > > > > > Sent: Thursday, April 2, 2020 12:38 AM
> > > > > > To: devel@edk2.groups.io; ard.biesheuvel@linaro.org; Kinney,
> > > > > > Michael D <michael.d.kinney@intel.com>
> > > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > > FltUsedLib for float.
> > > > > >
> > > > > > Hi Ard,
> > > > > >
> > > > > > I think adding a dependency in the module entry point libs is
> > > > > > also good way to guarantee an intrinsic lib is available.  I
> > > > > > agree that it can provide module type and arch specific
> > > > > > mappings.  The NULL lib instance can do the same if the NULL
> > > > > > instance is listed in the module/arch specific library classes
> > > > > > sections. a few example section names.
> > > > > >
> > > > > > [LibraryClasses.ARM.PEIM, LibraryClasses.AARCH64.PEIM]
> > > > > >
> > > > > > [LibraryClasses.IA32.UEFI_DRIVER]
> > > > > >
> > > > > > [LibraryClasses.X64.DXE_DRIVER]
> > > > > >
> > > > > > So either way, the DSC files need to provide the library mapping.
> > > > > > The only difference between these
> > > > > > 2 approaches is that adding a dependency to the module entry
> > > > > > point libs uses a formally defined library class name for the
> > > > > > intrinsic functions vs.
> > > > > > the un-named NULL library instance.  A formally defined library
> > > > > > class name is better supported for things like unit tests from
> > > > > > the UnitTestFrameworkPkg.
> > > > > >
> > > > > > The fltused issue would not go away of we removed the float
> > > > > > usage for OpenSSL.  One or more other libs or the module could
> > > > > > use float/double types and this issue would pop up again.  I do
> > > > > > agree it would be cleaner if we could use OpenSSL without floating
> > point.
> > > > > >
> > > > > > I think adding an intrinsic lib for IA32/X64 for VS20xx
> > > > > > generation of intrinsic functions would address fltused and
> > > > > > other common C implementation styles that generate intrinsic
> > > > > > functions (e.g. 64-bit int math on 32-bit, structure variable
> > > > > > assignments, and loops that fill a buffer with a const value) by
> > > > > > VS20xx compilers.  An intrinsic lib for GCC would also help with
> > > > > > these same common C implementation styles that generate intrinsic
> > functions.
> > > > > >
> > > > > > Mike
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Ard
> > > > > > > Biesheuvel
> > > > > > > Sent: Tuesday, March 31, 2020 11:43 PM
> > > > > > > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > > > > > > Cc: devel@edk2.groups.io; lersek@redhat.com;
> > > > > > > macarl@microsoft.com
> > > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
> > > > > > > Add FltUsedLib for float.
> > > > > > >
> > > > > > > On Tue, 31 Mar 2020 at 16:36, Kinney, Michael D
> > > > > > > <michael.d.kinney@intel.com> wrote:
> > > > > > > >
> > > > > > > > ARM and AARCH64 have a compiler intrinsic lib that is
> > > > > > > linked against all modules.
> > > > > > > >
> > > > > > > > [LibraryClasses.ARM, LibraryClasses.AARCH64]
> > > > > > > >   #
> > > > > > > >   # It is not possible to prevent ARM compiler calls to
> > > > > > > generic intrinsic functions.
> > > > > > > >   # This library provides the instrinsic functions
> > > > > > > generated by a given compiler.
> > > > > > > >   # [LibraryClasses.ARM] and NULL mean link this
> > > > > > > library into all ARM images.
> > > > > > > >   #
> > > > > > > >
> > > > > > > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrins
> > > > > > > icsLib.inf
> > > > > > > >
> > > > > > > > Can we use this same technique for IA32/X64 VS builds?
> > > > > > > >
> > > > > > >
> > > > > > > In my opinion, having these intrinsics libraries added via
> > > > > > > NULL library class resolution was a mistake to begin with.
> > > > > > >
> > > > > > > Every component that we build incorporates some kind of
> > > > > > > startup code library that defines the _ModuleEntryPoint
> > > > > > > symbol, and it would be much better to make those libraries
> > > > > > > include an IntrinsicsLibrary dependency that can be satisfied
> > > > > > > by arch specific versions that also encapsulate the toolchain
> > > > > > > dependencies (such as this _fltused symbol, or memcpy/memset
> > on ARM/GCC).
> > > > > > >
> > > > > > > On another note, it is still deeply disappointing that we need
> > > > > > > to jump through all of these hoops because the *only* single
> > > > > > > use of floating point in OpenSSL is the entropy estimate of an
> > > > > > > RNG, which is in the range of 0..1023 to begin with [IIRC).
> > > > > > > Remember that we also have a softfloat submodule for 32-bit
> > > > > > > ARM, for the same stupid reason. If we could stop pulling in
> > > > > > > that part of the code (or replace it with an
> > > > > > > UINT16 when building for the UEFI target), the whole floating
> > > > > > > point issue would mostly go away AFAICT.
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > >
> >
> 
> 
> 

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

[Guomin Jiang]

I have compare the Image different, the are two image size will be affect before and after move _fltused to BaseLib.

One is TcgPei, another is SecurityStubDxe, the detail as below table
                               TcgPei                    | SecurityStubDxe
Before move:       22688(decimal)   |   30624(decimal)
After move           22656(decimal)   |   30592(decimal)

The size of reducation is 32 or 0x20 bytes
Those component is located the boundary, so it will occupy more size to meet the alignment.

Another question is that some component will fail in CI result when move _fltused to BaseLib.
I am checking it.
 
> -----Original Message-----
> From: Gao, Liming <liming.gao@intel.com>
> Sent: Thursday, April 23, 2020 1:49 PM
> To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> ard.biesheuvel@linaro.org
> Cc: lersek@redhat.com; macarl@microsoft.com
> Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> float.
> 
> Guomin:
>   You can count the size of all generated EFI images for IA32 and X64, then
> compare them between two builds. This change should impact EFI image
> only. Based on this data, we can know the image size impact.
> 
> Thanks
> Liming
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Guomin
> > Jiang
> > Sent: Thursday, April 23, 2020 12:04 PM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Kinney, Michael
> > D <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > Cc: lersek@redhat.com; macarl@microsoft.com
> > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib
> for float.
> >
> > I guess the key point is /GL option. IntrinsicLib omit the /GL option
> > to avoid the build error, but it abandon the optimization meanwhile.
> > I will do a test: create a simplest application and just depend
> > IntrinsicLib and add /GL option to compare the different with and without
> /GL.
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni@intel.com>
> > > Sent: Thursday, April 23, 2020 11:32 AM
> > > To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io;
> > > Kinney, Michael D <michael.d.kinney@intel.com>;
> > > ard.biesheuvel@linaro.org
> > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > FltUsedLib for float.
> > >
> > > Guomin,
> > > Can you investigate why moving _fltused from CryptoPkg/IntrinsicLib
> > > to MdePkg/BaseLib saves size?
> > >
> > > Thanks,
> > > Ray
> > >
> > > > -----Original Message-----
> > > > From: Jiang, Guomin <guomin.jiang@intel.com>
> > > > Sent: Thursday, April 23, 2020 9:34 AM
> > > > To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>;
> > > > Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > FltUsedLib
> > > for float.
> > > >
> > > > The size comparation between with _fltused and without _fltused,
> > > > use
> > > OvmfPkgX64 as build target
> > > > OvmfX64  EFI_FV_TAKEN_SIZE  |   Without _fltused       | With _fltused
> > > > DXEFV                                             |  0x46bbe0                     | 0x46bbc0
> > > > FVMAIN_COMPACT                     |  0x12c8a0                      | 0x12c7f0
> > > > PEIFV                                              |  0x4cae8                         | 0x4ca68
> > > > From the result, it will occupy less size rather than more size.
> > > >
> > > > Code Change as below and build command is ```build -p
> > > > OvmfPkg/OvmfPkgX64 -b DEBUG -t VS2019 -a X64 -
> DTPM_ENABLE=TRUE```
> > > >
> > > > diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > > b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > > index 94fe341bec..c4136916d0 100644
> > > > --- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > > +++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > > @@ -2,7 +2,7 @@
> > > >    Intrinsic Memory Routines Wrapper Implementation for OpenSSL-
> based
> > > >    Cryptographic Library.
> > > >
> > > > -Copyright (c) 2010 - 2019, Intel Corporation. All rights
> > > > reserved.<BR>
> > > > +Copyright (c) 2010 - 2020, Intel Corporation. All rights
> > > > +reserved.<BR>
> > > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > >
> > > >  **/
> > > > @@ -13,16 +13,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > >
> > > >  typedef UINTN  size_t;
> > > >
> > > > -#if defined(__GNUC__) || defined(__clang__)
> > > > -  #define GLOBAL_USED __attribute__((used)) -#else
> > > > -  #define GLOBAL_USED
> > > > -#endif
> > > > -
> > > > -/* OpenSSL will use floating point support, and C compiler
> > > > produces the
> > > _fltused
> > > > -   symbol by default. Simply define this symbol here to satisfy the linker.
> */
> > > > -int  GLOBAL_USED _fltused = 1;
> > > > -
> > > >  /* Sets buffers to a specified character */  void * memset (void
> > > > *dest, int ch, size_t count)  { diff --git
> > > > a/MdePkg/Library/BaseLib/BaseLib.inf
> > > > b/MdePkg/Library/BaseLib/BaseLib.inf
> > > > index 3586beb0ab..bab31c07c7 100644
> > > > --- a/MdePkg/Library/BaseLib/BaseLib.inf
> > > > +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> > > > @@ -1,7 +1,7 @@
> > > >  ## @file
> > > >  #  Base Library implementation.
> > > >  #
> > > > -#  Copyright (c) 2007 - 2019, Intel Corporation. All rights
> > > > reserved.<BR>
> > > > +#  Copyright (c) 2007 - 2020, Intel Corporation. All rights
> > > > +reserved.<BR>
> > > >  #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights
> > > > reserved.<BR>  #  Portions copyright (c) 2011 - 2013, ARM Ltd. All
> > > > rights reserved.<BR>  # @@ -60,6 +60,7 @@
> > > >    String.c
> > > >    FilePaths.c
> > > >    BaseLibInternals.h
> > > > +  FltUsed.c               | MSFT
> > > >
> > > >  [Sources.Ia32]
> > > >    Ia32/WriteTr.nasm
> > > > diff --git a/MdePkg/Library/BaseLib/FltUsed.c
> > > > b/MdePkg/Library/BaseLib/FltUsed.c
> > > > new file mode 100644
> > > > index 0000000000..c065594266
> > > > --- /dev/null
> > > > +++ b/MdePkg/Library/BaseLib/FltUsed.c
> > > > @@ -0,0 +1,14 @@
> > > > +/** @file
> > > > +  Declare _fltused symbol for MSVC
> > > > +
> > > > +  MSVC need this symbol for float, andd it here to feed MSVC. it
> > > > + may remove  if MSVC not need it any more.
> > > > +
> > > > +  Copyright (c) 2020 - 2020, Intel Corporation. All rights
> > > > +reserved.<BR>
> > > > +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> > > > +
> > > > +//
> > > > +// Just for MSVC float
> > > > +//
> > > > +int _fltused = 0x1;
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > Guomin Jiang
> > > > > Sent: Tuesday, April 14, 2020 3:01 PM
> > > > > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Kinney,
> > > > > Michael D <michael.d.kinney@intel.com>;
> > > > > ard.biesheuvel@linaro.org
> > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > FltUsedLib for float.
> > > > >
> > > > > Summarize current status:
> > > > >
> > > > > Problem Statement:
> > > > > Openssl require _fltused to be defined as a constant anywhere
> > > > > floating point is used.
> > > > > It may use float out of edk2 tree and need _fltused, for
> > > > > example, Microsoft’s OnScreenKeyboard and UiToolKit.
> > > > >
> > > > > Current Proposal as below:
> > > > >
> > > > > Proposal 1: Add FltUsed.c into exist library
> > > > > Detail: Add FltUsed.c into EntryPointLib(PEIM, DXE).
> > > > > Recommend: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > Approve: Laszlo Ersek lersek@redhat.com
> > > > > Netual: Michael D Kinney <michael.d.kinney@intel.com>
> > > > > Benefit: Doesn’t need modify every .dsc description file.
> > > > > Defection: I test that it will fail because of /GL option, the
> > > > > error show fatal error LNK1237: during code generation, compiler
> > > > > introduced reference to symbol '_fltused' defined in module
> > > > > 'UefiApplicationEntryPoint.lib(FltUsed.obj)' compiled with /GL
> > > > >
> > > > > Proposal 2: Define NULL library
> > > > > Recommend: Michael D Kinney michael.d.kinney@intel.com
> > > > > Oppose: Sean sean.brogan=microsoft.com@groups.io , Ard
> > > > > Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > Benefit: I test it and prove that it is executable.
> > > > > Defection: It is required that modify every description file.
> > > > > Defection: It need be very careful that it should only apply
> > > > > some module type(PEIM, DXE_DRIVER, UEFI_APPLICATION) rather
> than all.
> > > > > Defection: Build break up.
> > > > > Action Required: Need evaluate the affection on size.
> > > > > Consideration: Add PCD to control the feature
> > > > >
> > > > > Proposal 3: Define FltUsedLib
> > > > > Detail: Define FltUsedLib and add dependence
> > > > > Oppose: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > Benefit: Doesn’t need care that which module will use it, we
> > > > > will explicitly point it in component file.
> > > > > Defection: More complex, It is required that modify every
> > > > > description file and modify component meanwhile.
> > > > > Defection: Build breakup
> > > > >
> > > > > Thanks
> > > > > guomin
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > > Ni,
> > > > > Ray
> > > > > > Sent: Tuesday, April 14, 2020 1:03 PM
> > > > > > To: devel@edk2.groups.io; Kinney, Michael D
> > > > > > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org;
> > > > > > Kinney, Michael D <michael.d.kinney@intel.com>
> > > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > > FltUsedLib for float.
> > > > > >
> > > > > > UEFI spec allows using float operation so asking module
> > > > > > developers avoid using float may not make sense. Even
> > > > > > UefiCpuPkg\Library\BaseUefiCpuLib\
> > > > > > provides routine to initialize float support for X86.
> > > > > >
> > > > > > Given ARM already uses NULL library class mechanism to supply
> > > > > > the compiler stub, I prefer X86 aligns to the ARM style.
> > > > > >
> > > > > > The only unsure thing is the size impact when a module is not
> > > > > > using float. We expect there is no size impact when a module
> > > > > > is not
> > > using float.
> > > > > >
> > > > > > Thanks,
> > > > > > Ray
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf
> > > > > > > Of
> > > > > > Michael
> > > > > > > D Kinney
> > > > > > > Sent: Thursday, April 2, 2020 12:38 AM
> > > > > > > To: devel@edk2.groups.io; ard.biesheuvel@linaro.org; Kinney,
> > > > > > > Michael D <michael.d.kinney@intel.com>
> > > > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > > > FltUsedLib for float.
> > > > > > >
> > > > > > > Hi Ard,
> > > > > > >
> > > > > > > I think adding a dependency in the module entry point libs
> > > > > > > is also good way to guarantee an intrinsic lib is available.
> > > > > > > I agree that it can provide module type and arch specific
> > > > > > > mappings.  The NULL lib instance can do the same if the NULL
> > > > > > > instance is listed in the module/arch specific library
> > > > > > > classes sections. a few example section names.
> > > > > > >
> > > > > > > [LibraryClasses.ARM.PEIM, LibraryClasses.AARCH64.PEIM]
> > > > > > >
> > > > > > > [LibraryClasses.IA32.UEFI_DRIVER]
> > > > > > >
> > > > > > > [LibraryClasses.X64.DXE_DRIVER]
> > > > > > >
> > > > > > > So either way, the DSC files need to provide the library mapping.
> > > > > > > The only difference between these
> > > > > > > 2 approaches is that adding a dependency to the module entry
> > > > > > > point libs uses a formally defined library class name for
> > > > > > > the intrinsic functions vs.
> > > > > > > the un-named NULL library instance.  A formally defined
> > > > > > > library class name is better supported for things like unit
> > > > > > > tests from the UnitTestFrameworkPkg.
> > > > > > >
> > > > > > > The fltused issue would not go away of we removed the float
> > > > > > > usage for OpenSSL.  One or more other libs or the module
> > > > > > > could use float/double types and this issue would pop up
> > > > > > > again.  I do agree it would be cleaner if we could use
> > > > > > > OpenSSL without floating
> > > point.
> > > > > > >
> > > > > > > I think adding an intrinsic lib for IA32/X64 for VS20xx
> > > > > > > generation of intrinsic functions would address fltused and
> > > > > > > other common C implementation styles that generate intrinsic
> > > > > > > functions (e.g. 64-bit int math on 32-bit, structure
> > > > > > > variable assignments, and loops that fill a buffer with a
> > > > > > > const value) by VS20xx compilers.  An intrinsic lib for GCC
> > > > > > > would also help with these same common C implementation
> > > > > > > styles that generate intrinsic
> > > functions.
> > > > > > >
> > > > > > > Mike
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On
> > > > > > > > Behalf Of
> > > > > Ard
> > > > > > > > Biesheuvel
> > > > > > > > Sent: Tuesday, March 31, 2020 11:43 PM
> > > > > > > > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > > > > > > > Cc: devel@edk2.groups.io; lersek@redhat.com;
> > > > > > > > macarl@microsoft.com
> > > > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
> > > > > > > > Add FltUsedLib for float.
> > > > > > > >
> > > > > > > > On Tue, 31 Mar 2020 at 16:36, Kinney, Michael D
> > > > > > > > <michael.d.kinney@intel.com> wrote:
> > > > > > > > >
> > > > > > > > > ARM and AARCH64 have a compiler intrinsic lib that is
> > > > > > > > linked against all modules.
> > > > > > > > >
> > > > > > > > > [LibraryClasses.ARM, LibraryClasses.AARCH64]
> > > > > > > > >   #
> > > > > > > > >   # It is not possible to prevent ARM compiler calls to
> > > > > > > > generic intrinsic functions.
> > > > > > > > >   # This library provides the instrinsic functions
> > > > > > > > generated by a given compiler.
> > > > > > > > >   # [LibraryClasses.ARM] and NULL mean link this
> > > > > > > > library into all ARM images.
> > > > > > > > >   #
> > > > > > > > >
> > > > > > > > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrins
> > > > > > > > icsLib.inf
> > > > > > > > >
> > > > > > > > > Can we use this same technique for IA32/X64 VS builds?
> > > > > > > > >
> > > > > > > >
> > > > > > > > In my opinion, having these intrinsics libraries added via
> > > > > > > > NULL library class resolution was a mistake to begin with.
> > > > > > > >
> > > > > > > > Every component that we build incorporates some kind of
> > > > > > > > startup code library that defines the _ModuleEntryPoint
> > > > > > > > symbol, and it would be much better to make those
> > > > > > > > libraries include an IntrinsicsLibrary dependency that can
> > > > > > > > be satisfied by arch specific versions that also
> > > > > > > > encapsulate the toolchain dependencies (such as this
> > > > > > > > _fltused symbol, or memcpy/memset
> > > on ARM/GCC).
> > > > > > > >
> > > > > > > > On another note, it is still deeply disappointing that we
> > > > > > > > need to jump through all of these hoops because the *only*
> > > > > > > > single use of floating point in OpenSSL is the entropy
> > > > > > > > estimate of an RNG, which is in the range of 0..1023 to begin
> with [IIRC).
> > > > > > > > Remember that we also have a softfloat submodule for
> > > > > > > > 32-bit ARM, for the same stupid reason. If we could stop
> > > > > > > > pulling in that part of the code (or replace it with an
> > > > > > > > UINT16 when building for the UEFI target), the whole
> > > > > > > > floating point issue would mostly go away AFAICT.
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
> >
> > 

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

[Liming Gao]

Guomin:
  The size impact is small. This solution that moves _fltused to BaseLib is OK to me. 

Thanks
Liming
> -----Original Message-----
> From: Jiang, Guomin <guomin.jiang@intel.com>
> Sent: Friday, April 24, 2020 1:08 PM
> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> Cc: lersek@redhat.com; macarl@microsoft.com
> Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.
> 
> I have compare the Image different, the are two image size will be affect before and after move _fltused to BaseLib.
> 
> One is TcgPei, another is SecurityStubDxe, the detail as below table
>                                TcgPei                    | SecurityStubDxe
> Before move:       22688(decimal)   |   30624(decimal)
> After move           22656(decimal)   |   30592(decimal)
> 
> The size of reducation is 32 or 0x20 bytes
> Those component is located the boundary, so it will occupy more size to meet the alignment.
> 
> Another question is that some component will fail in CI result when move _fltused to BaseLib.
> I am checking it.
> 
> > -----Original Message-----
> > From: Gao, Liming <liming.gao@intel.com>
> > Sent: Thursday, April 23, 2020 1:49 PM
> > To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> > ard.biesheuvel@linaro.org
> > Cc: lersek@redhat.com; macarl@microsoft.com
> > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> > float.
> >
> > Guomin:
> >   You can count the size of all generated EFI images for IA32 and X64, then
> > compare them between two builds. This change should impact EFI image
> > only. Based on this data, we can know the image size impact.
> >
> > Thanks
> > Liming
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Guomin
> > > Jiang
> > > Sent: Thursday, April 23, 2020 12:04 PM
> > > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Kinney, Michael
> > > D <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib
> > for float.
> > >
> > > I guess the key point is /GL option. IntrinsicLib omit the /GL option
> > > to avoid the build error, but it abandon the optimization meanwhile.
> > > I will do a test: create a simplest application and just depend
> > > IntrinsicLib and add /GL option to compare the different with and without
> > /GL.
> > >
> > > > -----Original Message-----
> > > > From: Ni, Ray <ray.ni@intel.com>
> > > > Sent: Thursday, April 23, 2020 11:32 AM
> > > > To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io;
> > > > Kinney, Michael D <michael.d.kinney@intel.com>;
> > > > ard.biesheuvel@linaro.org
> > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > FltUsedLib for float.
> > > >
> > > > Guomin,
> > > > Can you investigate why moving _fltused from CryptoPkg/IntrinsicLib
> > > > to MdePkg/BaseLib saves size?
> > > >
> > > > Thanks,
> > > > Ray
> > > >
> > > > > -----Original Message-----
> > > > > From: Jiang, Guomin <guomin.jiang@intel.com>
> > > > > Sent: Thursday, April 23, 2020 9:34 AM
> > > > > To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>;
> > > > > Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> > > > > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > FltUsedLib
> > > > for float.
> > > > >
> > > > > The size comparation between with _fltused and without _fltused,
> > > > > use
> > > > OvmfPkgX64 as build target
> > > > > OvmfX64  EFI_FV_TAKEN_SIZE  |   Without _fltused       | With _fltused
> > > > > DXEFV                                             |  0x46bbe0                     | 0x46bbc0
> > > > > FVMAIN_COMPACT                     |  0x12c8a0                      | 0x12c7f0
> > > > > PEIFV                                              |  0x4cae8                         | 0x4ca68
> > > > > From the result, it will occupy less size rather than more size.
> > > > >
> > > > > Code Change as below and build command is ```build -p
> > > > > OvmfPkg/OvmfPkgX64 -b DEBUG -t VS2019 -a X64 -
> > DTPM_ENABLE=TRUE```
> > > > >
> > > > > diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > > > b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > > > index 94fe341bec..c4136916d0 100644
> > > > > --- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > > > +++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > > > @@ -2,7 +2,7 @@
> > > > >    Intrinsic Memory Routines Wrapper Implementation for OpenSSL-
> > based
> > > > >    Cryptographic Library.
> > > > >
> > > > > -Copyright (c) 2010 - 2019, Intel Corporation. All rights
> > > > > reserved.<BR>
> > > > > +Copyright (c) 2010 - 2020, Intel Corporation. All rights
> > > > > +reserved.<BR>
> > > > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > >
> > > > >  **/
> > > > > @@ -13,16 +13,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > >
> > > > >  typedef UINTN  size_t;
> > > > >
> > > > > -#if defined(__GNUC__) || defined(__clang__)
> > > > > -  #define GLOBAL_USED __attribute__((used)) -#else
> > > > > -  #define GLOBAL_USED
> > > > > -#endif
> > > > > -
> > > > > -/* OpenSSL will use floating point support, and C compiler
> > > > > produces the
> > > > _fltused
> > > > > -   symbol by default. Simply define this symbol here to satisfy the linker.
> > */
> > > > > -int  GLOBAL_USED _fltused = 1;
> > > > > -
> > > > >  /* Sets buffers to a specified character */  void * memset (void
> > > > > *dest, int ch, size_t count)  { diff --git
> > > > > a/MdePkg/Library/BaseLib/BaseLib.inf
> > > > > b/MdePkg/Library/BaseLib/BaseLib.inf
> > > > > index 3586beb0ab..bab31c07c7 100644
> > > > > --- a/MdePkg/Library/BaseLib/BaseLib.inf
> > > > > +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> > > > > @@ -1,7 +1,7 @@
> > > > >  ## @file
> > > > >  #  Base Library implementation.
> > > > >  #
> > > > > -#  Copyright (c) 2007 - 2019, Intel Corporation. All rights
> > > > > reserved.<BR>
> > > > > +#  Copyright (c) 2007 - 2020, Intel Corporation. All rights
> > > > > +reserved.<BR>
> > > > >  #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights
> > > > > reserved.<BR>  #  Portions copyright (c) 2011 - 2013, ARM Ltd. All
> > > > > rights reserved.<BR>  # @@ -60,6 +60,7 @@
> > > > >    String.c
> > > > >    FilePaths.c
> > > > >    BaseLibInternals.h
> > > > > +  FltUsed.c               | MSFT
> > > > >
> > > > >  [Sources.Ia32]
> > > > >    Ia32/WriteTr.nasm
> > > > > diff --git a/MdePkg/Library/BaseLib/FltUsed.c
> > > > > b/MdePkg/Library/BaseLib/FltUsed.c
> > > > > new file mode 100644
> > > > > index 0000000000..c065594266
> > > > > --- /dev/null
> > > > > +++ b/MdePkg/Library/BaseLib/FltUsed.c
> > > > > @@ -0,0 +1,14 @@
> > > > > +/** @file
> > > > > +  Declare _fltused symbol for MSVC
> > > > > +
> > > > > +  MSVC need this symbol for float, andd it here to feed MSVC. it
> > > > > + may remove  if MSVC not need it any more.
> > > > > +
> > > > > +  Copyright (c) 2020 - 2020, Intel Corporation. All rights
> > > > > +reserved.<BR>
> > > > > +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> > > > > +
> > > > > +//
> > > > > +// Just for MSVC float
> > > > > +//
> > > > > +int _fltused = 0x1;
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > > Guomin Jiang
> > > > > > Sent: Tuesday, April 14, 2020 3:01 PM
> > > > > > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Kinney,
> > > > > > Michael D <michael.d.kinney@intel.com>;
> > > > > > ard.biesheuvel@linaro.org
> > > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > > FltUsedLib for float.
> > > > > >
> > > > > > Summarize current status:
> > > > > >
> > > > > > Problem Statement:
> > > > > > Openssl require _fltused to be defined as a constant anywhere
> > > > > > floating point is used.
> > > > > > It may use float out of edk2 tree and need _fltused, for
> > > > > > example, Microsoft’s OnScreenKeyboard and UiToolKit.
> > > > > >
> > > > > > Current Proposal as below:
> > > > > >
> > > > > > Proposal 1: Add FltUsed.c into exist library
> > > > > > Detail: Add FltUsed.c into EntryPointLib(PEIM, DXE).
> > > > > > Recommend: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > > Approve: Laszlo Ersek lersek@redhat.com
> > > > > > Netual: Michael D Kinney <michael.d.kinney@intel.com>
> > > > > > Benefit: Doesn’t need modify every .dsc description file.
> > > > > > Defection: I test that it will fail because of /GL option, the
> > > > > > error show fatal error LNK1237: during code generation, compiler
> > > > > > introduced reference to symbol '_fltused' defined in module
> > > > > > 'UefiApplicationEntryPoint.lib(FltUsed.obj)' compiled with /GL
> > > > > >
> > > > > > Proposal 2: Define NULL library
> > > > > > Recommend: Michael D Kinney michael.d.kinney@intel.com
> > > > > > Oppose: Sean sean.brogan=microsoft.com@groups.io , Ard
> > > > > > Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > > Benefit: I test it and prove that it is executable.
> > > > > > Defection: It is required that modify every description file.
> > > > > > Defection: It need be very careful that it should only apply
> > > > > > some module type(PEIM, DXE_DRIVER, UEFI_APPLICATION) rather
> > than all.
> > > > > > Defection: Build break up.
> > > > > > Action Required: Need evaluate the affection on size.
> > > > > > Consideration: Add PCD to control the feature
> > > > > >
> > > > > > Proposal 3: Define FltUsedLib
> > > > > > Detail: Define FltUsedLib and add dependence
> > > > > > Oppose: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > > Benefit: Doesn’t need care that which module will use it, we
> > > > > > will explicitly point it in component file.
> > > > > > Defection: More complex, It is required that modify every
> > > > > > description file and modify component meanwhile.
> > > > > > Defection: Build breakup
> > > > > >
> > > > > > Thanks
> > > > > > guomin
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > > > Ni,
> > > > > > Ray
> > > > > > > Sent: Tuesday, April 14, 2020 1:03 PM
> > > > > > > To: devel@edk2.groups.io; Kinney, Michael D
> > > > > > > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org;
> > > > > > > Kinney, Michael D <michael.d.kinney@intel.com>
> > > > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > > > FltUsedLib for float.
> > > > > > >
> > > > > > > UEFI spec allows using float operation so asking module
> > > > > > > developers avoid using float may not make sense. Even
> > > > > > > UefiCpuPkg\Library\BaseUefiCpuLib\
> > > > > > > provides routine to initialize float support for X86.
> > > > > > >
> > > > > > > Given ARM already uses NULL library class mechanism to supply
> > > > > > > the compiler stub, I prefer X86 aligns to the ARM style.
> > > > > > >
> > > > > > > The only unsure thing is the size impact when a module is not
> > > > > > > using float. We expect there is no size impact when a module
> > > > > > > is not
> > > > using float.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Ray
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf
> > > > > > > > Of
> > > > > > > Michael
> > > > > > > > D Kinney
> > > > > > > > Sent: Thursday, April 2, 2020 12:38 AM
> > > > > > > > To: devel@edk2.groups.io; ard.biesheuvel@linaro.org; Kinney,
> > > > > > > > Michael D <michael.d.kinney@intel.com>
> > > > > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > > > > FltUsedLib for float.
> > > > > > > >
> > > > > > > > Hi Ard,
> > > > > > > >
> > > > > > > > I think adding a dependency in the module entry point libs
> > > > > > > > is also good way to guarantee an intrinsic lib is available.
> > > > > > > > I agree that it can provide module type and arch specific
> > > > > > > > mappings.  The NULL lib instance can do the same if the NULL
> > > > > > > > instance is listed in the module/arch specific library
> > > > > > > > classes sections. a few example section names.
> > > > > > > >
> > > > > > > > [LibraryClasses.ARM.PEIM, LibraryClasses.AARCH64.PEIM]
> > > > > > > >
> > > > > > > > [LibraryClasses.IA32.UEFI_DRIVER]
> > > > > > > >
> > > > > > > > [LibraryClasses.X64.DXE_DRIVER]
> > > > > > > >
> > > > > > > > So either way, the DSC files need to provide the library mapping.
> > > > > > > > The only difference between these
> > > > > > > > 2 approaches is that adding a dependency to the module entry
> > > > > > > > point libs uses a formally defined library class name for
> > > > > > > > the intrinsic functions vs.
> > > > > > > > the un-named NULL library instance.  A formally defined
> > > > > > > > library class name is better supported for things like unit
> > > > > > > > tests from the UnitTestFrameworkPkg.
> > > > > > > >
> > > > > > > > The fltused issue would not go away of we removed the float
> > > > > > > > usage for OpenSSL.  One or more other libs or the module
> > > > > > > > could use float/double types and this issue would pop up
> > > > > > > > again.  I do agree it would be cleaner if we could use
> > > > > > > > OpenSSL without floating
> > > > point.
> > > > > > > >
> > > > > > > > I think adding an intrinsic lib for IA32/X64 for VS20xx
> > > > > > > > generation of intrinsic functions would address fltused and
> > > > > > > > other common C implementation styles that generate intrinsic
> > > > > > > > functions (e.g. 64-bit int math on 32-bit, structure
> > > > > > > > variable assignments, and loops that fill a buffer with a
> > > > > > > > const value) by VS20xx compilers.  An intrinsic lib for GCC
> > > > > > > > would also help with these same common C implementation
> > > > > > > > styles that generate intrinsic
> > > > functions.
> > > > > > > >
> > > > > > > > Mike
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On
> > > > > > > > > Behalf Of
> > > > > > Ard
> > > > > > > > > Biesheuvel
> > > > > > > > > Sent: Tuesday, March 31, 2020 11:43 PM
> > > > > > > > > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > > > > > > > > Cc: devel@edk2.groups.io; lersek@redhat.com;
> > > > > > > > > macarl@microsoft.com
> > > > > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
> > > > > > > > > Add FltUsedLib for float.
> > > > > > > > >
> > > > > > > > > On Tue, 31 Mar 2020 at 16:36, Kinney, Michael D
> > > > > > > > > <michael.d.kinney@intel.com> wrote:
> > > > > > > > > >
> > > > > > > > > > ARM and AARCH64 have a compiler intrinsic lib that is
> > > > > > > > > linked against all modules.
> > > > > > > > > >
> > > > > > > > > > [LibraryClasses.ARM, LibraryClasses.AARCH64]
> > > > > > > > > >   #
> > > > > > > > > >   # It is not possible to prevent ARM compiler calls to
> > > > > > > > > generic intrinsic functions.
> > > > > > > > > >   # This library provides the instrinsic functions
> > > > > > > > > generated by a given compiler.
> > > > > > > > > >   # [LibraryClasses.ARM] and NULL mean link this
> > > > > > > > > library into all ARM images.
> > > > > > > > > >   #
> > > > > > > > > >
> > > > > > > > > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrins
> > > > > > > > > icsLib.inf
> > > > > > > > > >
> > > > > > > > > > Can we use this same technique for IA32/X64 VS builds?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > In my opinion, having these intrinsics libraries added via
> > > > > > > > > NULL library class resolution was a mistake to begin with.
> > > > > > > > >
> > > > > > > > > Every component that we build incorporates some kind of
> > > > > > > > > startup code library that defines the _ModuleEntryPoint
> > > > > > > > > symbol, and it would be much better to make those
> > > > > > > > > libraries include an IntrinsicsLibrary dependency that can
> > > > > > > > > be satisfied by arch specific versions that also
> > > > > > > > > encapsulate the toolchain dependencies (such as this
> > > > > > > > > _fltused symbol, or memcpy/memset
> > > > on ARM/GCC).
> > > > > > > > >
> > > > > > > > > On another note, it is still deeply disappointing that we
> > > > > > > > > need to jump through all of these hoops because the *only*
> > > > > > > > > single use of floating point in OpenSSL is the entropy
> > > > > > > > > estimate of an RNG, which is in the range of 0..1023 to begin
> > with [IIRC).
> > > > > > > > > Remember that we also have a softfloat submodule for
> > > > > > > > > 32-bit ARM, for the same stupid reason. If we could stop
> > > > > > > > > pulling in that part of the code (or replace it with an
> > > > > > > > > UINT16 when building for the UEFI target), the whole
> > > > > > > > > floating point issue would mostly go away AFAICT.
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > >
> > > 

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

[Ni, Ray]

I may miss some discussions.
I remember the original discussion output was to create a compiler stub for x86.
Why now we trend to move to BaseLib?

> -----Original Message-----
> From: Gao, Liming <liming.gao@intel.com>
> Sent: Sunday, April 26, 2020 11:33 PM
> To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io; Ni, Ray
> <ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> ard.biesheuvel@linaro.org
> Cc: lersek@redhat.com; macarl@microsoft.com
> Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> float.
> 
> Guomin:
>   The size impact is small. This solution that moves _fltused to BaseLib is OK to
> me.
> 
> Thanks
> Liming
> > -----Original Message-----
> > From: Jiang, Guomin <guomin.jiang@intel.com>
> > Sent: Friday, April 24, 2020 1:08 PM
> > To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Ni, Ray
> <ray.ni@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > Cc: lersek@redhat.com; macarl@microsoft.com
> > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> float.
> >
> > I have compare the Image different, the are two image size will be affect
> before and after move _fltused to BaseLib.
> >
> > One is TcgPei, another is SecurityStubDxe, the detail as below table
> >                                TcgPei                    | SecurityStubDxe
> > Before move:       22688(decimal)   |   30624(decimal)
> > After move           22656(decimal)   |   30592(decimal)
> >
> > The size of reducation is 32 or 0x20 bytes
> > Those component is located the boundary, so it will occupy more size to meet
> the alignment.
> >
> > Another question is that some component will fail in CI result when move
> _fltused to BaseLib.
> > I am checking it.
> >
> > > -----Original Message-----
> > > From: Gao, Liming <liming.gao@intel.com>
> > > Sent: Thursday, April 23, 2020 1:49 PM
> > > To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>; Ni,
> Ray
> > > <ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> > > ard.biesheuvel@linaro.org
> > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> > > float.
> > >
> > > Guomin:
> > >   You can count the size of all generated EFI images for IA32 and X64, then
> > > compare them between two builds. This change should impact EFI image
> > > only. Based on this data, we can know the image size impact.
> > >
> > > Thanks
> > > Liming
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Guomin
> > > > Jiang
> > > > Sent: Thursday, April 23, 2020 12:04 PM
> > > > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Kinney, Michael
> > > > D <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib
> > > for float.
> > > >
> > > > I guess the key point is /GL option. IntrinsicLib omit the /GL option
> > > > to avoid the build error, but it abandon the optimization meanwhile.
> > > > I will do a test: create a simplest application and just depend
> > > > IntrinsicLib and add /GL option to compare the different with and without
> > > /GL.
> > > >
> > > > > -----Original Message-----
> > > > > From: Ni, Ray <ray.ni@intel.com>
> > > > > Sent: Thursday, April 23, 2020 11:32 AM
> > > > > To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io;
> > > > > Kinney, Michael D <michael.d.kinney@intel.com>;
> > > > > ard.biesheuvel@linaro.org
> > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > FltUsedLib for float.
> > > > >
> > > > > Guomin,
> > > > > Can you investigate why moving _fltused from CryptoPkg/IntrinsicLib
> > > > > to MdePkg/BaseLib saves size?
> > > > >
> > > > > Thanks,
> > > > > Ray
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jiang, Guomin <guomin.jiang@intel.com>
> > > > > > Sent: Thursday, April 23, 2020 9:34 AM
> > > > > > To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>;
> > > > > > Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> > > > > > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > > FltUsedLib
> > > > > for float.
> > > > > >
> > > > > > The size comparation between with _fltused and without _fltused,
> > > > > > use
> > > > > OvmfPkgX64 as build target
> > > > > > OvmfX64  EFI_FV_TAKEN_SIZE  |   Without _fltused       | With _fltused
> > > > > > DXEFV                                             |  0x46bbe0                     | 0x46bbc0
> > > > > > FVMAIN_COMPACT                     |  0x12c8a0                      | 0x12c7f0
> > > > > > PEIFV                                              |  0x4cae8                         | 0x4ca68
> > > > > > From the result, it will occupy less size rather than more size.
> > > > > >
> > > > > > Code Change as below and build command is ```build -p
> > > > > > OvmfPkg/OvmfPkgX64 -b DEBUG -t VS2019 -a X64 -
> > > DTPM_ENABLE=TRUE```
> > > > > >
> > > > > > diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > > > > b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > > > > index 94fe341bec..c4136916d0 100644
> > > > > > --- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > > > > +++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > > > > @@ -2,7 +2,7 @@
> > > > > >    Intrinsic Memory Routines Wrapper Implementation for OpenSSL-
> > > based
> > > > > >    Cryptographic Library.
> > > > > >
> > > > > > -Copyright (c) 2010 - 2019, Intel Corporation. All rights
> > > > > > reserved.<BR>
> > > > > > +Copyright (c) 2010 - 2020, Intel Corporation. All rights
> > > > > > +reserved.<BR>
> > > > > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > > >
> > > > > >  **/
> > > > > > @@ -13,16 +13,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > > >
> > > > > >  typedef UINTN  size_t;
> > > > > >
> > > > > > -#if defined(__GNUC__) || defined(__clang__)
> > > > > > -  #define GLOBAL_USED __attribute__((used)) -#else
> > > > > > -  #define GLOBAL_USED
> > > > > > -#endif
> > > > > > -
> > > > > > -/* OpenSSL will use floating point support, and C compiler
> > > > > > produces the
> > > > > _fltused
> > > > > > -   symbol by default. Simply define this symbol here to satisfy the
> linker.
> > > */
> > > > > > -int  GLOBAL_USED _fltused = 1;
> > > > > > -
> > > > > >  /* Sets buffers to a specified character */  void * memset (void
> > > > > > *dest, int ch, size_t count)  { diff --git
> > > > > > a/MdePkg/Library/BaseLib/BaseLib.inf
> > > > > > b/MdePkg/Library/BaseLib/BaseLib.inf
> > > > > > index 3586beb0ab..bab31c07c7 100644
> > > > > > --- a/MdePkg/Library/BaseLib/BaseLib.inf
> > > > > > +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> > > > > > @@ -1,7 +1,7 @@
> > > > > >  ## @file
> > > > > >  #  Base Library implementation.
> > > > > >  #
> > > > > > -#  Copyright (c) 2007 - 2019, Intel Corporation. All rights
> > > > > > reserved.<BR>
> > > > > > +#  Copyright (c) 2007 - 2020, Intel Corporation. All rights
> > > > > > +reserved.<BR>
> > > > > >  #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights
> > > > > > reserved.<BR>  #  Portions copyright (c) 2011 - 2013, ARM Ltd. All
> > > > > > rights reserved.<BR>  # @@ -60,6 +60,7 @@
> > > > > >    String.c
> > > > > >    FilePaths.c
> > > > > >    BaseLibInternals.h
> > > > > > +  FltUsed.c               | MSFT
> > > > > >
> > > > > >  [Sources.Ia32]
> > > > > >    Ia32/WriteTr.nasm
> > > > > > diff --git a/MdePkg/Library/BaseLib/FltUsed.c
> > > > > > b/MdePkg/Library/BaseLib/FltUsed.c
> > > > > > new file mode 100644
> > > > > > index 0000000000..c065594266
> > > > > > --- /dev/null
> > > > > > +++ b/MdePkg/Library/BaseLib/FltUsed.c
> > > > > > @@ -0,0 +1,14 @@
> > > > > > +/** @file
> > > > > > +  Declare _fltused symbol for MSVC
> > > > > > +
> > > > > > +  MSVC need this symbol for float, andd it here to feed MSVC. it
> > > > > > + may remove  if MSVC not need it any more.
> > > > > > +
> > > > > > +  Copyright (c) 2020 - 2020, Intel Corporation. All rights
> > > > > > +reserved.<BR>
> > > > > > +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> > > > > > +
> > > > > > +//
> > > > > > +// Just for MSVC float
> > > > > > +//
> > > > > > +int _fltused = 0x1;
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > > > Guomin Jiang
> > > > > > > Sent: Tuesday, April 14, 2020 3:01 PM
> > > > > > > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Kinney,
> > > > > > > Michael D <michael.d.kinney@intel.com>;
> > > > > > > ard.biesheuvel@linaro.org
> > > > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > > > FltUsedLib for float.
> > > > > > >
> > > > > > > Summarize current status:
> > > > > > >
> > > > > > > Problem Statement:
> > > > > > > Openssl require _fltused to be defined as a constant anywhere
> > > > > > > floating point is used.
> > > > > > > It may use float out of edk2 tree and need _fltused, for
> > > > > > > example, Microsoft’s OnScreenKeyboard and UiToolKit.
> > > > > > >
> > > > > > > Current Proposal as below:
> > > > > > >
> > > > > > > Proposal 1: Add FltUsed.c into exist library
> > > > > > > Detail: Add FltUsed.c into EntryPointLib(PEIM, DXE).
> > > > > > > Recommend: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > > > Approve: Laszlo Ersek lersek@redhat.com
> > > > > > > Netual: Michael D Kinney <michael.d.kinney@intel.com>
> > > > > > > Benefit: Doesn’t need modify every .dsc description file.
> > > > > > > Defection: I test that it will fail because of /GL option, the
> > > > > > > error show fatal error LNK1237: during code generation, compiler
> > > > > > > introduced reference to symbol '_fltused' defined in module
> > > > > > > 'UefiApplicationEntryPoint.lib(FltUsed.obj)' compiled with /GL
> > > > > > >
> > > > > > > Proposal 2: Define NULL library
> > > > > > > Recommend: Michael D Kinney michael.d.kinney@intel.com
> > > > > > > Oppose: Sean sean.brogan=microsoft.com@groups.io , Ard
> > > > > > > Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > > > Benefit: I test it and prove that it is executable.
> > > > > > > Defection: It is required that modify every description file.
> > > > > > > Defection: It need be very careful that it should only apply
> > > > > > > some module type(PEIM, DXE_DRIVER, UEFI_APPLICATION) rather
> > > than all.
> > > > > > > Defection: Build break up.
> > > > > > > Action Required: Need evaluate the affection on size.
> > > > > > > Consideration: Add PCD to control the feature
> > > > > > >
> > > > > > > Proposal 3: Define FltUsedLib
> > > > > > > Detail: Define FltUsedLib and add dependence
> > > > > > > Oppose: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > > > Benefit: Doesn’t need care that which module will use it, we
> > > > > > > will explicitly point it in component file.
> > > > > > > Defection: More complex, It is required that modify every
> > > > > > > description file and modify component meanwhile.
> > > > > > > Defection: Build breakup
> > > > > > >
> > > > > > > Thanks
> > > > > > > guomin
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf
> Of
> > > > > > > > Ni,
> > > > > > > Ray
> > > > > > > > Sent: Tuesday, April 14, 2020 1:03 PM
> > > > > > > > To: devel@edk2.groups.io; Kinney, Michael D
> > > > > > > > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org;
> > > > > > > > Kinney, Michael D <michael.d.kinney@intel.com>
> > > > > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > > > > FltUsedLib for float.
> > > > > > > >
> > > > > > > > UEFI spec allows using float operation so asking module
> > > > > > > > developers avoid using float may not make sense. Even
> > > > > > > > UefiCpuPkg\Library\BaseUefiCpuLib\
> > > > > > > > provides routine to initialize float support for X86.
> > > > > > > >
> > > > > > > > Given ARM already uses NULL library class mechanism to supply
> > > > > > > > the compiler stub, I prefer X86 aligns to the ARM style.
> > > > > > > >
> > > > > > > > The only unsure thing is the size impact when a module is not
> > > > > > > > using float. We expect there is no size impact when a module
> > > > > > > > is not
> > > > > using float.
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Ray
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf
> > > > > > > > > Of
> > > > > > > > Michael
> > > > > > > > > D Kinney
> > > > > > > > > Sent: Thursday, April 2, 2020 12:38 AM
> > > > > > > > > To: devel@edk2.groups.io; ard.biesheuvel@linaro.org; Kinney,
> > > > > > > > > Michael D <michael.d.kinney@intel.com>
> > > > > > > > > Cc: lersek@redhat.com; macarl@microsoft.com
> > > > > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > > > > > FltUsedLib for float.
> > > > > > > > >
> > > > > > > > > Hi Ard,
> > > > > > > > >
> > > > > > > > > I think adding a dependency in the module entry point libs
> > > > > > > > > is also good way to guarantee an intrinsic lib is available.
> > > > > > > > > I agree that it can provide module type and arch specific
> > > > > > > > > mappings.  The NULL lib instance can do the same if the NULL
> > > > > > > > > instance is listed in the module/arch specific library
> > > > > > > > > classes sections. a few example section names.
> > > > > > > > >
> > > > > > > > > [LibraryClasses.ARM.PEIM, LibraryClasses.AARCH64.PEIM]
> > > > > > > > >
> > > > > > > > > [LibraryClasses.IA32.UEFI_DRIVER]
> > > > > > > > >
> > > > > > > > > [LibraryClasses.X64.DXE_DRIVER]
> > > > > > > > >
> > > > > > > > > So either way, the DSC files need to provide the library mapping.
> > > > > > > > > The only difference between these
> > > > > > > > > 2 approaches is that adding a dependency to the module entry
> > > > > > > > > point libs uses a formally defined library class name for
> > > > > > > > > the intrinsic functions vs.
> > > > > > > > > the un-named NULL library instance.  A formally defined
> > > > > > > > > library class name is better supported for things like unit
> > > > > > > > > tests from the UnitTestFrameworkPkg.
> > > > > > > > >
> > > > > > > > > The fltused issue would not go away of we removed the float
> > > > > > > > > usage for OpenSSL.  One or more other libs or the module
> > > > > > > > > could use float/double types and this issue would pop up
> > > > > > > > > again.  I do agree it would be cleaner if we could use
> > > > > > > > > OpenSSL without floating
> > > > > point.
> > > > > > > > >
> > > > > > > > > I think adding an intrinsic lib for IA32/X64 for VS20xx
> > > > > > > > > generation of intrinsic functions would address fltused and
> > > > > > > > > other common C implementation styles that generate intrinsic
> > > > > > > > > functions (e.g. 64-bit int math on 32-bit, structure
> > > > > > > > > variable assignments, and loops that fill a buffer with a
> > > > > > > > > const value) by VS20xx compilers.  An intrinsic lib for GCC
> > > > > > > > > would also help with these same common C implementation
> > > > > > > > > styles that generate intrinsic
> > > > > functions.
> > > > > > > > >
> > > > > > > > > Mike
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On
> > > > > > > > > > Behalf Of
> > > > > > > Ard
> > > > > > > > > > Biesheuvel
> > > > > > > > > > Sent: Tuesday, March 31, 2020 11:43 PM
> > > > > > > > > > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > > > > > > > > > Cc: devel@edk2.groups.io; lersek@redhat.com;
> > > > > > > > > > macarl@microsoft.com
> > > > > > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
> > > > > > > > > > Add FltUsedLib for float.
> > > > > > > > > >
> > > > > > > > > > On Tue, 31 Mar 2020 at 16:36, Kinney, Michael D
> > > > > > > > > > <michael.d.kinney@intel.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > ARM and AARCH64 have a compiler intrinsic lib that is
> > > > > > > > > > linked against all modules.
> > > > > > > > > > >
> > > > > > > > > > > [LibraryClasses.ARM, LibraryClasses.AARCH64]
> > > > > > > > > > >   #
> > > > > > > > > > >   # It is not possible to prevent ARM compiler calls to
> > > > > > > > > > generic intrinsic functions.
> > > > > > > > > > >   # This library provides the instrinsic functions
> > > > > > > > > > generated by a given compiler.
> > > > > > > > > > >   # [LibraryClasses.ARM] and NULL mean link this
> > > > > > > > > > library into all ARM images.
> > > > > > > > > > >   #
> > > > > > > > > > >
> > > > > > > > > > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrins
> > > > > > > > > > icsLib.inf
> > > > > > > > > > >
> > > > > > > > > > > Can we use this same technique for IA32/X64 VS builds?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > In my opinion, having these intrinsics libraries added via
> > > > > > > > > > NULL library class resolution was a mistake to begin with.
> > > > > > > > > >
> > > > > > > > > > Every component that we build incorporates some kind of
> > > > > > > > > > startup code library that defines the _ModuleEntryPoint
> > > > > > > > > > symbol, and it would be much better to make those
> > > > > > > > > > libraries include an IntrinsicsLibrary dependency that can
> > > > > > > > > > be satisfied by arch specific versions that also
> > > > > > > > > > encapsulate the toolchain dependencies (such as this
> > > > > > > > > > _fltused symbol, or memcpy/memset
> > > > > on ARM/GCC).
> > > > > > > > > >
> > > > > > > > > > On another note, it is still deeply disappointing that we
> > > > > > > > > > need to jump through all of these hoops because the *only*
> > > > > > > > > > single use of floating point in OpenSSL is the entropy
> > > > > > > > > > estimate of an RNG, which is in the range of 0..1023 to begin
> > > with [IIRC).
> > > > > > > > > > Remember that we also have a softfloat submodule for
> > > > > > > > > > 32-bit ARM, for the same stupid reason. If we could stop
> > > > > > > > > > pulling in that part of the code (or replace it with an
> > > > > > > > > > UINT16 when building for the UEFI target), the whole
> > > > > > > > > > floating point issue would mostly go away AFAICT.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > > > 
> 

Re: [edk2-devel] [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

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

[Ard Biesheuvel]

On Tue, 31 Mar 2020 at 03:40, Jiang, Guomin <guomin.jiang@intel.com> wrote:
>
> 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
>

Doesn't the build error go away as well if you simply add FloatUsed.c
to all BaseCryptLib flavours if Visual Studio is being used?

Something like

diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
index 1bbe4f435aec..f40bf18e7f5d 100644
--- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -27,6 +27,7 @@ [Defines]
 #

 [Sources]
+  FloatUsed.c         | MSFT
   InternalCryptLib.h
   Hash/CryptMd4.c
   Hash/CryptMd5.c

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

[Laszlo Ersek]

On 03/31/20 09:13, Ard Biesheuvel wrote:
> On Tue, 31 Mar 2020 at 03:40, Jiang, Guomin <guomin.jiang@intel.com> wrote:
>>
>> 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
>>
> 
> Doesn't the build error go away as well if you simply add FloatUsed.c
> to all BaseCryptLib flavours if Visual Studio is being used?
> 
> Something like
> 
> diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> index 1bbe4f435aec..f40bf18e7f5d 100644
> --- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> @@ -27,6 +27,7 @@ [Defines]
>  #
> 
>  [Sources]
> +  FloatUsed.c         | MSFT
>    InternalCryptLib.h
>    Hash/CryptMd4.c
>    Hash/CryptMd5.c
> 

This is *exactly* what I've had in mind!

(With the additional (optional) tweak that IMO "FloatUsed.c" belongs
with the openssl INF files, as those are where the floating point usage
originates from. The cryptlib instances themselves have no floating
point code, AIUI.)

Thanks
Laszlo

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

[Bret Barkelew]

[-- Attachment #1: Type: text/plain, Size: 8308 bytes --]

Reviewed-by: Bret Barkelew <bret.barkelew@microsoft.com>

- Bret

________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Guomin Jiang via Groups.Io <guomin.jiang=intel.com@groups.io>
Sent: Monday, March 30, 2020 1:52:47 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Jian J Wang <jian.j.wang@intel.com>; Xiaoyu Lu <xiaoyux.lu@intel.com>
Subject: [EXTERNAL] [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.

REF: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2596&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7Cdcda1866343d4d6dab3908d7d487ba8e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637211551721384527&amp;sdata=LCgdHl1VW1DIGzMCei5PDRP7UQtD%2FERzkumrbgpYuJs%3D&amp;reserved=0

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>
---
 CryptoPkg/CryptoPkg.dsc                       |  2 ++
 .../Library/BaseCryptLib/BaseCryptLib.inf     |  1 +
 .../Library/BaseCryptLib/PeiCryptLib.inf      |  1 +
 .../Library/BaseCryptLib/RuntimeCryptLib.inf  |  1 +
 .../Library/BaseCryptLib/SmmCryptLib.inf      |  1 +
 CryptoPkg/Library/FltUsedLib/FltUsedLib.c     | 14 ++++++++
 CryptoPkg/Library/FltUsedLib/FltUsedLib.inf   | 33 +++++++++++++++++++
 CryptoPkg/Library/TlsLib/TlsLib.inf           |  1 +
 8 files changed, 54 insertions(+)
 create mode 100644 CryptoPkg/Library/FltUsedLib/FltUsedLib.c
 create mode 100644 CryptoPkg/Library/FltUsedLib/FltUsedLib.inf

diff --git a/CryptoPkg/CryptoPkg.dsc b/CryptoPkg/CryptoPkg.dsc
index 4cb37b1349..e9854087b5 100644
--- a/CryptoPkg/CryptoPkg.dsc
+++ b/CryptoPkg/CryptoPkg.dsc
@@ -97,6 +97,7 @@
   IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf                                          #???

   OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf

   IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf

+  FltUsedLib|CryptoPkg/Library/FltUsedLib/FltUsedLib.inf

   SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf



 [LibraryClasses.ARM]

@@ -232,6 +233,7 @@
   CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf

   CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf

   CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf

+  CryptoPkg/Library/FltUsedLib/FltUsedLib.inf

   CryptoPkg/Library/TlsLib/TlsLib.inf

   CryptoPkg/Library/TlsLibNull/TlsLibNull.inf

   CryptoPkg/Library/OpensslLib/OpensslLib.inf

diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
index 1bbe4f435a..c82e703aa0 100644
--- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -84,6 +84,7 @@
   DebugLib

   OpensslLib

   IntrinsicLib

+  FltUsedLib

   PrintLib



 #

diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
index c836c257f8..342aad008c 100644
--- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
@@ -78,6 +78,7 @@
   DebugLib

   OpensslLib

   IntrinsicLib

+  FltUsedLib



 #

 # Remove these [BuildOptions] after this library is cleaned up

diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
index bff308a4f5..dcf209d7f5 100644
--- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
@@ -89,6 +89,7 @@
   DebugLib

   OpensslLib

   IntrinsicLib

+  FltUsedLib

   PrintLib



 #

diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
index cc0b65fd25..7fdaaa48a6 100644
--- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
@@ -88,6 +88,7 @@
   MemoryAllocationLib

   OpensslLib

   IntrinsicLib

+  FltUsedLib

   PrintLib



 #

diff --git a/CryptoPkg/Library/FltUsedLib/FltUsedLib.c b/CryptoPkg/Library/FltUsedLib/FltUsedLib.c
new file mode 100644
index 0000000000..8f2487ea13
--- /dev/null
+++ b/CryptoPkg/Library/FltUsedLib/FltUsedLib.c
@@ -0,0 +1,14 @@
+/** @file

+  Need include this when use floats.

+

+  Copyright (C) Microsoft Corporation. All rights reserved.<BR>

+  Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>

+  SPDX-License-Identifier: BSD-2-Clause-Patent

+

+**/

+

+//

+// You need to include this to let the compiler know we are going to use

+// floating point

+//

+int _fltused = 0x9875;

diff --git a/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf b/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
new file mode 100644
index 0000000000..8d67eadfa5
--- /dev/null
+++ b/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
@@ -0,0 +1,33 @@
+## @file

+# It is depent on when using floats

+#

+# Copyright (C) Microsoft Corporation. All rights reserved.<BR>

+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>

+# SPDX-License-Identifier: BSD-2-Clause-Patent

+#

+##

+

+[Defines]

+  INF_VERSION                    = 0x00010005

+  BASE_NAME                      = FltUsedLib

+  FILE_GUID                      = C004F180-9FE2-4D2B-8318-BADC2A231774

+  MODULE_TYPE                    = BASE

+  VERSION_STRING                 = 1.0

+  LIBRARY_CLASS                  = FltUsedLib

+

+#

+# The following information is for reference only and not required by the build tools.

+#

+#  VALID_ARCHITECTURES           = IA32 X64 AARCH64

+#

+

+[Sources]

+  FltUsedLib.c

+

+[Packages]

+  MdePkg/MdePkg.dec

+

+[BuildOptions]

+  # Disable GL due to linker error LNK1237

+  # https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fcpp%2Ferror-messages%2Ftool-errors%2Flinker-tools-error-lnk1237%3Fview%3Dvs-2017&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7Cdcda1866343d4d6dab3908d7d487ba8e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637211551721384527&amp;sdata=vULhLfRubt3e7xbVLa%2BHC0E34JTcgOSbdYndn%2FxNEps%3D&amp;reserved=0

+  MSFT:*_*_*_CC_FLAGS = /GL-

diff --git a/CryptoPkg/Library/TlsLib/TlsLib.inf b/CryptoPkg/Library/TlsLib/TlsLib.inf
index 2f3ce695c3..febbdf5149 100644
--- a/CryptoPkg/Library/TlsLib/TlsLib.inf
+++ b/CryptoPkg/Library/TlsLib/TlsLib.inf
@@ -37,6 +37,7 @@
   BaseMemoryLib

   DebugLib

   IntrinsicLib

+  FltUsedLib

   MemoryAllocationLib

   OpensslLib

   SafeIntLib

--
2.25.1.windows.1


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

View/Reply Online (#56613): https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F56613&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7Cdcda1866343d4d6dab3908d7d487ba8e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637211551721384527&amp;sdata=DHVFRPBfEkbBLIgrIdwQxnGH0XjtN1oxsZYVyLDIqf4%3D&amp;reserved=0
Mute This Topic: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.io%2Fmt%2F72648022%2F1822150&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7Cdcda1866343d4d6dab3908d7d487ba8e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637211551721384527&amp;sdata=KyjsUA%2BJ4U6YdFn22%2BHhr%2F0JF5AojkVTU52vmE1zh2Q%3D&amp;reserved=0
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Funsub&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7Cdcda1866343d4d6dab3908d7d487ba8e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637211551721384527&amp;sdata=04dDAgBNEz8Ra5JuAfOVK3dvos1vhfQDma5Y2Qv4eao%3D&amp;reserved=0  [brbarkel@microsoft.com]
-=-=-=-=-=-=


[-- Attachment #2: Type: text/html, Size: 15167 bytes --]