From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from proxmox.maurer-it.com (proxmox.maurer-it.com [212.186.127.180]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4D4382095BB68 for ; Wed, 6 Sep 2017 08:06:08 -0700 (PDT) Received: from proxmox.maurer-it.com (localhost [127.0.0.1]) by proxmox.maurer-it.com (Proxmox) with ESMTP id C4915110E045; Wed, 6 Sep 2017 17:08:56 +0200 (CEST) To: Laszlo Ersek , edk2-devel@lists.01.org Cc: Ard Biesheuvel , "Gao, Liming" , "Zhu, Yonghong" References: <9fbd059f-18d8-a798-da00-951f85c9fd1a@proxmox.com> <554afa66-cc81-6115-ff83-2b0b6f01d455@redhat.com> <697823cf-4e21-137c-eb5c-8b4a2bce14d0@proxmox.com> <56159eae-7009-5e19-386a-bde680f27197@redhat.com> From: Thomas Lamprecht Message-ID: Date: Wed, 6 Sep 2017 17:08:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <56159eae-7009-5e19-386a-bde680f27197@redhat.com> Subject: Re: OvmfPkg/IoMmuDxe: unused-const-variable warning fails release build with GCC 6.3 X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Sep 2017 15:06:08 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 09/06/2017 04:22 PM, Laszlo Ersek wrote: > On 09/06/17 15:35, Thomas Lamprecht wrote: >> On 09/06/2017 01:15 PM, Laszlo Ersek wrote: >> [snip] >>> But gcc-5 would not recognize this option. I think we might have to >>> introduce the GCC6 toolchain for this. >>> >> >> After giving the "BaseTools/Conf/tools_def.template" another look, it >> seems that adding the target would not be the main work here, but >> testing it on all platforms and verifying that all other flags are sti= ll >> correct and that for all platforms would be, or? >> >> I could add a GCC6 one which mainly inherits from GCC5, i.e. in the sa= me >> way as GCC49 is already the "parent" for GCC5. >> But I could currently only compile and test OVMF for amd64 and aarch64= =2E >=20 > Hmm, if you can compile for X64, how can you not compile it for IA32? I= t > should not require different (native) compiler + binutils packages on > your system. >=20 Yes, I can test IA32 naturally too, just forgot to mention it. > If you are cross-compiling for AARCH64, the situation is then different= , > because for cross-compiling to ARM, separate compiler + binutils > packages could be required. >=20 As I have an APM x-gene here I can compile aarch64 natively running our Debian derivated distro. Did not configured a 32-bit environment yet, but that should be possible.= >> >> I mean from a developer perspective it seems OK to do that and add the= >> target even without full coverage and the adapt the target toolchain -= >> or the platform where appropriate,=C2=A0 on a case per case basis, onc= e the >> new toolchain will see more use there. >=20 > If you want to explore adding the new toolchain, I guess you could > submit a patch with the whole thing (all arches), and ask for help with= > testing (always specifying "-t GCC6" explicitly). I can see the > following test cases: >=20 > { DEBUG, RELEASE } x { IA32, X64, ARM, AARCH64 } x { gcc-6, gcc-7 } >=20 > 1 gcc-6 AARCH64 DEBUG > 2 gcc-6 AARCH64 RELEASE > 3 gcc-6 ARM DEBUG > 4 gcc-6 ARM RELEASE > 5 gcc-6 IA32 DEBUG > 6 gcc-6 IA32 RELEASE > 7 gcc-6 X64 DEBUG > 8 gcc-6 X64 RELEASE > 9 gcc-7 AARCH64 DEBUG > 10 gcc-7 AARCH64 RELEASE > 11 gcc-7 ARM DEBUG > 12 gcc-7 ARM RELEASE > 13 gcc-7 IA32 DEBUG > 14 gcc-7 IA32 RELEASE > 15 gcc-7 X64 DEBUG > 16 gcc-7 X64 RELEASE >=20 > You should be able to build-test cases 1-2, 5-8. >=20 > I can help you build-test cases 3-4 (already have that cross-compiler > installed). >=20 > Cases 9-16 are testable, for example, within a Fedora 26 virtual > machine. Unless someone on the list runs Fedora 26 on a permanent basis= , > the effort of setting up a VM would be the same for them as for you. > But, you might get lucky. >=20 I got a Fedora VM here already, so this should be also no problem for me then, great! > Boot testing is secondary in this case, IMO. If your build passes a > smoke test for booting, on your side, that should be sufficient. > Miscompilations due to new GCC versions have occurred, but they are > extremely time consuming to figure out, so we generally investigate the= m > when they occur, not in advance. The point is that older toolchains are= > not regressed by adding newer ones. >=20 OK. >>> Here's what I suggest: please file a TianoCore BZ about this issue, a= t >>> . Select "BaseTools" as Package. Fee= l >>> free to reference the mailing list thread in the BZ: >>> >>> https://lists.01.org/pipermail/edk2-devel/2017-September/014225.html >>> >> >> Done, I got #700 : >> >> >>> Now, addressing that will take a while. Meanwhile, can you please che= ck >>> if simply removing the CONST suppresses the issue? (Note, the CONST >>> should be removed from the "mBusMasterOperationName" variable, *not* >>> from the pointed-to CHAR8.) >>> >>> If that works, can you please submit a patch like this? >>> >> >> No, it does not work, it then does not falls under the >> "-Wno-unused-but-set-variable" case as its static, if I understand the= gcc >> Warning Options doc correctly: >> >> =C2=A0> -Wunused-but-set-variable: >> =C2=A0>=C2=A0=C2=A0=C2=A0=C2=A0 Warn whenever a local variable is ass= igned to, ... >> >> Whereas: >> >> =C2=A0> -Wunused-variable >> =C2=A0>=C2=A0=C2=A0=C2=A0=C2=A0 Warn whenever a local or static varia= ble is unused aside from its >> =C2=A0>=C2=A0=C2=A0=C2=A0=C2=A0 declaration. This option implies -Wun= used-const-variable=3D1 for C, >> ... >> >> So "no-unused-but-set" allows the local variable case but not the stat= ic >> variable case. >> >> >>>> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c >>>> b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c >>>> index bc57de5b572b..45d1c909b5ad 100644 >>>> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c >>>> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c >>>> @@ -47,7 +47,9 @@ STATIC LIST_ENTRY mRecycledMapInfos =3D >>>> INITIALIZE_LIST_HEAD_VARIABLE ( >>>> =C2=A0 // >>>> =C2=A0 // ASCII names for EDKII_IOMMU_OPERATION constants, for debu= g logging. >>>> =C2=A0 // >>>> -STATIC CONST CHAR8 * CONST >>>> +// Not CONST-qualified in order to suppress gcc-6+ warnings. >>>> +// >>>> +STATIC CONST CHAR8 * >>>> =C2=A0 mBusMasterOperationName[EdkiiIoMmuOperationMaximum] =3D { >>>> =C2=A0=C2=A0=C2=A0 "Read", >>>> =C2=A0=C2=A0=C2=A0 "Write", >>> >>> We could commit this patch for now. Once the BZ you file for BaseTool= s >>> is fixed, we can revert this patch. >>> >> >> With your proposed workaround changes my compiler emits: >> >> [...] >> /root/edk2/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c:51:1: error: >> =E2=80=98mBusMasterOperationName=E2=80=99 defined but not used [-Werro= r=3Dunused-variable] >> =C2=A0mBusMasterOperationName[EdkiiIoMmuOperationMaximum] =3D { >> =C2=A0^~~~~~~~~~~~~~~~~~~~~~~ >=20 > Thank you for trying it. >=20 > How incredibly annoying. :/ >=20 > I'm unsure if we want to disble "-Wunused-variable" even for RELEASE > builds. Probably not. Needs more discussion by others too. >=20 >> Another idea, why not also compile out the declaration of the >> "mBusMasterOperationName" variable when building a RELEASE? >=20 > Code that depends on DEBUG vs. RELEASE should generally be evicted by > the compiler, not the preprocessor. This makes for much better source > code coverage. >=20 Uh, ok now looking again I see why. I first just saw: "EdkCompatibilityPkg/Foundation/Include/EfiDebug.h" Where both cases, mine and DEBUG(expr), would get removed by the preprocessor. But the here actual used "MdePkg/Include/Library/DebugLib.h= " tells me otherwise. >> Something like: >> >> ----8<---- >> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c >> b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c >> index bc57de5b57..5af5ce258f 100644 >> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c >> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c >> @@ -47,6 +47,7 @@ STATIC LIST_ENTRY mRecycledMapInfos =3D >> INITIALIZE_LIST_HEAD_VARIABLE ( >> =C2=A0// >> =C2=A0// ASCII names for EDKII_IOMMU_OPERATION constants, for debug l= ogging. >> =C2=A0// >> +#ifdef EFI_DEBUG^M >> =C2=A0STATIC CONST CHAR8 * CONST >> =C2=A0mBusMasterOperationName[EdkiiIoMmuOperationMaximum] =3D { >> =C2=A0=C2=A0 "Read", >> @@ -56,6 +57,7 @@ mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = =3D { >> =C2=A0=C2=A0 "Write64", >> =C2=A0=C2=A0 "CommonBuffer64" >> =C2=A0}; >> +#endif^M >> >> =C2=A0// >> =C2=A0// The following structure enables Map() and Unmap() to perform= in-place >> ---->8---- >> >> works here, not sure if its up to your coding standards. >=20 > It's not. >=20 > We already have macros like DEBUG_CODE(), DEBUG_CODE_BEGIN() and > DEBUG_CODE_END() -- they provide the above coverage --, but they only > work in block scope, not file scope. >=20 > Sigh. Let me submit another OvmfPkg patch for this. I'll CC you. Please= > report back if it works. >=20 OK. cheers, Thomas