Re: [edk2-devel] [PATCH v2 2/2] SctPkg: Fix the UefiSct -Wincompatible-pointer-types warnings
Reviewed-by: G Edhaya Chandran -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112271): https://edk2.groups.io/g/devel/message/112271 Mute This Topic: https://groups.io/mt/91642654/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 1/2] SctPkg: Fix X64 build errors for GCC toolchain
Reviewed-by: G Edhaya Chandran -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112270): https://edk2.groups.io/g/devel/message/112270 Mute This Topic: https://groups.io/mt/91642649/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Question about the boundary and difference between System Firmware and UEFI CXL drivers
On 2023-12-08 15:42, Yoshinoya wrote: There is a description about UEFI Drivers in 1.4 Abbreviations in this doc. UEFI Drivers : UEFI CXL Bus and memory device drivers. I think UEFI CXL Drivers is a part of System Firmware(UEFI BIOS). These UEFI Drivers may do some basic configuation for some direct attached clx type-3 devices. _._,_._ I found that description too, but in "Figure 34-High-level sequence: System Firmware boot" it shows that system firmware could enumerate CXL components and be followed with an optional UEFI Boot Sequence which also could enumerate CXL components (Figure 35). In my understanding, only UEFI CXL Bus and memory device drivers could enumerate CXL components, therefore, how system firmware could finish this responsibilty? Many thanks Yuquan -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112269): https://edk2.groups.io/g/devel/message/112269 Mute This Topic: https://groups.io/mt/103008846/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg/StandaloneMmHobLib: Remove HOB creation
Merged at bb13a4adab
Thanks,
Ray
> -Original Message-
> From: Ni, Ray
> Sent: Saturday, December 9, 2023 9:27 AM
> To: Nhi Pham ; devel@edk2.groups.io
> Cc: Ard Biesheuvel ; Sami Mujawar
> ; Oliver Smith-Denny
> Subject: RE: [PATCH v1 1/1] StandaloneMmPkg/StandaloneMmHobLib:
> Remove HOB creation
>
> Nhi,
> Thanks for the follow up!
>
> Reviewed-by: Ray Ni
>
> Thanks,
> Ray
> > -Original Message-
> > From: Nhi Pham
> > Sent: Tuesday, December 5, 2023 9:48 PM
> > To: devel@edk2.groups.io
> > Cc: Nhi Pham ; Ard Biesheuvel
> > ; Ni, Ray ; Sami Mujawar
> > ; Oliver Smith-Denny
>
> > Subject: [PATCH v1 1/1] StandaloneMmPkg/StandaloneMmHobLib: Remove
> > HOB creation
> >
> > According to the discussion in "StandaloneMmPkg: Fix HOB space and
> > heap space conflicted issue" [1], Standalone MM modules should be HOB
> > consumers where HOB is read-only. Therefore, this patch removes the
> > supported functions for HOB creation in the StandaloneMmHobLib.
> >
> > [1] https://edk2.groups.io/g/devel/message/108333
> >
> > Cc: Ard Biesheuvel
> > Cc: Ray Ni
> > Cc: Sami Mujawar
> > Cc: Oliver Smith-Denny
> > Signed-off-by: Nhi Pham
> > ---
> >
> StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c |
> > 171 ++--
> > 1 file changed, 51 insertions(+), 120 deletions(-)
> >
> > diff --git
> >
> a/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
> >
> b/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
> > index ee61bdd227d0..bef66d167494 100644
> > ---
> >
> a/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
> > +++
> >
> b/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
> > @@ -1,5 +1,5 @@
> > /** @file
> >
> > - HOB Library implementation for Standalone MM Core.
> >
> > + HOB Library implementation for Standalone MM modules.
> >
> >
> >
> > Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
> >
> > Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
> >
> > @@ -250,48 +250,13 @@ GetBootModeHob (
> >return HandOffHob->BootMode;
> >
> > }
> >
> >
> >
> > -VOID *
> >
> > -CreateHob (
> >
> > - IN UINT16 HobType,
> >
> > - IN UINT16 HobLength
> >
> > - )
> >
> > -{
> >
> > - EFI_HOB_HANDOFF_INFO_TABLE *HandOffHob;
> >
> > - EFI_HOB_GENERIC_HEADER *HobEnd;
> >
> > - EFI_PHYSICAL_ADDRESSFreeMemory;
> >
> > - VOID*Hob;
> >
> > -
> >
> > - HandOffHob = GetHobList ();
> >
> > -
> >
> > - HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
> >
> > -
> >
> > - FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob-
> > >EfiFreeMemoryBottom;
> >
> > -
> >
> > - if (FreeMemory < HobLength) {
> >
> > -return NULL;
> >
> > - }
> >
> > -
> >
> > - Hob= (VOID
> *)(UINTN)HandOffHob->EfiEndOfHobList;
> >
> > - ((EFI_HOB_GENERIC_HEADER *)Hob)->HobType = HobType;
> >
> > - ((EFI_HOB_GENERIC_HEADER *)Hob)->HobLength = HobLength;
> >
> > - ((EFI_HOB_GENERIC_HEADER *)Hob)->Reserved = 0;
> >
> > -
> >
> > - HobEnd = (EFI_HOB_GENERIC_HEADER
> *)((UINTN)Hob +
> > HobLength);
> >
> > - HandOffHob->EfiEndOfHobList =
> (EFI_PHYSICAL_ADDRESS)(UINTN)HobEnd;
> >
> > -
> >
> > - HobEnd->HobType = EFI_HOB_TYPE_END_OF_HOB_LIST;
> >
> > - HobEnd->HobLength = sizeof (EFI_HOB_GENERIC_HEADER);
> >
> > - HobEnd->Reserved = 0;
> >
> > - HobEnd++;
> >
> > - HandOffHob->EfiFreeMemoryBottom =
> > (EFI_PHYSICAL_ADDRESS)(UINTN)HobEnd;
> >
> > -
> >
> > - return Hob;
> >
> > -}
> >
> > -
> >
> > /**
> >
> >Builds a HOB for a loaded PE32 module.
> >
> >
> >
> >This function builds a HOB for a loaded PE32 module.
> >
> > + It can only be invoked by Standalone MM Core.
> >
> > + For Standalone MM drivers, it will ASSERT() since HOB is read only for
> > Standalone MM drivers.
> >
> > +
> >
> >If ModuleName is NULL, then ASSERT().
> >
> >If there is no additional space for HOB creation, then ASSERT().
> >
> >
> >
> > @@ -310,33 +275,19 @@ BuildModuleHob (
> >IN EFI_PHYSICAL_ADDRESS EntryPoint
> >
> >)
> >
> > {
> >
> > - EFI_HOB_MEMORY_ALLOCATION_MODULE *Hob;
> >
> > -
> >
> > - ASSERT (
> >
> > -((MemoryAllocationModule & (EFI_PAGE_SIZE - 1)) == 0) &&
> >
> > -((ModuleLength & (EFI_PAGE_SIZE - 1)) == 0)
> >
> > -);
> >
> > -
> >
> > - Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof
> > (EFI_HOB_MEMORY_ALLOCATION_MODULE));
> >
> > -
> >
> > - CopyGuid (&(Hob->MemoryAllocationHeader.Name),
> > );
> >
> > - Hob->MemoryAllocationHeader.MemoryBaseAddress =
> > MemoryAllocationModule;
> >
> > - Hob->MemoryAllocationHeader.MemoryLength = ModuleLength;
> >
> > - Hob->MemoryAllocationHeader.MemoryType=
> EfiBootServicesCode;
> >
> > -
> >
> >//
> >
> > - // Zero the reserved space to match HOB spec
> >
> > + // HOB is read only for Standalone MM drivers
> >
> >//
> >
> > - ZeroMem (Hob->MemoryAllocationHeader.Reserved,
Re: [edk2-devel] [Patch V3 0/6] Create and consume a new gMpInformationHobGuid2 in UefiCpuPkg.
Hi Laszlo, Previously I sent a patch set " Move gMpInformationHobGuid from StandaloneMmPkg to UefiCpuPkg. " and thanks for your review. To solve the issue that the maximum length of one HOB might not be enough when CPU count is 1-2000 or bigger and extend the HOB, we decide to create a new MpInfo2Hob in UefiCpuPkg in this patch set. Do you have any comments about the patch set? Thanks, Dun -Original Message- From: devel@edk2.groups.io On Behalf Of duntan Sent: Friday, December 8, 2023 5:55 PM To: devel@edk2.groups.io Subject: [edk2-devel] [Patch V3 0/6] Create and consume a new gMpInformationHobGuid2 in UefiCpuPkg. In the V3 patch set, In patch "UefiCpuPkg: Build MpInfo2HOB in CpuMpPei", the DEBUG message format is modified In patch "UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe", remove unneccesary assert check. In patch "UefiCpuPkg: Avoid assuming only one smmbasehob", free allocated buffer when error returning case happen. Dun Tan (6): UefiCpuPkg: Create gMpInformationHobGuid2 in UefiCpuPkg UefiCpuPkg: Build MpInfo2HOB in CpuMpPei UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe UefiCpuPkg: Add a new field in MpInfo2 HOB UefiCpuPkg: Cache core type in MpInfo2 HOB UefiCpuPkg: Avoid assuming only one smmbasehob UefiCpuPkg/CpuMpPei/CpuMpPei.c | 146 ++ UefiCpuPkg/CpuMpPei/CpuMpPei.h | 6 +- UefiCpuPkg/CpuMpPei/CpuMpPei.inf | 3 ++- UefiCpuPkg/Include/Guid/MpInformation2.h | 58 ++ UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 354 ++ UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 2 +- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 8 UefiCpuPkg/UefiCpuPkg.dec| 3 +++ 8 files changed, 513 insertions(+), 67 deletions(-) create mode 100644 UefiCpuPkg/Include/Guid/MpInformation2.h -- 2.31.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112267): https://edk2.groups.io/g/devel/message/112267 Mute This Topic: https://groups.io/mt/103052268/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] Event: Tools, CI, Code base construction meeting series - Monday, December 11, 2023 #cal-reminder
*Reminder: Tools, CI, Code base construction meeting series* *When:* Monday, December 11, 2023 4:30pm to 5:30pm (UTC-08:00) America/Los Angeles *Where:* https://teams.microsoft.com/l/meetup-join/19%3ameeting_ZDI2ZDg4NmMtMjI1My00MzI5LWFmYjAtMGQyNjUzNTBjZGYw%40thread.v2/0?context=%7b%22Tid%22%3a%2272f988bf-86f1-41af-91ab-2d7cd011db47%22%2c%22Oid%22%3a%2223af6561-6e1c-450d-b917-d9d674eb3cb6%22%7d View Event ( https://edk2.groups.io/g/devel/viewevent?eventid=2127516 ) *Description:* TianoCore community, Microsoft and Intel will be hosting a series of open meetings to discuss build, CI, tools, and other related topics. If you are interested, have ideas/opinions please join us. These meetings will be Monday 4:30pm Pacific Time on Microsoft Teams. MS Teams Link in following discussion: * https://github.com/tianocore/edk2/discussions/2614 Anyone is welcome to join. * tianocore/edk2: EDK II (github.com) * tianocore/edk2-basetools: EDK II BaseTools Python tools as a PIP module (github.com) https://github.com/tianocore/edk2-basetools * tianocore/edk2-pytool-extensions: Extensions to the edk2 build system allowing for a more robust and plugin based build system and tool execution environment (github.com) https://github.com/tianocore/edk2-pytool-extensions * tianocore/edk2-pytool-library: Python library package that supports UEFI development (github.com) https://github.com/tianocore/edk2-pytool-library MS Teams Browser Clients * https://docs.microsoft.com/en-us/microsoftteams/get-clients?tabs=Windows#browser-client -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112265): https://edk2.groups.io/g/devel/message/112265 Mute This Topic: https://groups.io/mt/103100093/21656 Mute #cal-reminder:https://edk2.groups.io/g/devel/mutehashtag/cal-reminder Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
Thanks for the review. My comments inline:
On Fri, Dec 8, 2023 at 9:58 AM Sunil V L wrote:
> On Thu, Dec 07, 2023 at 10:31:48AM +0530, Dhaval Sharma wrote:
> > Comments inline:
> >
> >
> > On Wed, Dec 6, 2023 at 7:50 PM Sunil V L
> wrote:
> >
> > > Hi Dhaval,
> > >
> > > Thank you very much for fixing the issue with instruction cache
> > > invalidation and confirming with the spec owner. Few minor comments
> > > below.
> > >
> > > On Mon, Dec 04, 2023 at 01:59:49PM +0530, Dhaval Sharma wrote:
> > > > Use newly defined cache management operations for RISC-V where
> possible
> > > > It builds up on the support added for RISC-V cache management
> > > > instructions in BaseLib.
> > > > Cc: Michael D Kinney
> > > > Cc: Liming Gao
> > > > Cc: Zhiguang Liu
> > > > Cc: Laszlo Ersek
> > > >
> > > > Signed-off-by: Dhaval Sharma
> > > > Acked-by: Laszlo Ersek
> > > > ---
> > > >
> > > > Notes:
> > > > V9:
> > > > - Fixed an issue with Instruction cache invalidation. Use fence.i
> > > > instruction as CMO does not support i-cache operations.
> > > > V8:
> > > > - Added note to convert PCD into RISC-V feature bitmap pointer
> > > > - Modified function names to be more explicit about cache ops
> > > > - Added RB tag
> > > > V7:
> > > > - Added PcdLib
> > > > - Restructure DEBUG message based on feedback on V6
> > > > - Make naming consistent to CMO, remove all CBO references
> > > > - Add ASSERT for not supported functions instead of plain debug
> > > message
> > > > - Added RB tag
> > > > V6:
> > > > - Utilize cache management instructions if HW supports it
> > > > This patch is part of restructuring on top of v5
> > > >
> > > IMO, it is better to keep the change log in the cover letter. Since not
> > > all patches may be CC'd to every one apart from the cover letter, it is
> > > difficult to understand from the cover letter what has changed in the
> new
> > > series.
> > >
> > [Dhaval] AFAIU notes are tied to specific commits. But it makes sense, I
> > can add an update to the cover letter.
> >
> > >
> > > > MdePkg/MdePkg.dec |
> > > 8 +
> > > > MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |
> > > 5 +
> > > > MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c|
> > > 173
> > > > MdePkg/MdePkg.uni |
> > > 4 +
> > > > 4 files changed, 160 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> > > > index ac54338089e8..fa92673ff633 100644
> > > > --- a/MdePkg/MdePkg.dec
> > > > +++ b/MdePkg/MdePkg.dec
> > > > @@ -2399,6 +2399,14 @@ [PcdsFixedAtBuild.AARCH64,
> > > PcdsPatchableInModule.AARCH64]
> > > ># @Prompt CPU Rng algorithm's GUID.
> > > >
> > >
> gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x0037
> > > >
> > > > +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64]
> > > > + #
> > > > + # Configurability to override RISC-V CPU Features
> > > > + # BIT 0 = Cache Management Operations. This bit is relevant only
> if
> > > > + # previous stage has feature enabled and user wants to disable it.
> > > > + #
> > > > +
> > >
> gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0x|UINT64|0x69
> > > > +
> > > > [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic,
> PcdsDynamicEx]
> > > >## This value is used to set the base address of PCI express
> > > hierarchy.
> > > ># @Prompt PCI Express Base Address.
> > > > diff --git
> > > a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> > > b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> > > > index 6fd9cbe5f6c9..601a38d6c109 100644
> > > > ---
> a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> > > > +++
> b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> > > > @@ -56,3 +56,8 @@ [LibraryClasses]
> > > >BaseLib
> > > >DebugLib
> > > >
> > > > +[LibraryClasses.RISCV64]
> > > > + PcdLib
> > > > +
> > > > +[Pcd.RISCV64]
> > > > + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUMES
> > > > diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > > b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > > > index ac2a3c23a249..cacc38eff4f4 100644
> > > > --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > > > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > > > @@ -2,6 +2,7 @@
> > > >RISC-V specific functionality for cache.
> > > >
> > > >Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All
> > > rights reserved.
> > > > + Copyright (c) 2023, Rivos Inc. All rights reserved.
> > > >
> > > >SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > **/
> > > > @@ -9,10 +10,117 @@
> > > > #include
> > > >
Re: [edk2-devel] [PATCH] BaseTools/tools_def: Disable unneeded-internal-declaration warning in CLANGPDB
Resolves https://bugzilla.tianocore.org/show_bug.cgi?id=4620 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112262): https://edk2.groups.io/g/devel/message/112262 Mute This Topic: https://groups.io/mt/103087794/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] BaseTools/tools_def: Disable unneeded-internal-declaration warning in CLANGPDB
Repeats the commit already acked by Ard in https://edk2.groups.io/g/devel/topic/103083030, though with an attempt to provide an additional (not yet acked) useful commit message. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112261): https://edk2.groups.io/g/devel/message/112261 Mute This Topic: https://groups.io/mt/103087794/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] RELEASE CLANGPDB OVMF currently does not compile
I've sent a patch to this list (with some kind of proposed comment about the issues!) under separate cover. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112260): https://edk2.groups.io/g/devel/message/112260 Mute This Topic: https://groups.io/mt/103083030/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH] BaseTools/tools_def: Disable unneeded-internal-declaration warning in CLANGPDB
From: Mike Beaton This warning was already disabled in CLANGDWARF by commit d3225577123767fd09c91201d27e9c91663ae132. gcc can distinguish between optimised-away variable usage (as can occur in valid debug code) and genuinely unused variables, and only complains about the latter. clang cannot, and therefore this warning ends up complaining about valid debug code under clang. Since EDK-II code is in general going to be compiled by gcc as well as clang then disabling this warning in clang does not amount to entirely removing potentially valid warnings about genuinely unused variables. Signed-off-by: Mike Beaton --- BaseTools/Conf/tools_def.template | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template index c34ecfd557..48cf45245f 100755 --- 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 ### -- 2.39.2 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112259): https://edk2.groups.io/g/devel/message/112259 Mute This Topic: https://groups.io/mt/103087794/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] RELEASE CLANGPDB OVMF currently does not compile
> 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. Thank you for your feedback. On reflection, since gcc still _can_ distinguish between genuinely unused variables and variables who usage was optimized away like this (I think that's well known; but I just double-checked by adding a similar, but entirely unused variable to the same file - gcc then complains), and since all code in the project is going to end up being compiled under gcc as well clang, then just squelching the (slightly broken) warning under clang is not really losing useful information after all, in this case. So thank you for the ack, and agreed, now, that it is the best way to proceed. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112258): https://edk2.groups.io/g/devel/message/112258 Mute This Topic: https://groups.io/mt/103083030/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] RELEASE CLANGPDB OVMF currently does not compile
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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
