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 BF25D74003C for ; Sun, 10 Dec 2023 08:07:52 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=LIcI2CMKjh8oxrlWHjQr/qupND7UgjFxa5tk76xBBB0=; 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; s=20140610; t=1702195671; v=1; b=Xb+tmcCQc2QWtcVgahtLyJcO+7nlb5Xdd5gpIuNckgLFVcDBWR1C81fPACy3dATOX7cRxpTk VjZJkkBbn+7Uym0Q13wEcBfmXoyvBY/doJY+xRr9xe89pfrh5amBdstrCYrJZhrPGVjuTvcQXfM cOS7KalfOEwr7wcn3YXgWn0Y= X-Received: by 127.0.0.2 with SMTP id RNRnYY7687511xSiY4sgxH1w; Sun, 10 Dec 2023 00:07:51 -0800 X-Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web10.51123.1702195670322310872 for ; Sun, 10 Dec 2023 00:07:50 -0800 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by ams.source.kernel.org (Postfix) with ESMTP id 52D33B80880 for ; Sun, 10 Dec 2023 08:07:48 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id AA3B9C433CA for ; Sun, 10 Dec 2023 08:07:47 +0000 (UTC) X-Received: by mail-lj1-f182.google.com with SMTP id 38308e7fff4ca-2c9f7fe6623so42017311fa.3 for ; Sun, 10 Dec 2023 00:07:47 -0800 (PST) X-Gm-Message-State: RuNfqwqEVGf0ISIrAyrOVVQRx7686176AA= X-Google-Smtp-Source: AGHT+IGbQCugPkGllf0aJSiidu6bOX2DCXEjiV97deZ6cUXEFX8l0d5xr4hL+2C+eqI5q9pIn7pS5M8G76pP936CjO0= X-Received: by 2002:a2e:a376:0:b0:2cb:3d3d:b909 with SMTP id i22-20020a2ea376000000b002cb3d3db909mr333207ljn.104.1702195665817; Sun, 10 Dec 2023 00:07:45 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: "Ard Biesheuvel" Date: Sun, 10 Dec 2023 09:07:34 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] RELEASE CLANGPDB OVMF currently does not compile To: devel@edk2.groups.io, mjsbeaton@gmail.com Cc: Ard Biesheuvel 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" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=Xb+tmcCQ; 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 Sun, 10 Dec 2023 at 01:26, Mike Beaton wrote: > > Dear Ard, > > This one is about RELEASE CLANGPDB OVMF, which currently does not > compile, giving: > > ``` > /home/mjsbeaton/OpenSource/edk2/OvmfPkg/VirtioSerialDxe/VirtioSerial.c:28:22: > error: variable 'EventNames' is not needed and will not be emitted > [-Werror,-Wunneeded-internal-declaration] > STATIC CONST CHAR8 *EventNames[] = { > ^ > 1 error generated. > ``` > > I logged this at https://bugzilla.tianocore.org/show_bug.cgi?id=4620 > > This _can_ be fixed by: > > ``` > --- a/BaseTools/Conf/tools_def.template > +++ b/BaseTools/Conf/tools_def.template > @@ -1754,7 +1754,7 @@ DEFINE CLANGPDB_X64_PREFIX = ENV(CLANG_BIN) > DEFINE CLANGPDB_IA32_TARGET = -target i686-unknown-windows-gnu > DEFINE CLANGPDB_X64_TARGET = -target x86_64-unknown-windows-gnu > > -DEFINE CLANGPDB_WARNING_OVERRIDES = -Wno-parentheses-equality > -Wno-tautological-compare > -Wno-tautological-constant-out-of-range-compare -Wno-empty-body > -Wno-unused-const-variable -Wno-varargs -Wno-unknown-warning-option > -Wno-unused-but-set-variable -Wno-unused-const-variable > -Wno-unaligned-access -Wno-microsoft-enum-forward-reference > +DEFINE CLANGPDB_WARNING_OVERRIDES = -Wno-parentheses-equality > -Wno-tautological-compare > -Wno-tautological-constant-out-of-range-compare -Wno-empty-body > -Wno-unused-const-variable -Wno-varargs -Wno-unknown-warning-option > -Wno-unused-but-set-variable -Wno-unused-const-variable > -Wno-unaligned-access -Wno-unneeded-internal-declaration > -Wno-microsoft-enum-forward-reference > DEFINE CLANGPDB_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) > DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector -funsigned-char > -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang > -Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas > -Wno-incompatible-library-redeclaration -Wno-null-dereference > -mno-implicit-float -mms-bitfields -mno-stack-arg-probe -nostdlib > -nostdlibinc -fseh-exceptions > > ########################### > ``` > > which duplicates https://github.com/tianocore/edk2/commit/d3225577123, > which already applied the same change to CLANGDWARF. > > That change was discussed and approved here: > https://edk2.groups.io/g/devel/topic/98807494 - however, I'd like to > belatedly object, if I may! > > There is currently exactly one line of code which needs this warning > to be disabled (at least, in OVMF), the line mentioned above. So if we > were going to bring the warning back, now rather than later would be > the time to do it. The issue at that line can, of course, be worked > around by removing the STATIC (and presumably adding a comment, so > that someone doesn't mistakenly add it back again). > > I would guess that there must be several other places where people > _have_ already tiptoed round this issue before, in EDK-2 code, though > a quick search for the warning name itself only throws up one such > instance in OpenSslLib. > > The advantage of tiptoeing round the issue (in the reasonably rare > cases where needed) instead of disabling the error check, is that the > check may well show up genuine issues in code (perhaps most obviously, > variables left unused after code changes). > > For that reason, I'd propose re-enabling the warning in CLANGDWARF, > and removing the STATIC (and adding a comment) in the relevant line in > VirtioSerialDxe. > Removing STATIC means that (modulo LTO) the compiler will not know whether or not the definition can be dropped. It also pollutes the global namespace. IMO, lack of the use of STATIC where appropriate is a severe issue with the EDK2 codebase. Removing STATIC to appease compiler diagnostics is *not* the way to solve this. The patch you proposed looks fine to me: Acked-by: Ard Biesheuvel -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112257): https://edk2.groups.io/g/devel/message/112257 Mute This Topic: https://groups.io/mt/103083030/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-