Re: [edk2-devel] [PATCH 0/3] update ArmSoftFloatLib to latest upstream version

["Xiaoyu Lu" <xiaoyux.lu@intel.com>]

Hi Ard,

Thanks for these patches.

I did some function tests for it.
Here is the results:
--------------- Test 1: ---------------
test uint32_to_f64 4026531839: Mem: 00 00 E0 FF FF FF ED 41
test uint32_to_f64 4294967295: Mem: 00 00 E0 FF FF FF EF 41
------------ Test 2: ---------------
Test f64_to_uint32 5294967295.1: 4294967295
Test f64_to_uint32 4294967295.1: 4294967295
Test f64_to_uint32 4294967294.8: 4294967294
Test f64_to_uint32 4294967295.2: 4294967295
Test f64_to_uint32 4294967295.8: 4294967295
Test f64_to_uint32 -40.0: 0
Test f64_to_uint32 0.0: 0
Test f64_to_uint32 0.8: 0
Test f64_to_uint32 -0.1: 0

looks fine for me.

I also did CryptoPkg tests with OpenSSL_1_1_1b, it works too.

Tested-by: Xiaoyu Lu <xiaoyux.lu@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Ard Biesheuvel
> Sent: Saturday, May 25, 2019 5:33 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Gao, Liming
> <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Leif Lindholm
> <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [PATCH 0/3] update ArmSoftFloatLib to latest
> upstream version
> 
> On Fri, 24 May 2019 at 22:57, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > Hi Ard,
> >
> > On 05/24/19 17:11, Ard Biesheuvel wrote:
> > > Currently, our move to OpenSSL 1.1.1b is being blocked by an issue in
> > > the ARM software floating point library, which lacks some intrinsics
> > > that the ARM EABI spec defines.
> > >
> > > Since the code was in pretty sorry state, let's fix this by upgrading
> > > to the very latest version of the core library this code is based on,
> > > dated January 2018 (whereas the NetBSD fork of the old code dates back
> > > to 2002)
> >
> > Thanks for this series!
> >
> > I've fetched your branch noted below, and build-tested it with
> > ArmVirtQemu, ArmVirtQemuKernel, and ArmVirtXen. They all build fine.
> > And, AIUI, ArmSoftFloatLib is only needed for 32-bit ARM (not AArch64),
> > so I won't do other than build testing now.
> >
> > Build-tested-by: Laszlo Ersek <lersek@redhat.com>
> >
> > I'll make a number of comments below. I'm not requesting that *you* do
> > any of those, since you're already doing the community a favor, by
> > putting out this fire. I'll just list what I think should be done. If
> > there's agreement, I might take on a few of those.
> >
> > (1) We should file a new TianoCore BZ (Feature Request) for this
> > ArmSoftFloatLib upgrade, and we should block TianoCore#1089 with that
> > new BZ.
> >
> > (2) The new BZ should be referenced in all of the commit messages.
> >
> > (3) The new BZ should be added to the release planning wiki page.
> >
> 
> Fair enough.
> 
> > (4) In the longer term, we should investigate whether this (large)
> > library can be consumed as a git submodule. (Assuming that makes sense
> > -- if we don't expect another upgrade anytime soon, then this may not be
> > necessary.)
> >
> 
> This version of ArmSoftFloatLib implements all __aeabi routines that
> are listed in the spec. Only a few of those are referenced by OpenSSL,
> and in practice this code never gets exercised. So unless we grow
> another user of this library, I have no intention of doing lots of
> maintenance work on this library and (in response to your point below)
> this is the reason I simply imported the whole library - to make
> future upgrades, in case they do occur, as painless and
> straightforward as possible. So I think a git submodule is overkill,
> especially given the fact that there does not seem to be an
> authoritative git upstream for this library.
> 
> > > A few notable issues that may require some discussion:
> > > - this code is made available under the 3-clause BSD license
> >
> > That should be OK; "Readme.md" white-lists the 3-clause BSD License.
> >
> > > - RVCT support is being dropped, since it is untested and nobody
> > >   appears to still care.
> >
> > (5) I'm OK with that, but we should file a separate TianoCore BZ for
> > BaseTools, about RVCT removal.
> >
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1750
> 
> > > - no SPDX headers - this is left as an exercise for the steward.
> >
> > (6) Right, I've noticed that -- separate BZ (dependent on the one from
> > (2)), or else it should be solved as the fourth patch in this series.
> >
> 
> Sure. The only downside to that is that it increases the delta with
> the upstream library, so let's hope that this rebases cleanly if we do
> end up upgrading.
> 
> > >
> > > Code can be found here:
> > >
> https://github.com/ardbiesheuvel/edk2/tree/bz_1089_upgrade_to_openssl
> _1_1_1b_v4
> > >
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: "Gao, Liming" <liming.gao@intel.com>
> > > Cc: "Wang, Jian J" <jian.j.wang@intel.com>
> > > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > >
> > > Ard Biesheuvel (3):
> > >   ArmPkg: import latest version (3e) of the Berkeley Softfloat library
> > >   ArmPkg/ArmSoftFloatLib: switch to new version of softfloat library
> >
> > (7) This patch (patch#2) uses designated initializers (in the
> > initializers of the unions). I believe we never intend to build this
> > library with anything else than GCC, but I think the coding style still
> > requires us to avoid designated initializers.
> >
> 
> I can change that.
> 
> > >   ArmPkg/ArmSoftFloatLib: remove source files that are no longer used
> >
> > OK, so here's a real scientific method that I used for determining
> > whether the result of this patch was "minimal". It relies on the
> > "strictatime" mount option -- I don't tolerate "relatime" or "noatime"
> > exactly because the POSIX atime behavior is so useful for debugging file
> > access.
> >
> > So, a few minutes passed between my checking out your branch, and
> > starting the build tests. After the build tests above completed, I ran:
> >
> > $ find ArmPkg/Library/ArmSoftFloatLib/ -type f -printf '%A+ %p\n' \
> >   | sort -r
> >
> > which sorted the regular files in decreasing access time order (most
> > recent access near the top). The output suggests that the following
> > files are also not needed for the build(s) (with the
> > "ArmPkg/Library/ArmSoftFloatLib/SoftFloat-3e/" prefix stripped):
> >
> >      1  COPYING.txt
> >      2  README.html
> >      3  README.txt
> >      4  build/Linux-386-GCC/Makefile
> >      5  build/Linux-386-GCC/platform.h
> >      6  build/Linux-386-SSE2-GCC/Makefile
> >      7  build/Linux-386-SSE2-GCC/platform.h
> >      8  build/Linux-ARM-VFPv2-GCC/Makefile
> >      9  build/Linux-x86_64-GCC/Makefile
> >     10  build/Linux-x86_64-GCC/platform.h
> >     11  build/Win32-MinGW/Makefile
> >     12  build/Win32-MinGW/platform.h
> >     13  build/Win32-SSE2-MinGW/Makefile
> >     14  build/Win32-SSE2-MinGW/platform.h
> >     15  build/Win64-MinGW-w64/Makefile
> >     16  build/Win64-MinGW-w64/platform.h
> >     17  build/template-FAST_INT64/Makefile
> >     18  build/template-FAST_INT64/platform.h
> >     19  build/template-not-FAST_INT64/Makefile
> >     20  build/template-not-FAST_INT64/platform.h
> >     21  doc/SoftFloat-history.html
> >     22  doc/SoftFloat-source.html
> >     23  doc/SoftFloat.html
> >     24  source/8086-SSE/extF80M_isSignalingNaN.c
> >     25  source/8086-SSE/f128M_isSignalingNaN.c
> >     26  source/8086-SSE/s_commonNaNToExtF80M.c
> >     27  source/8086-SSE/s_commonNaNToExtF80UI.c
> >     28  source/8086-SSE/s_commonNaNToF128M.c
> >     29  source/8086-SSE/s_commonNaNToF128UI.c
> >     30  source/8086-SSE/s_commonNaNToF16UI.c
> >     31  source/8086-SSE/s_commonNaNToF32UI.c
> >     32  source/8086-SSE/s_commonNaNToF64UI.c
> >     33  source/8086-SSE/s_extF80MToCommonNaN.c
> >     34  source/8086-SSE/s_extF80UIToCommonNaN.c
> >     35  source/8086-SSE/s_f128MToCommonNaN.c
> >     36  source/8086-SSE/s_f128UIToCommonNaN.c
> >     37  source/8086-SSE/s_f16UIToCommonNaN.c
> >     38  source/8086-SSE/s_f32UIToCommonNaN.c
> >     39  source/8086-SSE/s_f64UIToCommonNaN.c
> >     40  source/8086-SSE/s_propagateNaNExtF80M.c
> >     41  source/8086-SSE/s_propagateNaNExtF80UI.c
> >     42  source/8086-SSE/s_propagateNaNF128M.c
> >     43  source/8086-SSE/s_propagateNaNF128UI.c
> >     44  source/8086-SSE/s_propagateNaNF16UI.c
> >     45  source/8086-SSE/s_propagateNaNF32UI.c
> >     46  source/8086-SSE/s_propagateNaNF64UI.c
> >     47  source/8086-SSE/softfloat_raiseFlags.c
> >     48  source/8086-SSE/specialize.h
> >     49  source/8086/extF80M_isSignalingNaN.c
> >     50  source/8086/f128M_isSignalingNaN.c
> >     51  source/8086/s_commonNaNToExtF80M.c
> >     52  source/8086/s_commonNaNToExtF80UI.c
> >     53  source/8086/s_commonNaNToF128M.c
> >     54  source/8086/s_commonNaNToF128UI.c
> >     55  source/8086/s_commonNaNToF16UI.c
> >     56  source/8086/s_commonNaNToF32UI.c
> >     57  source/8086/s_commonNaNToF64UI.c
> >     58  source/8086/s_extF80MToCommonNaN.c
> >     59  source/8086/s_extF80UIToCommonNaN.c
> >     60  source/8086/s_f128MToCommonNaN.c
> >     61  source/8086/s_f128UIToCommonNaN.c
> >     62  source/8086/s_f16UIToCommonNaN.c
> >     63  source/8086/s_f32UIToCommonNaN.c
> >     64  source/8086/s_f64UIToCommonNaN.c
> >     65  source/8086/s_propagateNaNExtF80M.c
> >     66  source/8086/s_propagateNaNExtF80UI.c
> >     67  source/8086/s_propagateNaNF128M.c
> >     68  source/8086/s_propagateNaNF128UI.c
> >     69  source/8086/s_propagateNaNF16UI.c
> >     70  source/8086/s_propagateNaNF32UI.c
> >     71  source/8086/s_propagateNaNF64UI.c
> >     72  source/8086/softfloat_raiseFlags.c
> >     73  source/8086/specialize.h
> >     74  source/ARM-VFPv2-defaultNaN/extF80M_isSignalingNaN.c
> >     75  source/ARM-VFPv2-defaultNaN/f128M_isSignalingNaN.c
> >     76  source/ARM-VFPv2-defaultNaN/s_commonNaNToExtF80M.c
> >     77  source/ARM-VFPv2-defaultNaN/s_commonNaNToExtF80UI.c
> >     78  source/ARM-VFPv2-defaultNaN/s_commonNaNToF128M.c
> >     79  source/ARM-VFPv2-defaultNaN/s_commonNaNToF128UI.c
> >     80  source/ARM-VFPv2-defaultNaN/s_commonNaNToF16UI.c
> >     81  source/ARM-VFPv2-defaultNaN/s_commonNaNToF32UI.c
> >     82  source/ARM-VFPv2-defaultNaN/s_commonNaNToF64UI.c
> >     83  source/ARM-VFPv2-defaultNaN/s_extF80MToCommonNaN.c
> >     84  source/ARM-VFPv2-defaultNaN/s_extF80UIToCommonNaN.c
> >     85  source/ARM-VFPv2-defaultNaN/s_f128MToCommonNaN.c
> >     86  source/ARM-VFPv2-defaultNaN/s_f128UIToCommonNaN.c
> >     87  source/ARM-VFPv2-defaultNaN/s_f16UIToCommonNaN.c
> >     88  source/ARM-VFPv2-defaultNaN/s_f32UIToCommonNaN.c
> >     89  source/ARM-VFPv2-defaultNaN/s_f64UIToCommonNaN.c
> >     90  source/ARM-VFPv2-defaultNaN/s_propagateNaNExtF80M.c
> >     91  source/ARM-VFPv2-defaultNaN/s_propagateNaNExtF80UI.c
> >     92  source/ARM-VFPv2-defaultNaN/s_propagateNaNF128M.c
> >     93  source/ARM-VFPv2-defaultNaN/s_propagateNaNF128UI.c
> >     94  source/ARM-VFPv2-defaultNaN/s_propagateNaNF16UI.c
> >     95  source/ARM-VFPv2-defaultNaN/s_propagateNaNF32UI.c
> >     96  source/ARM-VFPv2-defaultNaN/s_propagateNaNF64UI.c
> >     97  source/ARM-VFPv2-defaultNaN/softfloat_raiseFlags.c
> >     98  source/ARM-VFPv2-defaultNaN/specialize.h
> >     99  source/ARM-VFPv2/extF80M_isSignalingNaN.c
> >    100  source/ARM-VFPv2/f128M_isSignalingNaN.c
> >    101  source/ARM-VFPv2/s_commonNaNToExtF80M.c
> >    102  source/ARM-VFPv2/s_commonNaNToExtF80UI.c
> >    103  source/ARM-VFPv2/s_commonNaNToF128M.c
> >    104  source/ARM-VFPv2/s_commonNaNToF128UI.c
> >    105  source/ARM-VFPv2/s_commonNaNToF16UI.c
> >    106  source/ARM-VFPv2/s_commonNaNToF32UI.c
> >    107  source/ARM-VFPv2/s_commonNaNToF64UI.c
> >    108  source/ARM-VFPv2/s_extF80MToCommonNaN.c
> >    109  source/ARM-VFPv2/s_extF80UIToCommonNaN.c
> >    110  source/ARM-VFPv2/s_f128MToCommonNaN.c
> >    111  source/ARM-VFPv2/s_f128UIToCommonNaN.c
> >    112  source/ARM-VFPv2/s_f16UIToCommonNaN.c
> >    113  source/ARM-VFPv2/s_f32UIToCommonNaN.c
> >    114  source/ARM-VFPv2/s_f64UIToCommonNaN.c
> >    115  source/ARM-VFPv2/s_propagateNaNExtF80M.c
> >    116  source/ARM-VFPv2/s_propagateNaNExtF80UI.c
> >    117  source/ARM-VFPv2/s_propagateNaNF128M.c
> >    118  source/ARM-VFPv2/s_propagateNaNF128UI.c
> >    119  source/ARM-VFPv2/s_propagateNaNF16UI.c
> >    120  source/ARM-VFPv2/s_propagateNaNF32UI.c
> >
> > (8) Should we remove the "build/" (lines 4 through 20) and "source/"
> > (lines 24 through 120) subsets of this list, in patch #3? (Or maybe in a
> > totally separate patch?)
> >
> 
> I'll let Leif chime in here. I'd be fine with removing them, not
> adding them in the first place or leaving them where they are.
> 
>