Re: [edk2-devel] [PATCH] NetworkPkg/HttpBootDxe: Correctly uninstall HttpBootCallbackProtocol
On Sat, 20 Apr 2024 at 17:31, Michael Brown wrote: > > On 19/04/2024 11:02, Mike Beaton wrote: > > Dear Michael, > > > > I don't know if you had time to answer one follow-up question. > > > > Obviously one thing that someone might want to do is to notify on > > protocol installs and trap installs of this protocol - e.g. so that > > something other than UefiBootManagerLib can manage and monitor HTTP > > boot, but still allowing the original callback to occur, by hooking it. > > Not sure if this counts as 'supported' or not (possibly not...) though I > > think it may count as 'quite likely to happen'. However, one could hook > > in such a way that the uninstall would succeed anyway, assuming that the > > function pointer within the original installed protocol is writeable. > > > > My question is: was the above is roughly what you were thinking of, that > > might cause the assert to fail, or, if not, if you had the time to give > > a very brief sketch of what else it might be (just a plausible, very > > rough example)? Certainly not saying you're wrong, just that it would be > > helpful (to me!) to understand what sort of thing you were thinking of! > > I don't have a specific use case in mind for why someone might want to > have opened this particular protocol in a way that would subsequently > cause UninstallMultipleProtocolInterfaces() to fail (e.g. opening with > BY_CHILD_CONTROLLER attributes). Just that, as a general rule, there > exists a design flaw in the UEFI specification that means that > operations that should have been chosen at the design stage to be > conceptually impossible to fail (such as freeing memory or uninstalling > protocols) are instead allowed to return a failure status. > > This design issue manifests itself as extremely unreliable behaviour on > the removal or shutdown paths of many UEFI drivers. For example: many > drivers will simply deadlock the system if disconnected from their > underlying controllers (e.g. via the UEFI shell "disconnect" command). > > In the case of UninstallMultipleProtocolInterfaces(), the failure mode > is particularly problematic since the specification dictates that the > firmware must do the absolutely worst thing possible by *reinstalling* > any protocol instances that it had managed to uninstall, and > consequently retriggering driver Start() method calls. This generally > leads to chaos and confusion (and use-after-free bugs that could > probably be fairly easily extended to obtain a Secure Boot exploit). > > There's nothing that you really need to do specifically in HttpBootDxe > to work around this design flaw. But it's definitely worth removing the > unjustified ASSERT(), since that ASSERT() may cause a crash in a system > that could otherwise continue to operate successfully. > > Hope that helps, > > Michael > It does help. Thank you for a useful and clear explanation - I was already aware of some (but certainly not all) of it. I have already posted a revised patch with the ASSERT removed - but am now more clear that I really had better stick with that, not try to argue against it. ;) Thanks again, Mike -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#118052): https://edk2.groups.io/g/devel/message/118052 Mute This Topic: https://groups.io/mt/105368366/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] NetworkPkg/HttpBootDxe: Correctly uninstall HttpBootCallbackProtocol
Dear Michael, I don't know if you had time to answer one follow-up question. Obviously one thing that someone might want to do is to notify on protocol installs and trap installs of this protocol - e.g. so that something other than UefiBootManagerLib can manage and monitor HTTP boot, but still allowing the original callback to occur, by hooking it. Not sure if this counts as 'supported' or not (possibly not...) though I think it may count as 'quite likely to happen'. However, one could hook in such a way that the uninstall would succeed anyway, assuming that the function pointer within the original installed protocol is writeable. My question is: was the above is roughly what you were thinking of, that might cause the assert to fail, or, if not, if you had the time to give a very brief sketch of what else it might be (just a plausible, very rough example)? Certainly not saying you're wrong, just that it would be helpful (to me!) to understand what sort of thing you were thinking of! Many thanks in advance for any time you might have to reply. Mike Beaton -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#118016): https://edk2.groups.io/g/devel/message/118016 Mute This Topic: https://groups.io/mt/105368366/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] NetworkPkg/HttpBootDxe: Correctly uninstall HttpBootCallbackProtocol
Does anybody have any suggestions who I should cc as regards this? Is it preferred to make a bugtracker issue too? -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117500): https://edk2.groups.io/g/devel/message/117500 Mute This Topic: https://groups.io/mt/105371091/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v2] NetworkPkg/HttpBootDxe: Correctly uninstall HttpBootCallbackProtocol
The existing uninstall call was passing the wrong handle (parent object,
not the correct child object) and additionally passing the address
of a pointer to the interface to be removed rather than the pointer
itself, so always failed with EFI_NOT_FOUND.
While it might seem attractive to ASSERT to ensure that the uninstall
proceeds as expected, uninstallation of protocol interfaces is allowed
to fail under the UEFI model. An ASSERT could therefore result in a
sequence of events which is perfectly valid - or at least is out of
the control of this driver - resulting in an ASSERT() failure.
Cc: Michael Brown
Cc: Maciej Rabeda
Cc: Jiaxin Wu
Signed-off-by: Mike Beaton
xyz
---
NetworkPkg/HttpBootDxe/HttpBootImpl.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/NetworkPkg/HttpBootDxe/HttpBootImpl.c
b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
index b4c61925b9..f78eef4a83 100644
--- a/NetworkPkg/HttpBootDxe/HttpBootImpl.c
+++ b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
@@ -77,11 +77,19 @@ HttpBootUninstallCallback (
IN HTTP_BOOT_PRIVATE_DATA *Private
)
{
+ EFI_HANDLE ControllerHandle;
+
if (Private->HttpBootCallback == >LoadFileCallback) {
+if (!Private->UsingIpv6) {
+ ControllerHandle = Private->Ip4Nic->Controller;
+} else {
+ ControllerHandle = Private->Ip6Nic->Controller;
+}
+
gBS->UninstallProtocolInterface (
- Private->Controller,
+ ControllerHandle,
,
- >HttpBootCallback
+ Private->HttpBootCallback
);
Private->HttpBootCallback = NULL;
}
--
2.44.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117470): https://edk2.groups.io/g/devel/message/117470
Mute This Topic: https://groups.io/mt/105371091/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH] NetworkPkg/HttpBootDxe: Correctly uninstall HttpBootCallbackProtocol
The existing uninstall call was passing the wrong handle (parent object,
not the correct child object) and additionally passing the address
of a pointer to the interface to be removed rather than the pointer
itself, so always failed with EFI_NOT_FOUND. After altering these, we
add an ASSERT which confirms that the modified uninstall is succeeding.
Cc: Maciej Rabeda
Cc: Jiaxin Wu
Cc: Siyuan Fu
Signed-off-by: Mike Beaton
---
NetworkPkg/HttpBootDxe/HttpBootImpl.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/NetworkPkg/HttpBootDxe/HttpBootImpl.c
b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
index b4c61925b9..100b721ad4 100644
--- a/NetworkPkg/HttpBootDxe/HttpBootImpl.c
+++ b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
@@ -77,12 +77,23 @@ HttpBootUninstallCallback (
IN HTTP_BOOT_PRIVATE_DATA *Private
)
{
+ EFI_STATUS Status;
+ EFI_HANDLE ControllerHandle;
+
if (Private->HttpBootCallback == >LoadFileCallback) {
-gBS->UninstallProtocolInterface (
- Private->Controller,
- ,
- >HttpBootCallback
- );
+if (!Private->UsingIpv6) {
+ ControllerHandle = Private->Ip4Nic->Controller;
+} else {
+ ControllerHandle = Private->Ip6Nic->Controller;
+}
+
+Status = gBS->UninstallProtocolInterface (
+ControllerHandle,
+,
+Private->HttpBootCallback
+);
+ASSERT_EFI_ERROR (Status);
+
Private->HttpBootCallback = NULL;
}
}
--
2.44.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117468): https://edk2.groups.io/g/devel/message/117468
Mute This Topic: https://groups.io/mt/105368366/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V4 2/2] ArmPkg: Remove manual exclusion of debug vars when MDEPKG_NDEBUG is not defined
> For the exception handler case, we can just drop the #Ifdefs around > the definition of BaseName () entirely given that it will now always > be referenced. But that does depend a lot on how other toolchains deal > with this (VS201x primarily) The V5/V6 version of the patch builds on VS2019 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112602): https://edk2.groups.io/g/devel/message/112602 Mute This Topic: https://groups.io/mt/103166254/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V6] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined
I did ask. Thank you for the considered answer. Ack. :) On Thu, 14 Dec 2023 at 13:25, Laszlo Ersek wrote: > > On 12/14/23 10:33, Mike Beaton wrote: > >> Please stop sending patches. > > > > I believe this version is a clear improvement, with motivation. > > (Certainly, it was meant as such!) > > > > How am I meant to send improvements or updates in this email-based workflow? > > By pacing yourself. Posting two versions of the same patch set on the > same day is usually the highest tolerable posting frequency. Many would > say 1 version/day is the limit (unless there is a pressing security > issue or CI failure etc). > > Basically the request is for the submitter to think more, let their > latest version soak for longer locally, before posting it. > > BTW I don't think this is specific to email, the same issue exists on > any git forge, except perhaps to a lesser extent. Both channels are > asynchronous, so if you repost or force-push too quickly, you don't give > reviewers a chance to finish (or even *start*) the last version's > review. Well, interrupting them may actually be your intent, but that's > just not how async communication works. Once you posted it, it's out > there, and it's going to take up some consumer cycles for sure, either > way, regardless of what you do later. You can't recall it, you're not in > the same office at the same time. > > github *seems* to mitigate this, because the old version more or less > just disappears. But that's actually a bug of git forges, not a feature. > Patch posting history should never be forgotten. Mailing lists get this > right, but that makes misbehavior (= too frequent posting) more > damaging, as the total traffic the receiver will have to wade through > will be higher. > > In short: don't experiment, don't thrash. Make every version of your > patch set *count*. Give yourself more time to think about your latest > version *in the background*, before posting it. If you sleep over it, > the next day you might get a new idea, regardless of whether you posted > or didn't yet post that version. So, as long as it hasn't settled, don't > post it. If you realize an issue after posting the latest version, > respond -- just like a reviewer would -- to the problematic patch email, > pointing out the error; but *don't* post a new version just yet (i.e., > don't create a new version / thread on the list). Your attempt to > "recall" the problematic version is bound to fail. > > In short, s/TCP_NODELAY/TCP_CORK/. > > Sorry about the sermon -- you asked. :) > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112527): https://edk2.groups.io/g/devel/message/112527 Mute This Topic: https://groups.io/mt/103166935/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V5] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined
> IOW, please don't send a v6 until the discussion comes to a conclusion. Apologies, I did _not_ see this before sending. > > - #if !defined (MDEPKG_NDEBUG) > > + #if defined (__CC_ARM) || defined (__GNUC__) > > No, this is not going to be acceptable to me. You noted that only the > ARM code seems to suffer from this issue, so surely, we can find a way > to change this code that doesn't introduce spurious dependencies on > the exact toolchain we are using. With all due respect, I believe that you are incorrect that this is spurious. Please check the surrounding code (specifically, this compiler dependency exactly matches - and is there because of - a compiler dependency in the surrounding code). (Additionally, though it doesn't affect the point either way, I thought this is what you had spotted and were asking for, when previously saying "What about GCC?"!) Mike -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112515): https://edk2.groups.io/g/devel/message/112515 Mute This Topic: https://groups.io/mt/103166459/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V6] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined
> Please stop sending patches. I believe this version is a clear improvement, with motivation. (Certainly, it was meant as such!) How am I meant to send improvements or updates in this email-based workflow? Mike -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112514): https://edk2.groups.io/g/devel/message/112514 Mute This Topic: https://groups.io/mt/103166935/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V6] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined
Repeats V5, but with a hopefully clearer (on motivation and history) commit message. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112511): https://edk2.groups.io/g/devel/message/112511 Mute This Topic: https://groups.io/mt/103166935/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH V6] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined
From: Mike Beaton
The variant provided when MDEPKG_NDEBUG is defined will be optimised
away in RELEASE builds, but by referencing the argument list, avoids
unused variable errors from valid debug code, for example when STATIC
variables are used only in DEBUG statements.
This issue was being caused by variable EventNames in
OvmfPkg/VirtioSerialDxe/VirtioSerial.c in CLANGPDB X64 RELEASE build
prior to this commit, and was also being caused by the same variable
in CLANGDWARF X64 RELEASE build, prior to
d3225577123767fd09c91201d27e9c91663ae132 - which we
revert, restoring the desirable feature of failure to build on
genuinely (unintentionally) unused variables when building in either
clang toolchain.
Also change manual exclusion of debug vars when MDEPKG_NDEBUG is
not defined in a similar cases in ArmPkg, to inclusion when used
regardless of MDEPKG_NDEBUG (the revised DEBUG macro automatically
compiles away unused variable accesses, but there has to be a
variable, access to which to discard).
Signed-off-by: Mike Beaton
---
.../DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c | 4 ++--
.../AArch64/DefaultExceptionHandler.c | 3 ---
BaseTools/Conf/tools_def.template | 2 +-
MdePkg/Include/Library/DebugLib.h | 7 ++-
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git
a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
index 432112354f..c5c53ef3e1 100644
--- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
+++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
@@ -71,7 +71,7 @@ PeCoffLoaderRelocateImageExtraAction (
IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext
)
{
- #if !defined (MDEPKG_NDEBUG)
+ #if defined (__CC_ARM) || defined (__GNUC__)
CHAR8 Temp[512];
#endif
@@ -106,7 +106,7 @@ PeCoffLoaderUnloadImageExtraAction (
IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext
)
{
- #if !defined (MDEPKG_NDEBUG)
+ #if defined (__CC_ARM) || defined (__GNUC__)
CHAR8 Temp[512];
#endif
diff --git
a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
index a39896d576..1d3ea61311 100644
---
a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
+++
b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
@@ -157,7 +157,6 @@ DescribeExceptionSyndrome (
DEBUG ((DEBUG_ERROR, "\n %a \n", Message));
}
-#ifndef MDEPKG_NDEBUG
STATIC
CONST CHAR8 *
BaseName (
@@ -177,8 +176,6 @@ BaseName (
return Str;
}
-#endif
-
/**
This is the default action to take on an unexpected exception
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=
-Wl,--defsym=PECOFF_HEADER_SIZE=0x22
DEFINE CLANGDWARF_IA32_TARGET = -target i686-pc-linux-gnu
DEFINE CLANGDWARF_X64_TARGET = -target x86_64-pc-linux-gnu
-DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
-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
+DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
-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
DEFINE CLANGDWARF_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS)
DEF(CLANGDWARF_WARNING_OVERRIDES) -fno-stack-protector -mms-bitfields
-Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas
-Wno-incompatible-library-redeclaration -fno-asynchronous-unwind-tables
-mno-sse -mno-mmx -msoft-float -mno-implicit-float
-ftrap-function=undefined_behavior_has_been_optimized_away_by_clang
-funsigned-char -fno-ms-extensions -Wno-null-dereference
###
diff --git a/MdePkg/Include/Library/DebugLib.h
b/MdePkg/Include/Library/DebugLib.h
index 40772f2e0f..bc7789f01c 100644
--- 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
/**
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112510): https://edk2.groups.io/g/devel/mess
[edk2-devel] [PATCH V5] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined
From: Mike Beaton
The variant provided when MDEPKG_NDEBUG is defined will be optimised
away in RELEASE builds, but by referencing the argument list, avoids
unused variable errors from valid debug code, for example when STATIC
variables are used only in DEBUG statements.
Variable EventNames in OvmfPkg/VirtioSerialDxe/VirtioSerial.c
was causing this issue in CLANGPDB X64 RELEASE build, prior to this
commit.
We also change manual exclusion of debug vars when MDEPKG_NDEBUG is
not defined, in a similar case in
ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
to inclusion when used regardless of MDEPKG_NDEBUG (the revised DEBUG
macro automatically compiles away unused variable accesses, but there
has to be a variable, access to which to discard).
Signed-off-by: Mike Beaton
---
.../DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c | 4 ++--
.../AArch64/DefaultExceptionHandler.c | 3 ---
BaseTools/Conf/tools_def.template | 2 +-
MdePkg/Include/Library/DebugLib.h | 7 ++-
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git
a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
index 432112354f..c5c53ef3e1 100644
--- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
+++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
@@ -71,7 +71,7 @@ PeCoffLoaderRelocateImageExtraAction (
IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext
)
{
- #if !defined (MDEPKG_NDEBUG)
+ #if defined (__CC_ARM) || defined (__GNUC__)
CHAR8 Temp[512];
#endif
@@ -106,7 +106,7 @@ PeCoffLoaderUnloadImageExtraAction (
IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext
)
{
- #if !defined (MDEPKG_NDEBUG)
+ #if defined (__CC_ARM) || defined (__GNUC__)
CHAR8 Temp[512];
#endif
diff --git
a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
index a39896d576..1d3ea61311 100644
---
a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
+++
b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
@@ -157,7 +157,6 @@ DescribeExceptionSyndrome (
DEBUG ((DEBUG_ERROR, "\n %a \n", Message));
}
-#ifndef MDEPKG_NDEBUG
STATIC
CONST CHAR8 *
BaseName (
@@ -177,8 +176,6 @@ BaseName (
return Str;
}
-#endif
-
/**
This is the default action to take on an unexpected exception
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=
-Wl,--defsym=PECOFF_HEADER_SIZE=0x22
DEFINE CLANGDWARF_IA32_TARGET = -target i686-pc-linux-gnu
DEFINE CLANGDWARF_X64_TARGET = -target x86_64-pc-linux-gnu
-DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
-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
+DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
-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
DEFINE CLANGDWARF_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS)
DEF(CLANGDWARF_WARNING_OVERRIDES) -fno-stack-protector -mms-bitfields
-Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas
-Wno-incompatible-library-redeclaration -fno-asynchronous-unwind-tables
-mno-sse -mno-mmx -msoft-float -mno-implicit-float
-ftrap-function=undefined_behavior_has_been_optimized_away_by_clang
-funsigned-char -fno-ms-extensions -Wno-null-dereference
###
diff --git a/MdePkg/Include/Library/DebugLib.h
b/MdePkg/Include/Library/DebugLib.h
index 40772f2e0f..bc7789f01c 100644
--- 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
/**
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112506): https://edk2.groups.io/g/devel/message/112506
Mute This Topic: https://groups.io/mt/103166459/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V4 2/2] ArmPkg: Remove manual exclusion of debug vars when MDEPKG_NDEBUG is not defined
Hi, I'm not fully used to email-based git commits. In this case, I've got two commits with different messages which are meant to make one patch set. I was under the impression that the first line of each commit is taken from the subject, but in that case these cease to appear as one thread in edk2-devel. Not yet sure how to handle? Apologies! -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112503): https://edk2.groups.io/g/devel/message/112503 Mute This Topic: https://groups.io/mt/103166254/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/2] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined
Apologies - sent in error without V4 tag, please ignore. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112502): https://edk2.groups.io/g/devel/message/112502 Mute This Topic: https://groups.io/mt/103166248/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH V4 2/2] ArmPkg: Remove manual exclusion of debug vars when MDEPKG_NDEBUG is not defined
From: Mike Beaton
This is no longer required since the revised DEBUG macro automatically
compiles away unused var accesses when MDEPKG_NDEBUG is defined;
keeping these lines is incompatible with the updated DEBUG macro, as
there has to be a variable, access to which to discard.
Signed-off-by: Mike Beaton
---
.../DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c | 4
.../AArch64/DefaultExceptionHandler.c | 3 ---
2 files changed, 7 deletions(-)
diff --git
a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
index 432112354f..dc49e8eba7 100644
--- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
+++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
@@ -71,9 +71,7 @@ PeCoffLoaderRelocateImageExtraAction (
IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext
)
{
- #if !defined (MDEPKG_NDEBUG)
CHAR8 Temp[512];
- #endif
if (ImageContext->PdbPointer) {
#ifdef __CC_ARM
@@ -106,9 +104,7 @@ PeCoffLoaderUnloadImageExtraAction (
IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext
)
{
- #if !defined (MDEPKG_NDEBUG)
CHAR8 Temp[512];
- #endif
if (ImageContext->PdbPointer) {
#ifdef __CC_ARM
diff --git
a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
index a39896d576..1d3ea61311 100644
---
a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
+++
b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
@@ -157,7 +157,6 @@ DescribeExceptionSyndrome (
DEBUG ((DEBUG_ERROR, "\n %a \n", Message));
}
-#ifndef MDEPKG_NDEBUG
STATIC
CONST CHAR8 *
BaseName (
@@ -177,8 +176,6 @@ BaseName (
return Str;
}
-#endif
-
/**
This is the default action to take on an unexpected exception
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112501): https://edk2.groups.io/g/devel/message/112501
Mute This Topic: https://groups.io/mt/103166254/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH V4 1/2] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined
From: Mike Beaton
The variant provided when MDEPKG_NDEBUG is defined will be optimised
away in RELEASE builds, but by referencing the argument list, avoids
unused variable errors from valid debug code, for example when STATIC
variables are used only in DEBUG statements.
Variables EventNames in OvmfPkg/VirtioSerialDxe/VirtioSerial.c
was causing this issue in CLANGPDB X64 RELEASE build, prior to this
commit. It is also necessary to remove manual work-arounds which had
been applied to some similar cases in ArmPkg (see following commit).
Signed-off-by: Mike Beaton
---
BaseTools/Conf/tools_def.template | 2 +-
MdePkg/Include/Library/DebugLib.h | 7 ++-
2 files changed, 7 insertions(+), 2 deletions(-)
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=
-Wl,--defsym=PECOFF_HEADER_SIZE=0x22
DEFINE CLANGDWARF_IA32_TARGET = -target i686-pc-linux-gnu
DEFINE CLANGDWARF_X64_TARGET = -target x86_64-pc-linux-gnu
-DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
-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
+DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
-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
DEFINE CLANGDWARF_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS)
DEF(CLANGDWARF_WARNING_OVERRIDES) -fno-stack-protector -mms-bitfields
-Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas
-Wno-incompatible-library-redeclaration -fno-asynchronous-unwind-tables
-mno-sse -mno-mmx -msoft-float -mno-implicit-float
-ftrap-function=undefined_behavior_has_been_optimized_away_by_clang
-funsigned-char -fno-ms-extensions -Wno-null-dereference
###
diff --git a/MdePkg/Include/Library/DebugLib.h
b/MdePkg/Include/Library/DebugLib.h
index 40772f2e0f..bc7789f01c 100644
--- 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
/**
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112500): https://edk2.groups.io/g/devel/message/112500
Mute This Topic: https://groups.io/mt/103166252/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH 1/2] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined
From: Mike Beaton
The variant provided when MDEPKG_NDEBUG is defined will be optimised
away in RELEASE builds, but by referencing the argument list, avoids
unused variable errors from valid debug code, for example when STATIC
variables are used only in DEBUG statements.
Variables EventNames in OvmfPkg/VirtioSerialDxe/VirtioSerial.c
was causing this issue in CLANGPDB X64 RELEASE build, prior to this
commit. It is also necessary to remove manual work-arounds which had
been applied to some similar cases in ArmPkg (see following commit).
Signed-off-by: Mike Beaton
---
BaseTools/Conf/tools_def.template | 2 +-
MdePkg/Include/Library/DebugLib.h | 7 ++-
2 files changed, 7 insertions(+), 2 deletions(-)
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=
-Wl,--defsym=PECOFF_HEADER_SIZE=0x22
DEFINE CLANGDWARF_IA32_TARGET = -target i686-pc-linux-gnu
DEFINE CLANGDWARF_X64_TARGET = -target x86_64-pc-linux-gnu
-DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
-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
+DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
-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
DEFINE CLANGDWARF_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS)
DEF(CLANGDWARF_WARNING_OVERRIDES) -fno-stack-protector -mms-bitfields
-Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas
-Wno-incompatible-library-redeclaration -fno-asynchronous-unwind-tables
-mno-sse -mno-mmx -msoft-float -mno-implicit-float
-ftrap-function=undefined_behavior_has_been_optimized_away_by_clang
-funsigned-char -fno-ms-extensions -Wno-null-dereference
###
diff --git a/MdePkg/Include/Library/DebugLib.h
b/MdePkg/Include/Library/DebugLib.h
index 40772f2e0f..bc7789f01c 100644
--- 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
/**
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112499): https://edk2.groups.io/g/devel/message/112499
Mute This Topic: https://groups.io/mt/103166248/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V3] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined
Got it. These are the fixes for ArmVirtQemu.dsc. If people have
already manually worked round this issue it interferes with this
global fix (we had this in one file in Acidanthera code). There don't
seem to be many instances of that in the entire EDK-II codebase,
however, fortunately.
diff --git
a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
index 432112354f..dc49e8eba7 100644
--- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
+++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
@@ -71,9 +71,7 @@ PeCoffLoaderRelocateImageExtraAction (
IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext
)
{
- #if !defined (MDEPKG_NDEBUG)
CHAR8 Temp[512];
- #endif
if (ImageContext->PdbPointer) {
#ifdef __CC_ARM
@@ -106,9 +104,7 @@ PeCoffLoaderUnloadImageExtraAction (
IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext
)
{
- #if !defined (MDEPKG_NDEBUG)
CHAR8 Temp[512];
- #endif
if (ImageContext->PdbPointer) {
#ifdef __CC_ARM
diff --git
a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
index a39896d576..1d3ea61311 100644
---
a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
+++
b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
@@ -157,7 +157,6 @@ DescribeExceptionSyndrome (
DEBUG ((DEBUG_ERROR, "\n %a \n", Message));
}
-#ifndef MDEPKG_NDEBUG
STATIC
CONST CHAR8 *
BaseName (
@@ -177,8 +176,6 @@ BaseName (
return Str;
}
-#endif
-
/**
This is the default action to take on an unexpected exception
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112494): https://edk2.groups.io/g/devel/message/112494
Mute This Topic: https://groups.io/mt/103160238/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V3] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined
I have currently gone with `if (FALSE)` in the above, even though Michael Brown kindly offered tests which showed that `if ((FALSE))` might help in a closely related context - i.e. when discussing Mikhail's variant of this and related code - two years ago now: https://edk2.groups.io/g/devel/message/83938 (and see https://bugzilla.tianocore.org/show_bug.cgi?id=3704). I cannot replicate that finding, or so far find any info about it, but possibly it relates to an older version of clang? (Which if so, possibly means it doesn't matter now anyway?) >From my own tests and fairly extensive Acidanthera CI suite, `if (FALSE)` works on all current toolchains. @Ard - Does this variant still fail on Arm? It would be interesting to know which specific build of what is/was failing. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112493): https://edk2.groups.io/g/devel/message/112493 Mute This Topic: https://groups.io/mt/103160238/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH V3] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined
From: Mike Beaton
The variant provided when MDEPKG_NDEBUG is defined will be optimised
away in RELEASE builds, but by referencing the argument list, avoids
unused variable errors from valid debug code, for example when STATIC
variables are used only in DEBUG statements.
Signed-off-by: Mike Beaton
---
BaseTools/Conf/tools_def.template | 2 +-
MdePkg/Include/Library/DebugLib.h | 7 ++-
2 files changed, 7 insertions(+), 2 deletions(-)
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=
-Wl,--defsym=PECOFF_HEADER_SIZE=0x22
DEFINE CLANGDWARF_IA32_TARGET = -target i686-pc-linux-gnu
DEFINE CLANGDWARF_X64_TARGET = -target x86_64-pc-linux-gnu
-DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
-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
+DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
-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
DEFINE CLANGDWARF_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS)
DEF(CLANGDWARF_WARNING_OVERRIDES) -fno-stack-protector -mms-bitfields
-Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas
-Wno-incompatible-library-redeclaration -fno-asynchronous-unwind-tables
-mno-sse -mno-mmx -msoft-float -mno-implicit-float
-ftrap-function=undefined_behavior_has_been_optimized_away_by_clang
-funsigned-char -fno-ms-extensions -Wno-null-dereference
###
diff --git a/MdePkg/Include/Library/DebugLib.h
b/MdePkg/Include/Library/DebugLib.h
index 40772f2e0f..bc7789f01c 100644
--- 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
/**
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112492): https://edk2.groups.io/g/devel/message/112492
Mute This Topic: https://groups.io/mt/103160238/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] DebugLib: Allow -Wunneeded-internal-declaration with clang
> > +#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.
```
$ cat test.c
static int a = 0;
void foo()
{
if ((0)) {
(void)((void *)(long)0, a);
}
}
$ clang -c -Wall test.c -o test.o
test.c:6:12: warning: expression result unused; should this cast be to
'void'? [-Wunused-value]
(void)((void *)(long)0, a);
^ ~
1 warning generated.
```
Without the pragmas, various DEBUG statements sent into the macro
generate unused *value* warnings exactly such as the above. Once we
suppress these unused value warnings, the existence of the
comma-operator separated list of values, cast to void, avoids unused
*variable* warnings, such as the unused variable warning that would
otherwise occur about `a` above - allowing `a` to instead just be
optimised away when it becomes unused in this way, as we want.
For the other questions, let me get back to you! Thank you.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112461): https://edk2.groups.io/g/devel/message/112461
Mute This Topic: https://groups.io/mt/103138778/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] DebugLib: Allow -Wunneeded-internal-declaration with clang
On Tue, 12 Dec 2023 at 21:45, 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
> ---
> 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=
> -Wl,--defsym=PECOFF_HEADER_SIZE=0x22
> DEFINE CLANGDWARF_IA32_TARGET = -target i686-pc-linux-gnu
> DEFINE CLANGDWARF_X64_TARGET = -target x86_64-pc-linux-gnu
>
> -DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
> -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
> +DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
> -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
> DEFINE CLANGDWARF_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS)
> DEF(CLANGDWARF_WARNING_OVERRIDES) -fno-stack-protector -mms-bitfields
> -Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas
> -Wno-incompatible-library-redeclaration -fno-asynchronous-unwind-tables
> -mno-sse -mno-mmx -msoft-float -mno-implicit-float
> -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang
> -funsigned-char -fno-ms-extensions -Wno-null-dereference
>
> ###
> diff --git a/MdePkg/Include/Library/DebugLib.h
> b/MdePkg/Include/Library/DebugLib.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__)
> +#define DEBUG(Expression)\
> +do { \
> + _Pragma("GCC diagnostic push") \
> + _Pragma("GCC diagnostic ignored \"-Wunused-value\"") \
> + if ((FALSE)) { \
> +(VOID) Expression; \
> + }\
> + _Pragma("GCC diagnostic pop")\
> +} while (FALSE)
> #else
> #define DEBUG(Expression)
> #endif
> --
> 2.39.2
>
Hi Mikhail,
Are you okay to reply confirming that you're happy with your
Signed-off-by in the above?
Thanks,
Mike
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112458): https://edk2.groups.io/g/devel/message/112458
Mute This Topic: https://groups.io/mt/103138778/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH V2] DebugLib: Allow -Wunneeded-internal-declaration with clang
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
---
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=
-Wl,--defsym=PECOFF_HEADER_SIZE=0x22
DEFINE CLANGDWARF_IA32_TARGET = -target i686-pc-linux-gnu
DEFINE CLANGDWARF_X64_TARGET = -target x86_64-pc-linux-gnu
-DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
-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
+DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
-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
DEFINE CLANGDWARF_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS)
DEF(CLANGDWARF_WARNING_OVERRIDES) -fno-stack-protector -mms-bitfields
-Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas
-Wno-incompatible-library-redeclaration -fno-asynchronous-unwind-tables
-mno-sse -mno-mmx -msoft-float -mno-implicit-float
-ftrap-function=undefined_behavior_has_been_optimized_away_by_clang
-funsigned-char -fno-ms-extensions -Wno-null-dereference
###
diff --git a/MdePkg/Include/Library/DebugLib.h
b/MdePkg/Include/Library/DebugLib.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__)
+#define DEBUG(Expression)\
+do { \
+ _Pragma("GCC diagnostic push") \
+ _Pragma("GCC diagnostic ignored \"-Wunused-value\"") \
+ if ((FALSE)) { \
+(VOID) Expression; \
+ }\
+ _Pragma("GCC diagnostic pop")\
+} while (FALSE)
#else
#define DEBUG(Expression)
#endif
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112457): https://edk2.groups.io/g/devel/message/112457
Mute This Topic: https://groups.io/mt/103138778/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang
> I believe one way to approach it would be to (a) amend the patch with > >--author="Marvin Häuser " > > and (b) have S-o-b's from both Marvin and you at the end of the commit > message. > As I say, I think the code in question is actual by Mikhail, despite appearances, but I should be able to get one of them to sign it off. Cheers. > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112450): https://edk2.groups.io/g/devel/message/112450 Mute This Topic: https://groups.io/mt/103126777/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang
> You failed to mention where you found this patch. It's from https://github.com/acidanthera/audk/commit/dcd0a768b0f (which actually contains the code described in its commit message, but I believe some other code, including this specific part, which got squashed in at some point, and I believe is from the committer, not the alleged author actually!). Would it work to just add that origin commit to the commit message (given the licensing, which is the same as EDK-II, ofc), or would you prefer/need an additional Signed-off-by:? (Instead?) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112390): https://edk2.groups.io/g/devel/message/112390 Mute This Topic: https://groups.io/mt/103126777/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang
> This seems redundant to me. Either we set the pragma and the compiler > does not care, or we don't, and rely on the fact that the compiler can > infer that 'Expression' will never be evaluated at runtime, but won't > complain about symbols that are only referenced via 'Expression' and > nowhere else. I thought so too on initially reading the code, so I tried removing the pragmas, and in fact the pragma is to tell the compiler not to warn about the contained `(VOID) Expression`, not to fix the actual problem warning - which `(VOID) Expression` itself does, once allowed. So without the pragmas we get instead errors such as: ``` /home/mjsbeaton/OpenSource/edk2/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c:444:86: error: expression result unused; should this cast be to 'void'? [-Werror,-Wunused-value] DEBUG ((DEBUG_INFO | DEBUG_LOAD, "Loading DXE CORE at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN)DxeCoreAddress, FUNCTION_ENTRY_POINT (DxeCoreEntryPoint))); ^ ~ /home/mjsbeaton/OpenSource/edk2/MdePkg/Include/Library/DebugLib.h:432:16: note: expanded from macro 'DEBUG' (VOID) Expression; \ ^ 1 error generated. ``` -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112387): https://edk2.groups.io/g/devel/message/112387 Mute This Topic: https://groups.io/mt/103126777/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
> I'm running some further tests to confirm that the simple version > proposed above builds in all the Acidanthera test environments, which > is relevant in that it should rule out at least any obvious problems > with it building in all edk-2 environments, too. (I had already run > quick tests for CLANGDWARF, CLANGPDB and GCC RELEASE in edk2, with no > problems.) If there aren't any issues there, I'd prefer to submit a > patch for this simple version instead (combined with removing the > -Wno-unneeded-internal-declaration for clangdwarf). Apologies for the running commentary, from these tests it appears that the VS2019 toolchain does not like the 'simple' version of the fix. So I'm back to proposing the version in https://edk2.groups.io/g/devel/topic/103126777 . -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112386): https://edk2.groups.io/g/devel/message/112386 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
> I have realised that this was already fixed (i.e. allowing keeping the > warning) in Acidanthera fork of EDK-II. Discussed here > https://bugzilla.tianocore.org/show_bug.cgi?id=3704 - includes the fix > in question and other fixes for newer gcc as well. I'll post a new > patch to the list proposing just the relevant fix for clang. I'm discussing this with the Acidanthera authors, and coming round to prefer the version proposed above, with no pragmas, actually. I think Ard is saying that, while I misdescribed somewhat when things would be optimised away, that they will be optimised away, and that probably the object files wouldn't be polluted, with this version? I'm running some further tests to confirm that the simple version proposed above builds in all the Acidanthera test environments, which is relevant in that it should rule out at least any obvious problems with it building in all edk-2 environments, too. (I had already run quick tests for CLANGDWARF, CLANGPDB and GCC RELEASE in edk2, with no problems.) If there aren't any issues there, I'd prefer to submit a patch for this simple version instead (combined with removing the -Wno-unneeded-internal-declaration for clangdwarf). -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112385): https://edk2.groups.io/g/devel/message/112385 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] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang
From: Mike Beaton
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, therefore we can keep
-Wunneeded-internal-declaration (as part of -Wall) to warn about any
mistakenly genuinely unused variables.
Signed-off-by: Mike Beaton
---
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=
-Wl,--defsym=PECOFF_HEADER_SIZE=0x22
DEFINE CLANGDWARF_IA32_TARGET = -target i686-pc-linux-gnu
DEFINE CLANGDWARF_X64_TARGET = -target x86_64-pc-linux-gnu
-DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
-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
+DEFINE CLANGDWARF_WARNING_OVERRIDES= -Wno-parentheses-equality
-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
DEFINE CLANGDWARF_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS)
DEF(CLANGDWARF_WARNING_OVERRIDES) -fno-stack-protector -mms-bitfields
-Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas
-Wno-incompatible-library-redeclaration -fno-asynchronous-unwind-tables
-mno-sse -mno-mmx -msoft-float -mno-implicit-float
-ftrap-function=undefined_behavior_has_been_optimized_away_by_clang
-funsigned-char -fno-ms-extensions -Wno-null-dereference
###
diff --git a/MdePkg/Include/Library/DebugLib.h
b/MdePkg/Include/Library/DebugLib.h
index f0c9f64487..e2158b1a3d 100644
--- a/MdePkg/Include/Library/DebugLib.h
+++ b/MdePkg/Include/Library/DebugLib.h
@@ -425,6 +425,16 @@ UnitTestDebugAssert (
_DEBUG (Expression); \
}\
} while (FALSE)
+#elif defined (__clang__)
+#define DEBUG(Expression)\
+do { \
+ _Pragma("GCC diagnostic push") \
+ _Pragma("GCC diagnostic ignored \"-Wunused-value\"") \
+ if ((FALSE)) { \
+(VOID) Expression; \
+ }\
+ _Pragma("GCC diagnostic pop")\
+} while (FALSE)
#else
#define DEBUG(Expression)
#endif
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112363): https://edk2.groups.io/g/devel/message/112363
Mute This Topic: https://groups.io/mt/103126777/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
I have realised that this was already fixed (i.e. allowing keeping the
warning) in Acidanthera fork of EDK-II. Discussed here
https://bugzilla.tianocore.org/show_bug.cgi?id=3704 - includes the fix
in question and other fixes for newer gcc as well. I'll post a new
patch to the list proposing just the relevant fix for clang.
On Tue, 12 Dec 2023 at 07:49, Ard Biesheuvel wrote:
>
> On Tue, 12 Dec 2023 at 08:17, Mike Beaton wrote:
> >
> > > > A completely different approach, which allows clang to spot that the
> > > > usage has been 'optimised away' and so to not complain (and therefore
> > > > allows us to re-enable the warning in CLANGDWARF as well), is the
> > > > following:
> > > >
> > > > --- a/MdePkg/Include/Library/DebugLib.h
> > > > +++ b/MdePkg/Include/Library/DebugLib.h
> > > > @@ -426,7 +426,10 @@ UnitTestDebugAssert (
> > > >}\
> > > > } while (FALSE)
> > > > #else
> > > > -#define DEBUG(Expression)
> > > > +#define DEBUG(Expression)\
> > > > +if (FALSE) { \
> > > > + _DEBUG (Expression); \
> > > > +}
> > > > #endif
> > > >
> > > > /**
> > > >
> > >
> > > But will this not litter the object files with a bunch of format strings
> > > etc?
> >
> > Yes. Which would be optimized away at link time. (Or rather, I believe
> > so, would need further tests to confirm exactly what is optimized away
> > when.)
> >
>
> Link time optimization does not usually optimize away assets at this
> granularity. Even if --gc-sections is set, the only thing it can
> optimize away is an entire input section.
>
> However, the compiler should be smart enough not to emit any
> references to format strings etc in the first place, as it knows the
> condition can never become true (but NOOPT builds should retain them).
>
> > > It feels like, for CLANGPDB (and maybe CLANGDWARF), the RELEASE target
> > > should not define MDEPKG_NDEBUG, but make sure (instead) that
> > > DebugPrintEnabled() evals to FALSE. If PcdDebugPropertyMask is
> > > fixed-at-build, then DebugPrintEnabled() should be possible to evaluate
> > > at compile time; is that right? (At least for clang?)
> >
> > Yes, that is my understanding of how compile-time Pcds work too - but
> > wouldn't the net result be the same as what I suggested?
>
> It depends on the kind of access. For PCDs that are fixed-at-build
> only, the FixedPcdGet###() accessors will evaluate to constant
> preprocessor expressions, allowing the compiler to do optimizations.
> The ordinary PcdGet###() routines will not produce compile time
> constant expressions in the same way, so there, all the logic is
> retained (again, modulo LTO)
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112362): https://edk2.groups.io/g/devel/message/112362
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
> > A completely different approach, which allows clang to spot that the
> > usage has been 'optimised away' and so to not complain (and therefore
> > allows us to re-enable the warning in CLANGDWARF as well), is the
> > following:
> >
> > --- a/MdePkg/Include/Library/DebugLib.h
> > +++ b/MdePkg/Include/Library/DebugLib.h
> > @@ -426,7 +426,10 @@ UnitTestDebugAssert (
> >}\
> > } while (FALSE)
> > #else
> > -#define DEBUG(Expression)
> > +#define DEBUG(Expression)\
> > +if (FALSE) { \
> > + _DEBUG (Expression); \
> > +}
> > #endif
> >
> > /**
> >
>
> But will this not litter the object files with a bunch of format strings
> etc?
Yes. Which would be optimized away at link time. (Or rather, I believe
so, would need further tests to confirm exactly what is optimized away
when.)
> It feels like, for CLANGPDB (and maybe CLANGDWARF), the RELEASE target
> should not define MDEPKG_NDEBUG, but make sure (instead) that
> DebugPrintEnabled() evals to FALSE. If PcdDebugPropertyMask is
> fixed-at-build, then DebugPrintEnabled() should be possible to evaluate
> at compile time; is that right? (At least for clang?)
Yes, that is my understanding of how compile-time Pcds work too - but
wouldn't the net result be the same as what I suggested?
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112358): https://edk2.groups.io/g/devel/message/112358
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
> > I believe this would be logically wrong, as the other versions still
> > wouldn't compile if you changed the relevant debug Pcds. (Which are
> > logically independent of the compile and link options - e.g. what if for
> > some reason you wanted to single step with the Debug Pcds set to
> > disabled, in a NOOPT build?)
>
> I don't think that use case exists in practice.
>
> Anyway, my suggestion is based on prior art: I *think* we ask gcc to
> whine about unused local variables in RELEASE builds only, too. See
> commits 20d00edf21d2 ("BaseTools/GCC: set -Wno-unused-but-set-variables
> only on RELEASE builds", 2016-03-25) and 8b6366f87584 ("BaseTools/GCC:
> set -Wno-unused-const-variable on RELEASE builds", 2017-09-08).
>
> ... TBH I don't understand the current state of
> "-Wno-unused-but-set-variables" and "-Wno-unused-const-variable" between
> X64 and AARCH64, considering the DEBUG target. Today,
> DEBUG_GCC5_AARCH64_CC_FLAGS disables these warnings, but
> DEBUG_GCC5_X64_CC_FLAGS doesn't, even though *both* macros specify
> -flto. Compare commit 06c8a34cc4bc ("BaseTool/tools_def GCC5: enable
> optimization for ARM/AARCH64 DEBUG builds", 2017-12-08) -- I don't
> understand why "-flto" had to be accompanied by
> "-Wno-unused-but-set-variable -Wno-unused-const-variable" in that commit.
>
> In brief, IA32 and X64 prior art supports my suggestion to shut up the
> warning only for RELEASE (for CLANGPDB too), but ARM/AARCH64 prior art
> contradicts that proposal. IOW, prior art is inconsistent per se... I
> don't understand.
>
> Laszlo
Hunting around further, it is not the Pcds which are causing this to
be optimised away, but the definition of MDEPKG_NDEBUG.
A completely different approach, which allows clang to spot that the
usage has been 'optimised away' and so to not complain (and therefore
allows us to re-enable the warning in CLANGDWARF as well), is the
following:
--- a/MdePkg/Include/Library/DebugLib.h
+++ b/MdePkg/Include/Library/DebugLib.h
@@ -426,7 +426,10 @@ UnitTestDebugAssert (
}\
} while (FALSE)
#else
-#define DEBUG(Expression)
+#define DEBUG(Expression)\
+if (FALSE) { \
+ _DEBUG (Expression); \
+}
#endif
/**
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112323): https://edk2.groups.io/g/devel/message/112323
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
I believe this would be logically wrong, as the other versions still wouldn't compile if you changed the relevant debug Pcds. (Which are logically independent of the compile and link options - e.g. what if for some reason you wanted to single step with the Debug Pcds set to disabled, in a NOOPT build?) On Mon, 11 Dec 2023, 15:00 Laszlo Ersek, wrote: > On 12/10/23 11:18, Mike Beaton wrote: > > 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 > > > > ### > > AFAICT, CLANGPDB_WARNING_OVERRIDES gets included in > CLANGPDB_ALL_CC_FLAGS, which in turn gets included in all three of > DEBUG, RELEASE and NOOPT build target flags. > > The original report was "RELEASE CLANGPDB OVMF currently does not compile". > > Can we use "-Wno-unneeded-internal-declaration" with RELEASE builds only? > > Thanks, > Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112306): https://edk2.groups.io/g/devel/message/112306 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
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] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] RELEASE CLANGPDB OVMF currently does not compile
Dear Ard,
Thanks for your attention on my other issue, about NOOPT CLANGDWARF OVMF.
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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112255): https://edk2.groups.io/g/devel/message/112255
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] CLANGDWARF OVMF hangs on start
I spotted that Ard Biesheuvel authored the original commit https://github.com/tianocore/edk2/commit/28ade7b802e which sets the pragma in question. I hope it is okay to issue a cc in case they could shed any additional light on what might be going wrong with the clangdrawf build, without the (now removed) pragma? On Sat, 9 Dec 2023 at 15:24, Mike Beaton wrote: > > I have raised a bug, > https://bugzilla.tianocore.org/show_bug.cgi?id=4617 - adding a little > bit of new information which I believe is correct, that I've been able > to dig out. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112252): https://edk2.groups.io/g/devel/message/112252 Mute This Topic: https://groups.io/mt/103030855/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] CLANGDWARF OVMF hangs on start
I have raised a bug, https://bugzilla.tianocore.org/show_bug.cgi?id=4617 - adding a little bit of new information which I believe is correct, that I've been able to dig out. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112251): https://edk2.groups.io/g/devel/message/112251 Mute This Topic: https://groups.io/mt/103030855/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] CLANGDWARF OVMF hangs on start
> - I have just replicated exactly the same issue from a clean, new > checkout and build on Fedora 38 and clang 16 (Fedora 16.0.6-3.fc38). > - I then upgraded that same VM to Fedora 39 and clang 17 and ... I > still get the same issue, including that it works again when switching > to 140e4422b16482f0bcafdc20d42141434d450450~1 (then cleaning Conf/ and > Build/, then `make clean`, `make` in BaseTools/, then retry, as for > each re-test). I assume we must all be talking about clean builds at each point, more or less by definition. But for the avoidance of doubt the script I'm using to fully rebuild at each commit (which is reproducing these problems, on all versions listed above, and which matches what happens on a clean checkout - for me) is: ``` #!/bin/bash git submodule update --init rm -rf Build rm Conf/* rm -rf Conf/.cache rm Conf/.AutoGenIdFile.txt cd BaseTools make clean make cd ../OvmfPkg ./build.sh -a X64 -b NOOPT -t CLANGDWARF -D DEBUG_ON_SERIAL_PORT=1 || exit 1 ./build.sh -a X64 -b NOOPT -t CLANGDWARF qemu -serial stdio ``` -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112187): https://edk2.groups.io/g/devel/message/112187 Mute This Topic: https://groups.io/mt/103030855/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] CLANGDWARF OVMF hangs on start
> > Which clang version is this?
> >
> > It works for me, fedora 39 / clang 17, and I'm pretty sure I tested it
> > when creating the patch on a slightly older clang version (15 or 16).
>
> It is 14 ('Ubuntu clang version 14.0.0-1ubuntu1.1' on Ubuntu 22.04.3 LTS)
>
> Cheers,
> Mike
Strangely, given what you say:
- I have just replicated exactly the same issue from a clean, new
checkout and build on Fedora 38 and clang 16 (Fedora 16.0.6-3.fc38).
- I then upgraded that same VM to Fedora 39 and clang 17 and ... I
still get the same issue, including that it works again when switching
to 140e4422b16482f0bcafdc20d42141434d450450~1 (then cleaning Conf/ and
Build/, then `make clean`, `make` in BaseTools/, then retry, as for
each re-test).
Mike
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112186): https://edk2.groups.io/g/devel/message/112186
Mute This Topic: https://groups.io/mt/103030855/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] CLANGDWARF OVMF hangs on start
> Which clang version is this?
>
> It works for me, fedora 39 / clang 17, and I'm pretty sure I tested it
> when creating the patch on a slightly older clang version (15 or 16).
It is 14 ('Ubuntu clang version 14.0.0-1ubuntu1.1' on Ubuntu 22.04.3 LTS)
Cheers,
Mike
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112183): https://edk2.groups.io/g/devel/message/112183
Mute This Topic: https://groups.io/mt/103030855/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] CLANGDWARF OVMF hangs on start
>From commit
>https://github.com/tianocore/edk2/commit/140e4422b16482f0bcafdc20d42141434d450450
(and see also the preceding commit, which I believe is related) up to
the current tip of master, CLANGDWARF OVMF is broken. It builds and
starts but hangs early (more info below). I do not currently have a
patch to offer though of course it would be useful if this could be
resolved.
Note: I've cc'd those folks who have more or less recently (this year)
authored or reviewed commits which have modified CLANGDWARF OVMF build
behaviour in various interesting ways, including the above, as
described below.
### More detail:
Using the following commands
```
cd OvmfPkg
./build.sh -a X64 -b NOOPT -t CLANGDWARF -D DEBUG_ON_SERIAL_PORT=1
./build.sh -a X64 -b NOOPT -t CLANGDWARF qemu -serial stdio
```
from the mentioned commit up to the current tip of master CLANGDWARF
OVMF builds, but when started it hangs, repeatedly outputting the line
`SecCoreStartupWithStack(0xFFFCC000, 0x82)`.
Prior to that commit, CLANGDWARF OVMF builds and starts normally
(though see the following).
As additional possibly helpful information, it may be worth noting
that if we go back further we reach
https://github.com/tianocore/edk2/commit/c6f47e678f994ac86d36955d24baae465330d356
which is the earliest commit with correct behaviour. In the commit
before that CLANGDWARF OVMF builds but when run stops with an ASSERT,
and in the commit before that again, CLANGDWARF OVMF does not build,
giving:
```
In file included from
/home/mjsbeaton/OpenSource/edk2/CryptoPkg/Library/OpensslLib/openssl/crypto/asn1/a_sign.c:22:
...
In file included from /usr/include/stdint.h:34:
/usr/include/x86_64-linux-gnu/bits/stdint-intn.h:27:19: error: typedef
redefinition with different types ('__int64_t' (aka 'long') vs 'INT64'
(aka 'long long'))
typedef __int64_t int64_t;
^
/home/mjsbeaton/OpenSource/edk2/CryptoPkg/Library/OpensslLib/openssl/include/openssl/e_os2.h:238:15:
note: previous definition is here
typedef INT64 int64_t;
^
...
/usr/include/x86_64-linux-gnu/bits/stdint-uintn.h:27:20: error:
typedef redefinition with different types ('__uint64_t' (aka 'unsigned
long') vs 'UINT64' (aka 'unsigned long long'))
typedef __uint64_t uint64_t;
^
/home/mjsbeaton/OpenSource/edk2/CryptoPkg/Library/OpensslLib/openssl/include/openssl/e_os2.h:239:16:
note: previous definition is here
typedef UINT64 uint64_t;
^
...
2 errors generated.
```
That failure to build persists back in the commit history for some
time. I didn't find a commit further back than that where CLANGDWARF
OVMF builds and runs correctly with current tools, though the exact
build failure changes at different points in the commit history, and I
haven't bisected everything which happens. (Note: when doing the above
bisects, it seems helpful/necessary to rebuild base tools, plus of
course `git submodule update --init`, at each step to get reliable
results.)
Mike
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112168): https://edk2.groups.io/g/devel/message/112168
Mute This Topic: https://groups.io/mt/103030855/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5] MdeModulePkg/HiiDatabase: Fix incorrect AllocateCopyPool size
On Thu, 7 Sept 2023 at 04:35, Mike Beaton wrote: > > The immediately preceding call, GetBestLanguage, plus the implementation of > HiiGetString, which is called immediately afterwards, make it clear that > BestLanguage is a null-terminated ASCII string, and not just a five byte, > non-null terminated buffer. > > Therefore AsciiStrLen is one byte too short, meaning that whether the space > allocated is really sufficient and whether the resultant string is really > null-terminated becomes implementation-dependent. Rather than switching to > AsciiStrSize, we use an explicitly compile-time string length calculation > (both compile-time and run-time approaches are currently used elsewhere in > the codebase for copying static strings). Apologies for the multiple versions, but I thought it was important to clarify in the commit message above that this really was a fix, not a misunderstanding. I also realised, in this last version, that sizeof is preferable here to AsciiStrSize (and that sizeof is, in fact, already often used when copying static strings elsewhere in the codebase - both sizeof and (Ascii)StrSize are used in various places, but with more use of sizeof). -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108358): https://edk2.groups.io/g/devel/message/108358 Mute This Topic: https://groups.io/mt/101208544/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v5] MdeModulePkg/HiiDatabase: Fix incorrect AllocateCopyPool size
The immediately preceding call, GetBestLanguage, plus the implementation of
HiiGetString, which is called immediately afterwards, make it clear that
BestLanguage is a null-terminated ASCII string, and not just a five byte,
non-null terminated buffer.
Therefore AsciiStrLen is one byte too short, meaning that whether the space
allocated is really sufficient and whether the resultant string is really
null-terminated becomes implementation-dependent. Rather than switching to
AsciiStrSize, we use an explicitly compile-time string length calculation
(both compile-time and run-time approaches are currently used elsewhere in
the codebase for copying static strings).
Signed-off-by: Mike Beaton
---
MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
index 96e05d4cf9..6e791783a6 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
@@ -1987,7 +1987,7 @@ GetNameFromId (
NULL
);
if (BestLanguage == NULL) {
-BestLanguage = AllocateCopyPool (AsciiStrLen ("en-US"), "en-US");
+BestLanguage = AllocateCopyPool (sizeof ("en-US"), "en-US");
ASSERT (BestLanguage != NULL);
}
--
2.41.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108357): https://edk2.groups.io/g/devel/message/108357
Mute This Topic: https://groups.io/mt/101208544/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v4] MdeModulePkg/HiiDatabase: Fix incorrect AllocateCopyPool size
AsciiStrLen is one byte too short, thus whether the space allocated is really
sufficient and whether the resultant string is really null-terminated becomes
implementation-dependent. An explicitly compile-time string length calculation
might be even more ideal, but AsciiStrSize matches usage elsewhere in the
codebase.
Signed-off-by: Mike Beaton
---
MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
index 96e05d4cf9..f67b7760f0 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
@@ -1987,7 +1987,7 @@ GetNameFromId (
NULL
);
if (BestLanguage == NULL) {
-BestLanguage = AllocateCopyPool (AsciiStrLen ("en-US"), "en-US");
+BestLanguage = AllocateCopyPool (AsciiStrSize ("en-US"), "en-US");
ASSERT (BestLanguage != NULL);
}
--
2.41.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108345): https://edk2.groups.io/g/devel/message/108345
Mute This Topic: https://groups.io/mt/101204385/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3] MdeModulePkg/HiiDatabase: Fix incorrect AllocateCopyPool size
On Wed, 6 Sept 2023 at 12:55, Mike Beaton wrote: > > On Wed, 6 Sept 2023 at 11:34, Mike Beaton wrote: > > > > AsciiStrLen was one byte too short (though with alignment up from an odd > > size > > would probably always have had the required space in practice). AsciiStrSize > > matches usage elsewhere in this file and in the codebase. Have just realised that the severity is worse than implied in my current commit message, since not only are (potentially - though almost certainly not, in practice) too few bytes allocated, but definitely too few bytes are then copied, so the resulting string is only null terminated by the grace of the specific implementation, too. Could update to a v4 of this (small) patch with a commit message mentioning this? -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108329): https://edk2.groups.io/g/devel/message/108329 Mute This Topic: https://groups.io/mt/101189764/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3] MdeModulePkg/HiiDatabase: Fix incorrect AllocateCopyPool size
On Wed, 6 Sept 2023 at 11:34, Mike Beaton wrote: > > AsciiStrLen was one byte too short (though with alignment up from an odd size > would probably always have had the required space in practice). AsciiStrSize > matches usage elsewhere in this file and in the codebase. I was intended to cc Ard Biesheuvel as well - I hope that is okay, since you have definitely committed on this file (as no doubt many others!) - but I managed to use your now-bouncing old email address from one of those earlier commits, so I have fixed that in this reply, and also removed from the cc list the addresses of a couple of Intel employees who originally authored or worked on this file but which now bounce. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108328): https://edk2.groups.io/g/devel/message/108328 Mute This Topic: https://groups.io/mt/101189764/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v3] MdeModulePkg/HiiDatabase: Fix incorrect AllocateCopyPool size
AsciiStrLen was one byte too short (though with alignment up from an odd size
would probably always have had the required space in practice). AsciiStrSize
matches usage elsewhere in this file and in the codebase.
Signed-off-by: Mike Beaton
---
MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
index 96e05d4cf9..f67b7760f0 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
@@ -1987,7 +1987,7 @@ GetNameFromId (
NULL
);
if (BestLanguage == NULL) {
-BestLanguage = AllocateCopyPool (AsciiStrLen ("en-US"), "en-US");
+BestLanguage = AllocateCopyPool (AsciiStrSize ("en-US"), "en-US");
ASSERT (BestLanguage != NULL);
}
--
2.41.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108322): https://edk2.groups.io/g/devel/message/108322
Mute This Topic: https://groups.io/mt/101189764/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v3] Revert "OvmfPkg: Update build.sh to allow building OVMF then running QEMU"
This reverts commit 173a7a7daaad560cd69e1000faca1d2b91774c46 Fixes https://bugzilla.tianocore.org/show_bug.cgi?id=4528 The build.sh qemu option starts the correct qemu executable for the selected architecture (build.sh -a option, or implicit) and uses the correct previously built OVMF image for the selected architecture and build target (build.sh -b option, or implicit). With this revert, the above step will fail if there is no matching previously built OVMF image. This is advantageous over rebuilding each time the build.sh qemu option is used (as in the reverted commit), because it provides a quick way to run a just-built OVMF image in place, while: a) Starting immediately (saving the time required for a rebuild on each usage, if the VM is started multiple times) b) Preserving the NVRAM contents between multiple runs (i.e. until the image is next rebuilt) Signed-off-by: Mike Beaton --- OvmfPkg/build.sh | 15 +++ 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh index b0334fb76e..279f0d099a 100755 --- a/OvmfPkg/build.sh +++ b/OvmfPkg/build.sh @@ -246,11 +246,11 @@ else fi # -# Build the edk2 OvmfPkg +# Run previously built OVMF image for current build options, in place. +# Do not rebuild first, rather allow multiple runs of a previously built +# image to start quickly (without rebuild), and with preserved NVRAM contents +# between runs (until the next rebuild). # -echo Running edk2 build for OvmfPkg$Processor -build -p $PLATFORMFILE $BUILD_OPTIONS -b $BUILDTARGET -t $TARGET_TOOLS -n $THREADNUMBER -DDEBUG_ON_SERIAL_PORT=TRUE - if [[ "$RUN_QEMU" == "yes" ]]; then if [[ ! -d $QEMU_FIRMWARE_DIR ]]; then mkdir $QEMU_FIRMWARE_DIR @@ -265,3 +265,10 @@ if [[ "$RUN_QEMU" == "yes" ]]; then $QEMU_COMMAND "$@" exit $? fi + +# +# Build the edk2 OvmfPkg +# +echo Running edk2 build for OvmfPkg$Processor +build -p $PLATFORMFILE $BUILD_OPTIONS -b $BUILDTARGET -t $TARGET_TOOLS -n $THREADNUMBER +exit $? -- 2.40.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108320): https://edk2.groups.io/g/devel/message/108320 Mute This Topic: https://groups.io/mt/101189234/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Update build.sh to allow building OVMF then running QEMU
On Mon, 4 Sept 2023, 21:14 Rebecca Cran, wrote: > Sorry I've not responded to your emails before now. > > I don't normally use the build.sh script, so I'm fine with reverting to > the previous behavior. > Ah - thank you for the response - that'd be great, if the original people who ack-d and committed are fine to approve the change back too. I did put up a patch to this list (my v2 patch reverts the change, and at the same time removes an unwanted extra white space line which was actually there at the end of the original file) - but I am tbh not as familiar as I should be with text-based patches, I'll double check the process and check that it applies using (whatever I find out) the normal commands (are...!). Mike > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108275): https://edk2.groups.io/g/devel/message/108275 Mute This Topic: https://groups.io/mt/96836052/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Update build.sh to allow building OVMF then running QEMU
> -Original Message- > From: Rebecca Cran > Sent: Wednesday, February 8, 2023 6:02 PM > To: devel@edk2.groups.io > Cc: Rebecca Cran ; Ard Biesheuvel > ; Jiewen Yao ; Jordan Justen > ; Gerd Hoffmann > Subject: [PATCH 1/1] OvmfPkg: Update build.sh to allow building OVMF then > running QEMU > > Allow users to build OVMF then run QEMU by moving the build block above > the run block and removing the exit line. > > Signed-off-by: Rebecca Cran > Cc: Ard Biesheuvel > Cc: Jiewen Yao > Cc: Jordan Justen > Cc: Gerd Hoffmann > --- > OvmfPkg/build.sh | 13 + > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh > index 91b1442ade6b..b0334fb76e74 100755 > --- a/OvmfPkg/build.sh > +++ b/OvmfPkg/build.sh > @@ -245,6 +245,11 @@ else >echo using prebuilt tools > fi > > +# > +# Build the edk2 OvmfPkg > +# > +echo Running edk2 build for OvmfPkg$Processor > +build -p $PLATFORMFILE $BUILD_OPTIONS -b $BUILDTARGET -t $TARGET_TOOLS -n > $THREADNUMBER -DDEBUG_ON_SERIAL_PORT=TRUE > > if [[ "$RUN_QEMU" == "yes" ]]; then >if [[ ! -d $QEMU_FIRMWARE_DIR ]]; then > @@ -260,11 +265,3 @@ if [[ "$RUN_QEMU" == "yes" ]]; then >$QEMU_COMMAND "$@" >exit $? > fi > - > -# > -# Build the edk2 OvmfPkg > -# > -echo Running edk2 build for OvmfPkg$Processor > -build -p $PLATFORMFILE $BUILD_OPTIONS -b $BUILDTARGET -t $TARGET_TOOLS -n > $THREADNUMBER > -exit $? > - > -- > 2.30.2 Hi, I was trying to raise the issue that I believe this change from Feb. of this year breaks intended and useful previous behaviour. It occurred to me that replying to the original post might make most sense. I've raised what I think are the issues here: https://bugzilla.tianocore.org/show_bug.cgi?id=4528 . WIth many thanks in advance for your attention. Mike Beaton -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108260): https://edk2.groups.io/g/devel/message/108260 Mute This Topic: https://groups.io/mt/96836052/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] Revert "OvmfPkg: Update build.sh to allow building OVMF then running QEMU"
PS Extra whitespace line was in original file, i.e. revert here is technically correct, but assume v2 revert is preferable. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108049): https://edk2.groups.io/g/devel/message/108049 Mute This Topic: https://groups.io/mt/100930099/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] MdeModulePkg/HiiDatabase: Fix incorrect AllocateCopyPool size
Apologies, no Signed-off-by, I will send v2.
On Thu, 24 Aug 2023 at 06:52, Mike Beaton wrote:
>
> ---
> MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
> b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
> index 96e05d4cf9..f67b7760f0 100644
> --- a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
> +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
> @@ -1987,7 +1987,7 @@ GetNameFromId (
> NULL
> );
> if (BestLanguage == NULL) {
> - BestLanguage = AllocateCopyPool (AsciiStrLen ("en-US"), "en-US");
> + BestLanguage = AllocateCopyPool (AsciiStrSize ("en-US"), "en-US");
> ASSERT (BestLanguage != NULL);
> }
> --
> 2.37.5
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107999): https://edk2.groups.io/g/devel/message/107999
Mute This Topic: https://groups.io/mt/100930530/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v2] MdeModulePkg/HiiDatabase: Fix incorrect AllocateCopyPool size
Signed-off-by: Mike Beaton
---
MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
index 96e05d4cf9..f67b7760f0 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
@@ -1987,7 +1987,7 @@ GetNameFromId (
NULL
);
if (BestLanguage == NULL) {
- BestLanguage = AllocateCopyPool (AsciiStrLen ("en-US"), "en-US");
+ BestLanguage = AllocateCopyPool (AsciiStrSize ("en-US"), "en-US");
ASSERT (BestLanguage != NULL);
}
--
2.37.5
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107998): https://edk2.groups.io/g/devel/message/107998
Mute This Topic: https://groups.io/mt/100930546/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH] MdeModulePkg/HiiDatabase: Fix incorrect AllocateCopyPool size
---
MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
index 96e05d4cf9..f67b7760f0 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c
@@ -1987,7 +1987,7 @@ GetNameFromId (
NULL
);
if (BestLanguage == NULL) {
- BestLanguage = AllocateCopyPool (AsciiStrLen ("en-US"), "en-US");
+ BestLanguage = AllocateCopyPool (AsciiStrSize ("en-US"), "en-US");
ASSERT (BestLanguage != NULL);
}
--
2.37.5
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107997): https://edk2.groups.io/g/devel/message/107997
Mute This Topic: https://groups.io/mt/100930530/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] PATCH v2] Revert "OvmfPkg: Update build.sh to allow building OVMF then running QEMU"
This reverts commit 173a7a7daaad560cd69e1000faca1d2b91774c46. Fixes https://bugzilla.tianocore.org/show_bug.cgi?id=4528 Signed-off-by: Mike Beaton --- OvmfPkg/build.sh | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh index b0334fb76e..be2cbce0fd 100755 --- a/OvmfPkg/build.sh +++ b/OvmfPkg/build.sh @@ -245,11 +245,6 @@ else echo using prebuilt tools fi -# -# Build the edk2 OvmfPkg -# -echo Running edk2 build for OvmfPkg$Processor -build -p $PLATFORMFILE $BUILD_OPTIONS -b $BUILDTARGET -t $TARGET_TOOLS -n $THREADNUMBER -DDEBUG_ON_SERIAL_PORT=TRUE if [[ "$RUN_QEMU" == "yes" ]]; then if [[ ! -d $QEMU_FIRMWARE_DIR ]]; then @@ -265,3 +260,10 @@ if [[ "$RUN_QEMU" == "yes" ]]; then $QEMU_COMMAND "$@" exit $? fi + +# +# Build the edk2 OvmfPkg +# +echo Running edk2 build for OvmfPkg$Processor +build -p $PLATFORMFILE $BUILD_OPTIONS -b $BUILDTARGET -t $TARGET_TOOLS -n $THREADNUMBER +exit $? -- 2.37.5 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107995): https://edk2.groups.io/g/devel/message/107995 Mute This Topic: https://groups.io/mt/100930184/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] Revert "OvmfPkg: Update build.sh to allow building OVMF then running QEMU"
This includes an extraneous whitespace line at end of file, I will send a v2. On Thu, 24 Aug 2023 at 05:45, Mike Beaton wrote: > > This reverts commit 173a7a7daaad560cd69e1000faca1d2b91774c46. > > Fixes https://bugzilla.tianocore.org/show_bug.cgi?id=4528 > > Signed-off-by: Mike Beaton > --- > OvmfPkg/build.sh | 13 - > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh > index b0334fb76e..91b1442ade 100755 > --- a/OvmfPkg/build.sh > +++ b/OvmfPkg/build.sh > @@ -245,11 +245,6 @@ else > echo using prebuilt tools > fi > -# > -# Build the edk2 OvmfPkg > -# > -echo Running edk2 build for OvmfPkg$Processor > -build -p $PLATFORMFILE $BUILD_OPTIONS -b $BUILDTARGET -t > $TARGET_TOOLS -n $THREADNUMBER -DDEBUG_ON_SERIAL_PORT=TRUE > if [[ "$RUN_QEMU" == "yes" ]]; then > if [[ ! -d $QEMU_FIRMWARE_DIR ]]; then > @@ -265,3 +260,11 @@ if [[ "$RUN_QEMU" == "yes" ]]; then > $QEMU_COMMAND "$@" > exit $? > fi > + > +# > +# Build the edk2 OvmfPkg > +# > +echo Running edk2 build for OvmfPkg$Processor > +build -p $PLATFORMFILE $BUILD_OPTIONS -b $BUILDTARGET -t > $TARGET_TOOLS -n $THREADNUMBER > +exit $? > + > -- > 2.37.5 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107994): https://edk2.groups.io/g/devel/message/107994 Mute This Topic: https://groups.io/mt/100930099/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH] Revert "OvmfPkg: Update build.sh to allow building OVMF then running QEMU"
This reverts commit 173a7a7daaad560cd69e1000faca1d2b91774c46. Fixes https://bugzilla.tianocore.org/show_bug.cgi?id=4528 Signed-off-by: Mike Beaton --- OvmfPkg/build.sh | 13 - 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh index b0334fb76e..91b1442ade 100755 --- a/OvmfPkg/build.sh +++ b/OvmfPkg/build.sh @@ -245,11 +245,6 @@ else echo using prebuilt tools fi -# -# Build the edk2 OvmfPkg -# -echo Running edk2 build for OvmfPkg$Processor -build -p $PLATFORMFILE $BUILD_OPTIONS -b $BUILDTARGET -t $TARGET_TOOLS -n $THREADNUMBER -DDEBUG_ON_SERIAL_PORT=TRUE if [[ "$RUN_QEMU" == "yes" ]]; then if [[ ! -d $QEMU_FIRMWARE_DIR ]]; then @@ -265,3 +260,11 @@ if [[ "$RUN_QEMU" == "yes" ]]; then $QEMU_COMMAND "$@" exit $? fi + +# +# Build the edk2 OvmfPkg +# +echo Running edk2 build for OvmfPkg$Processor +build -p $PLATFORMFILE $BUILD_OPTIONS -b $BUILDTARGET -t $TARGET_TOOLS -n $THREADNUMBER +exit $? + -- 2.37.5 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107993): https://edk2.groups.io/g/devel/message/107993 Mute This Topic: https://groups.io/mt/100930099/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] OVMF build.sh change is incorrect
This is now in https://bugzilla.tianocore.org/show_bug.cgi?id=4528 as
it should have been in the first place!
On Sun, 13 Aug 2023 at 11:34, Mike Beaton wrote:
>
> Perhaps I can briefly clarify:
>
> "This now no longer works" was too brief - of course the listed command does
> build and start QEMU. But, to clarify, a rebuild is not what is wanted here,
> both for the additional time it takes, and for the fact that it resets
> (rebuilds) the NVRAM of the VM.
>
> The advantage of using `build.sh -a ... -b ... qemu ...` to launch QEMU
> without building (as was previously possible) is that it automatically
> selects the correct qemu command and correct built OVMF BIOS binary to match
> the related build command.
>
> Mike
>
> On Sun, 13 Aug 2023 at 11:13, Mike Beaton wrote:
>>
>> I believe this change
>> https://github.com/tianocore/edk2/commit/173a7a7daaad560cd69e1000faca1d2b91774c46
>> may have misunderstood the purpose of the previous code.
>>
>> I used to frequently use:
>>
>> `./build.sh -a X64 -b RELEASE` (or whichever arch and build target I
>> required) to build OVMF
>>
>> and then:
>>
>> `./build.sh -a X64 -b RELEASE qemu {my-qemu-flags}` to run OVMF
>>
>> This now no longer works, since the second command also attempts to rebuild
>> OVMF every time you run it.
>>
>> I believe the previous behaviour was intended and more useful.
>>
>> Mike Beaton
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107897): https://edk2.groups.io/g/devel/message/107897
Mute This Topic: https://groups.io/mt/100750154/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] OVMF build.sh change is incorrect
Perhaps I can briefly clarify:
"This now no longer works" was too brief - of course the listed command
does build and start QEMU. But, to clarify, a rebuild is not what is wanted
here, both for the additional time it takes, and for the fact that it
resets (rebuilds) the NVRAM of the VM.
The advantage of using `build.sh -a ... -b ... qemu ...` to launch QEMU
without building (as was previously possible) is that it automatically
selects the correct qemu command and correct built OVMF BIOS binary to
match the related build command.
Mike
On Sun, 13 Aug 2023 at 11:13, Mike Beaton wrote:
> I believe this change
> https://github.com/tianocore/edk2/commit/173a7a7daaad560cd69e1000faca1d2b91774c46
> may have misunderstood the purpose of the previous code.
>
> I used to frequently use:
>
> `./build.sh -a X64 -b RELEASE` (or whichever arch and build target I
> required) to build OVMF
>
> and then:
>
> `./build.sh -a X64 -b RELEASE qemu {my-qemu-flags}` to run OVMF
>
> This now no longer works, since the second command also attempts to
> rebuild OVMF every time you run it.
>
> I believe the previous behaviour was intended and more useful.
>
> Mike Beaton
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107750): https://edk2.groups.io/g/devel/message/107750
Mute This Topic: https://groups.io/mt/100750154/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] OVMF build.sh change is incorrect
I believe this change
https://github.com/tianocore/edk2/commit/173a7a7daaad560cd69e1000faca1d2b91774c46
may have misunderstood the purpose of the previous code.
I used to frequently use:
`./build.sh -a X64 -b RELEASE` (or whichever arch and build target I
required) to build OVMF
and then:
`./build.sh -a X64 -b RELEASE qemu {my-qemu-flags}` to run OVMF
This now no longer works, since the second command also attempts to rebuild
OVMF every time you run it.
I believe the previous behaviour was intended and more useful.
Mike Beaton
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107749): https://edk2.groups.io/g/devel/message/107749
Mute This Topic: https://groups.io/mt/100750154/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
