From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 950DF740032 for ; Tue, 12 Dec 2023 22:26:57 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=+P8IgwvmYyipHIEhUcRrMOa4gJIuKKhI+qBuYzJmNs4=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Transfer-Encoding; s=20140610; t=1702420016; v=1; b=sjvA/KqvL8fKm+6SqNZ9AuVPdRK2xkEbo3PxeER3AINvkZ2lnZizXZuSva19dzufa/L2QUhb buu7EN3+H0WLBDoOaGzOHfdwS3g4KtHWdemai/0LXYmgsI5OSZMSCfxYkX95cBEXusANsTaETjP nOqwFvHsEJBHrawAn7BH3eE8= X-Received: by 127.0.0.2 with SMTP id PK6TYY7687511xf1DPsOzxUF; Tue, 12 Dec 2023 14:26:56 -0800 X-Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web11.9688.1702420015219084038 for ; Tue, 12 Dec 2023 14:26:55 -0800 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by ams.source.kernel.org (Postfix) with ESMTP id DC689B817B8 for ; Tue, 12 Dec 2023 22:26:52 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id 35B1EC433C9 for ; Tue, 12 Dec 2023 22:26:52 +0000 (UTC) X-Received: by mail-lj1-f176.google.com with SMTP id 38308e7fff4ca-2ca1e6a94a4so81506871fa.0 for ; Tue, 12 Dec 2023 14:26:52 -0800 (PST) X-Gm-Message-State: TRb9LcVPe0cn8alPa6BxeSZox7686176AA= X-Google-Smtp-Source: AGHT+IH/au08tZNwBQYAneJ//oH2iWBhoHF7WkpFvd/97Yru36ZI1v4dXQfkR8FrKtWeNvKArpik9xMrhS3q/8Wi264= X-Received: by 2002:a05:651c:b09:b0:2cc:2602:d065 with SMTP id b9-20020a05651c0b0900b002cc2602d065mr1906964ljr.90.1702420010350; Tue, 12 Dec 2023 14:26:50 -0800 (PST) MIME-Version: 1.0 References: <20231212214524.49914-1-mjsbeaton@gmail.com> In-Reply-To: <20231212214524.49914-1-mjsbeaton@gmail.com> From: "Ard Biesheuvel" Date: Tue, 12 Dec 2023 23:26:39 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH V2] DebugLib: Allow -Wunneeded-internal-declaration with clang To: devel@edk2.groups.io, mjsbeaton@gmail.com Cc: ardb@google.com, lersek@redhat.com, Mikhail Krichanov Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="sjvA/Kqv"; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On Tue, 12 Dec 2023 at 22:46, Mike Beaton wrote: > > From: Mikhail Krichanov > > Provides a variant of the DEBUG macro for clang when MDEPKG_NDEBUG is > defined, which uses but discards the contained expression. This means > clang can tell that it has optimised away variable usage as part of > valid debug patterns (such as a STATIC variable which is only used in > DEBUG statements) - rather than just seeing such variables as > completely unused - therefore we can keep > -Wunneeded-internal-declaration (as part of -Wall) to warn about > mistakenly genuinely unused variables elsewhere. > > Note that the _Pragma in the clang/gcc variant is to temporarily > suppress the warning about `(VOID) Expression;`, whilst the presence > of `(VOID) Expression;` (once allowed) is what prevents the unwanted > warning about unneeded variables. > > Signed-off-by: Mikhail Krichanov > Signed-off-by: Mike Beaton This patch breaks the build on ARM (undeclared functions and variables). Nothing we couldn't fix, but that needs to be done before we can consider this change. > --- > BaseTools/Conf/tools_def.template | 2 +- > MdePkg/Include/Library/DebugLib.h | 10 ++++++++++ > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def= .template > index c34ecfd557..eaccf0b698 100755 > --- a/BaseTools/Conf/tools_def.template > +++ b/BaseTools/Conf/tools_def.template > @@ -1859,7 +1859,7 @@ DEFINE CLANGDWARF_X64_DLINK2_FLAGS =3D -Wl,-= -defsym=3DPECOFF_HEADER_SIZE=3D0x22 > DEFINE CLANGDWARF_IA32_TARGET =3D -target i686-pc-linux-gnu > DEFINE CLANGDWARF_X64_TARGET =3D -target x86_64-pc-linux-gn= u > > -DEFINE CLANGDWARF_WARNING_OVERRIDES =3D -Wno-parentheses-equality -Wn= o-empty-body -Wno-unused-const-variable -Wno-varargs -Wno-unknown-warning-o= ption -Wno-unused-but-set-variable -Wno-unused-const-variable -Wno-unaligne= d-access -Wno-unneeded-internal-declaration > +DEFINE CLANGDWARF_WARNING_OVERRIDES =3D -Wno-parentheses-equality -Wn= o-empty-body -Wno-unused-const-variable -Wno-varargs -Wno-unknown-warning-o= ption -Wno-unused-but-set-variable -Wno-unused-const-variable -Wno-unaligne= d-access > DEFINE CLANGDWARF_ALL_CC_FLAGS =3D DEF(GCC48_ALL_CC_FLAGS) DEF(C= LANGDWARF_WARNING_OVERRIDES) -fno-stack-protector -mms-bitfields -Wno-addre= ss -Wno-shift-negative-value -Wno-unknown-pragmas -Wno-incompatible-library= -redeclaration -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -msoft-flo= at -mno-implicit-float -ftrap-function=3Dundefined_behavior_has_been_optim= ized_away_by_clang -funsigned-char -fno-ms-extensions -Wno-null-dereference > > ########################### > diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/D= ebugLib.h > index 40772f2e0f..7368004f46 100644 > --- a/MdePkg/Include/Library/DebugLib.h > +++ b/MdePkg/Include/Library/DebugLib.h > @@ -425,6 +425,16 @@ UnitTestDebugAssert ( > _DEBUG (Expression); \ > } \ > } while (FALSE) > +#elif defined (__GNUC__) || defined (__clang__) This is now being enabled on GCC too - why? > +#define DEBUG(Expression) \ > + do { \ > + _Pragma("GCC diagnostic push") \ > + _Pragma("GCC diagnostic ignored \"-Wunused-value\"") \ > + if ((FALSE)) { \ > + (VOID) Expression; \ So what is the point of the VOID cast here? Usually, these are added to avoid unused value warnings, but these are disabled explicitly via the pragma. Why can't we just use --- a/MdePkg/Include/Library/DebugLib.h +++ b/MdePkg/Include/Library/DebugLib.h @@ -426,7 +426,12 @@ UnitTestDebugAssert ( } \ } while (FALSE) #else -#define DEBUG(Expression) +#define DEBUG(Expression) \ + do { \ + if (FALSE) { \ + _DEBUG (Expression); \ + } \ + } while (FALSE) #endif /** here? > + } \ > + _Pragma("GCC diagnostic pop") \ > + } while (FALSE) > #else > #define DEBUG(Expression) > #endif > -- > 2.39.2 > > > >=20 > > -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112459): https://edk2.groups.io/g/devel/message/112459 Mute This Topic: https://groups.io/mt/103138778/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-