From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Fri, 24 May 2019 13:57:14 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CDF663086268; Fri, 24 May 2019 20:57:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-122-122.rdu2.redhat.com [10.10.122.122]) by smtp.corp.redhat.com (Postfix) with ESMTP id D22075D9E5; Fri, 24 May 2019 20:57:11 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 0/3] update ArmSoftFloatLib to latest upstream version To: devel@edk2.groups.io, ard.biesheuvel@linaro.org Cc: "Gao, Liming" , "Wang, Jian J" , Leif Lindholm , Michael D Kinney References: <20190524151140.23539-1-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: Date: Fri, 24 May 2019 22:57:10 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190524151140.23539-1-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Fri, 24 May 2019 20:57:13 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 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. (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.) > 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. > - 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. > > Code can be found here: > https://github.com/ardbiesheuvel/edk2/tree/bz_1089_upgrade_to_openssl_1_1_1b_v4 > > Cc: Laszlo Ersek > Cc: "Gao, Liming" > Cc: "Wang, Jian J" > Cc: Leif Lindholm > Cc: Michael D Kinney > > 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. > 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?) Thanks Laszlo