From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 7F33921E62BBD for ; Wed, 6 Sep 2017 04:12:48 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F04A53680F; Wed, 6 Sep 2017 11:15:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com F04A53680F Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-41.rdu2.redhat.com [10.10.120.41]) by smtp.corp.redhat.com (Postfix) with ESMTP id 93CA780F96; Wed, 6 Sep 2017 11:15:36 +0000 (UTC) To: Thomas Lamprecht , edk2-devel@lists.01.org References: <9fbd059f-18d8-a798-da00-951f85c9fd1a@proxmox.com> From: Laszlo Ersek Cc: Ard Biesheuvel , "Gao, Liming" , "Zhu, Yonghong" Message-ID: <554afa66-cc81-6115-ff83-2b0b6f01d455@redhat.com> Date: Wed, 6 Sep 2017 13:15:35 +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: <9fbd059f-18d8-a798-da00-951f85c9fd1a@proxmox.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Wed, 06 Sep 2017 11:15:38 +0000 (UTC) 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 11:12:48 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hello Thomas, (CC Ard, Liming, Yonghong) On 09/06/17 09:44, Thomas Lamprecht wrote: > Hi, > > commit 2ad6ba80a1bd58382bde6b994070f7c01d2fb48d > Author: Laszlo Ersek > Date: Wed Aug 30 14:00:58 2017 +0200 > > OvmfPkg/IoMmuDxe: IoMmuMap(): log nicer and more informative DEBUG msgs > > triggers a unused-const-variable warning for mBusMasterOperationName, > and thus for releases with warnings-as-errors a build error. > > I'm using a quite vanilla Debian Stretch machine on amd64/x86_64 > (running in qemu/KVM) as build host, gcc version is 6.3.0 20170516 > (Debian 6.3.0-18) > > My build procedure looks like: > > # make -C BaseTools/ > # . edksetup.sh > # OvmfPkg/build.sh -a X64 -b RELEASE -n 4 -t GCC5 > > With current master (12cfc9009e7cf1a69ca675110c2cf6e21b152992) checked > out. > > I suspect that gcc, at least in this version, cannot track the usage > of the variable in crime in the DEBUG macros, and thus (falsely?) > detects this warning. > > So I'm not quite sure if this is a problem with edk2/Ovmf itself or a > problem stemming from gcc. > > The following patch fixes the problem quite nonchalantly by adding an > unused attribute, which is highly probably not what is wanted as a > clean fix - I guess - but it achieves my desired result :) > > ----8<---- > diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c > b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c > index bc57de5b57..8c0b8b0931 100644 > --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c > +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c > @@ -48,7 +48,7 @@ STATIC LIST_ENTRY mRecycledMapInfos = > INITIALIZE_LIST_HEAD_VARIABLE ( > // ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging. > // > STATIC CONST CHAR8 * CONST > -mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = { > +mBusMasterOperationName[EdkiiIoMmuOperationMaximum] __attribute__ > ((unused)) = { > "Read", > "Write", > "CommonBuffer", > ---->8---- > > It naturally could be that I've got something wrong, wouldn't be the > first time, but I suspect that it's edk2 in combination with my GCC > version this time. As I lack in depth knowledge of Ovmf it'd be nice > if someone could confirm my suspicion. > > CC'ing Laszlo as he is the author of the patch which lets this problem > trigger and following this mailing list since a bit it seems that he > surely has the in depth knowledge. Sorry that I wasn't patient enough > to test other compiler version before posting here. Thanks for the report. When I (recently) wrote the above commit, I was fully aware that the variable would become unused if DEBUG macro invocations were compiled out of the code (i.e., in the RELEASE build target). However, that didn't worry me because we had already introduced a distinction for such variables, between RELEASE and DEBUG builds. As you can expect, this phenomenon is quite wide-spread; for example a Status variable may capture a function return value, only to be ASSERT()-ed upon. In a RELEASE build, the ASSERT() disappears, and we don't want the compiler to yell at us for setting, but never checking, Status. ... Please refer to git commit 20d00edf21d2 ("BaseTools/GCC: set -Wno-unused-but-set-variables only on RELEASE builds", 2016-03-24). Looks like gcc-6 has a knob specific to const variables that we should give the same treatment. Looking at the following pages: https://gcc.gnu.org/onlinedocs/gcc-4.9.4/gcc/Warning-Options.html https://gcc.gnu.org/onlinedocs/gcc-5.4.0/gcc/Warning-Options.html https://gcc.gnu.org/onlinedocs/gcc-6.4.0/gcc/Warning-Options.html https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Warning-Options.html it appears that "-Wunused-const-variable" has first appeared in gcc-6. Now, that's a problem. In edk2, we currently do not have a specific GCC6 toolchain setting. We have several GCC4x toolchains, and one GCC5 toolchain. Normally I would have asked you to send a patch, similar to commit 20d00edf21d2 above, to add "-Wno-unused-const-variable" to the "oldest" applicable macros in "BaseTools/Conf/tools_def.template", for example: RELEASE_GCC5_IA32_CC_FLAGS RELEASE_GCC5_X64_CC_FLAGS RELEASE_GCC5_ARM_CC_FLAGS RELEASE_GCC5_AARCH64_CC_FLAGS But gcc-5 would not recognize this option. I think we might have to introduce the GCC6 toolchain for this. Here's what I suggest: please file a TianoCore BZ about this issue, at . Select "BaseTools" as Package. Feel free to reference the mailing list thread in the BZ: https://lists.01.org/pipermail/edk2-devel/2017-September/014225.html Now, addressing that will take a while. Meanwhile, can you please check 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? > 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 = INITIALIZE_LIST_HEAD_VARIABLE ( > // > // ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging. > // > -STATIC CONST CHAR8 * CONST > +// Not CONST-qualified in order to suppress gcc-6+ warnings. > +// > +STATIC CONST CHAR8 * > mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = { > "Read", > "Write", We could commit this patch for now. Once the BZ you file for BaseTools is fixed, we can revert this patch. Thank you! Laszlo