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 AAE3B2095BB65 for ; Wed, 6 Sep 2017 10:06:30 -0700 (PDT) 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 5AE244792; Wed, 6 Sep 2017 17:09:20 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 5AE244792 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 F0A3C179D1; Wed, 6 Sep 2017 17:09:15 +0000 (UTC) To: Ard Biesheuvel Cc: edk2-devel-01 , Brijesh Singh , Jordan Justen , Thomas Lamprecht References: <20170906164819.5082-1-lersek@redhat.com> <20170906164819.5082-2-lersek@redhat.com> From: Laszlo Ersek Message-ID: <0868f6e6-597c-6b1d-0864-7b6b1997adcf@redhat.com> Date: Wed, 6 Sep 2017 19:09:14 +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: 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.30]); Wed, 06 Sep 2017 17:09:20 +0000 (UTC) Subject: Re: [PATCH 1/1] OvmfPkg/IoMmuDxe: shut up "unused-const-variable" gcc-6 warning in RELEASE 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 17:06:30 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/06/17 18:56, Ard Biesheuvel wrote: > On 6 September 2017 at 17:48, Laszlo Ersek wrote: >> Starting with gcc-6, a new warning option called "-Wunused-const-variable" >> has become available (and enabled by default under our build settings): >> >> https://gcc.gnu.org/onlinedocs/gcc-6.4.0/gcc/Warning-Options.html >> >> We should give it the same treatment as Ard gave "unused-but-set-variable" >> in commit 20d00edf21d2 ("BaseTools/GCC: set -Wno-unused-but-set-variables >> only on RELEASE builds", 2016-03-24); i.e., we should restrict the warning >> to the DEBUG build target. >> >> However, because the new warning is gcc-6+ only, we cannot add >> "-Wno-unused-const-variable" to any GCC5 macros in >> "BaseTools/Conf/tools_def.template". While the GCC6 toolchain and/or the >> desired handling of the new warning are investigated in >> , suppress the warning >> for gcc-6+ (in RELEASE builds) as follows: >> >> - Replace the "mBusMasterOperationName" array with the >> BUS_MASTER_OPERATION_NAME(Expression, ShortName) macro that compares >> Expression against EdkiiIoMmuOperationBusMaster, and in case >> of a match, evaluates to "ShortName", stringified, >> >> - when composing the DEBUG message -- which the preprocessor might >> eliminate from RELEASE builds, thereby removing its references to >> variables --, build a ladder of comparisons with >> BUS_MASTER_OPERATION_NAME(). >> > > I wonder if we should put this in a header file somewhere. I really > hate that we need to create new GCCx versions just for small > deviations between versions of the compiler. > > I.e., add this to MdePkg/Include/Base.h > > #ifdef MDEPKG_NDEBUG > // > // Don't warn about unused but set variables: they may only be > // referenced by DEBUG code. > // > #pragma GCC diagnostic ignored "-Wunused-but-set-variable" > #if __GNUC__ >= 6 > #pragma GCC diagnostic ignored "-Wunused-const-variable" > #endif > #endif The option in question is "-Wno-unused-const-variable". But, that's a side question. If the above is acceptable to the MdePkg maintainers, it is certainly good enough for me. In that case, I will reassign TianoCore BZ#700 to MdePkg. (I was careful to write "desired handling of the new warning" in the commit message, because I expected that the introduction of the GCC6 toolchain just for this might not be universally liked.) Personally I wouldn't like to submit the MdePkg patch myself, as GCC pragmas are another delicacy that doesn't fit on my plate right now. (If I happen to break something with it, I get to fix it too... Like I'm fixing it now within OvmfPkg.) The point of this patch is to get OVMF back to a building state ASAP. Once we fix BZ#700, I'm more than happy to revert the patch. I sought to write the patch in a way that would be tolerable both short-term and mid/long-term. Thanks! Laszlo > > >> Cc: Ard Biesheuvel >> Cc: Brijesh Singh >> Cc: Jordan Justen >> Cc: Thomas Lamprecht >> Reported-by: Thomas Lamprecht >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Laszlo Ersek >> --- >> OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 29 +++++++++++--------- >> 1 file changed, 16 insertions(+), 13 deletions(-) >> >> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c >> index bc57de5b572b..dcd03d8f0474 100644 >> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c >> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c >> @@ -45,17 +45,17 @@ STATIC LIST_ENTRY mRecycledMapInfos = INITIALIZE_LIST_HEAD_VARIABLE ( >> #define COMMON_BUFFER_SIG SIGNATURE_64 ('C', 'M', 'N', 'B', 'U', 'F', 'F', 'R') >> >> // >> -// ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging. >> +// The following macro builds the first two operands of a conditional (ternary) >> +// operator that matches Expression against the EDKII_IOMMU_OPERATION constant >> +// derived from ShortName. In case of a match, the replacement text evaluates >> +// to an ASCII string literal that stands for the constant. The replacement >> +// text stops before the colon (":"). The macro invocation site is supposed to >> +// provide the colon and the third operand, either as another invocation of the >> +// same macro, or as a default (fallback) string literal. >> // >> -STATIC CONST CHAR8 * CONST >> -mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = { >> - "Read", >> - "Write", >> - "CommonBuffer", >> - "Read64", >> - "Write64", >> - "CommonBuffer64" >> -}; >> +#define BUS_MASTER_OPERATION_NAME(Expression, ShortName) \ >> + ((Expression) == (EdkiiIoMmuOperationBusMaster ## ShortName)) ? \ >> + (# ShortName) >> >> // >> // The following structure enables Map() and Unmap() to perform in-place >> @@ -133,9 +133,12 @@ IoMmuMap ( >> DEBUG_VERBOSE, >> "%a: Operation=%a Host=0x%p Bytes=0x%Lx\n", >> __FUNCTION__, >> - ((Operation >= 0 && >> - Operation < ARRAY_SIZE (mBusMasterOperationName)) ? >> - mBusMasterOperationName[Operation] : >> + (BUS_MASTER_OPERATION_NAME (Operation, Read) : >> + BUS_MASTER_OPERATION_NAME (Operation, Write) : >> + BUS_MASTER_OPERATION_NAME (Operation, CommonBuffer) : >> + BUS_MASTER_OPERATION_NAME (Operation, Read64) : >> + BUS_MASTER_OPERATION_NAME (Operation, Write64) : >> + BUS_MASTER_OPERATION_NAME (Operation, CommonBuffer64) : >> "Invalid"), >> HostAddress, >> (UINT64)((NumberOfBytes == NULL) ? 0 : *NumberOfBytes) >> -- >> 2.14.1.3.gb7cf6e02401b >>