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

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

On 05/24/19 23:32, Ard Biesheuvel wrote:
> 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.

Thanks! I'll keep this tagged and seek to do (1) and (3) unless someone
beats me to them until next Mon/Tues or so.

>> (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.

Sounds convincing, thank you.

> 
>>> 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

Heh, reported about a month ago :)

> 
>>> - 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.

If we split the SPDX IDs to a separate patch now, then we could revert
it temporarily at the time of the upgrade, apply the upgrade, and
regenerate the SPDX tags right after the upgrade. And, these three
patches could be squased into one before posting the upgrade. IOW, the
"revert" would only be visible in isolation to the developer that
implements the upgrade.

(We could also attempt to contribute the SPDX IDs to the upstream
ArmSoftFloatLib project, but you mention it doesn't have a central git
repo, so it likely doesn't follow a distributed development model.)

... I've just grown so fond of the eminent greppability of SPDX IDs :)

>>>
>>> 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.

Thanks!

> 
>>>   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.

For easy upgrades down the road, leaving these files in place, or at
least removing them with a revertible stand-alone patch, would be better
than never adding them. Anyway, I fully defer to you and Leif on this.

Thanks!
Laszlo