Skip to content

MdeModulePkg: Fix clang build regressions. #11803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+28 −15
Open

MdeModulePkg: Fix clang build regressions. #11803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0adb89
MdeModulePkg: Fix clang build regressions.
apop5 Nov 25, 2025
3d0fad5
MdeModulePkg: DisplayEngineDxe fix regression from 11687.
apop5 Nov 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1607,11 +1607,15 @@ GetFullSmramRanges (
UINTN MaxCount;
BOOLEAN Rescan;

SmmConfiguration = NULL;
TempSmramRanges = NULL;
SmramReservedRanges = NULL;
SmramRanges = NULL;

//
// Get SMM Configuration Protocol if it is present.
//
SmmConfiguration = NULL;
Status = gBS->LocateProtocol (&gEfiSmmConfigurationProtocolGuid, NULL, (VOID **)&SmmConfiguration);
Status = gBS->LocateProtocol (&gEfiSmmConfigurationProtocolGuid, NULL, (VOID **)&SmmConfiguration);

//
// Get SMRAM information.
Expand Down
1 change: 1 addition & 0 deletions MdeModulePkg/Universal/BdsDxe/BdsEntry.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,7 @@ BdsEntry (

HotkeyTriggered = NULL;
Status = EFI_SUCCESS;
BootSuccess = FALSE;

//
// Insert the performance probe
Expand Down
8 changes: 4 additions & 4 deletions MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3269,7 +3269,7 @@ UiDisplayMenu (
// If the screen has no menu items, and the user didn't select UiReset
// ignore the selection and go back to reading keys.
//
if (MenuOption != NULL ) {
if (MenuOption == NULL ) {
Copy link
Contributor

@pierregondois pierregondois Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are other occurrences of this at https://github.com/tianocore/edk2/pull/11687/files
I count 3 in total. Is it possible to check ?

Copy link
Contributor Author

@apop5 apop5 Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are other occurrences of this at https://github.com/tianocore/edk2/pull/11687/files I count 3 in total. Is it possible to check ?

I went back through the 4 PRs addressing static analysis problems and found some additional problems. Thanks for reporting them @pierregondois

To reverify functionality, I booted Ovmf ia32,x64 and went into setup and checked the functionality of the menus, to the best of my ability, and did not see any additional problems. Thanks!

ASSERT (MenuOption != NULL);
break;
}
Expand Down Expand Up @@ -3324,7 +3324,7 @@ UiDisplayMenu (
break;
}

if (MenuOption != NULL) {
if (MenuOption == NULL) {
ASSERT (MenuOption != NULL);
break;
}
Expand Down Expand Up @@ -3413,7 +3413,7 @@ UiDisplayMenu (

case CfUiSelect:
ControlFlag = CfRepaint;
if (MenuOption != NULL ) {
if (MenuOption == NULL ) {
ASSERT (MenuOption != NULL);
break;
}
Expand Down Expand Up @@ -3472,7 +3472,7 @@ UiDisplayMenu (

case CfUiHotKey:
ControlFlag = CfRepaint;
if (HotKey != NULL ) {
if (HotKey == NULL ) {
ASSERT (HotKey != NULL);
break;
}
Expand Down
2 changes: 1 addition & 1 deletion MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1717,7 +1717,7 @@ GetSelectionInputPopUp (
gUserInput->InputValue.BufferLen = Question->CurrentValue.BufferLen;
}
} else {
if (CurrentOption != NULL) {
if (CurrentOption == NULL) {
ASSERT (CurrentOption != NULL);
break;
}
Expand Down
2 changes: 1 addition & 1 deletion MdeModulePkg/Universal/EbcDxe/EbcDebugger/EdbSymbol.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1268,7 +1268,7 @@ EdbLoadCodBySymbolByIec (
// get function name, function name is followed by char 0x09.
//
FieldBuffer = AsciiStrGetNewTokenField (LineBuffer, Char);
if (FieldBuffer != NULL) {
if (FieldBuffer == NULL) {
break;
}

Expand Down
1 change: 1 addition & 0 deletions MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,7 @@ CompareNameElementDefault (

AppendString = NULL;
NvConfigExist = NULL;
Status = EFI_OUT_OF_RESOURCES;
Copy link
Member

@mikebeaton mikebeaton Nov 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't EFI_ABORTED a better choice here? (The failures this is for are not out of resources errors.)

Copy link

@khaliid2040 khaliid2040 Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hii is a protocol documented in UEFI spec( chapter 34). The spec didn't document to return EFI_ABORTED so this will break protocol consumers and spec complaince.

Copy link
Member

@mikebeaton mikebeaton Nov 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right that any changes must match the UEFI Spec. Thanks for raising this issue.

However I believe this return status is - eventually - not returned by anything up at the level affect by the spec.

Actually, the following methods should be marked STATIC:

  • CompareNameElementDefault
  • CompareAndMergeDefaultString
  • MergeDefaultString

EFI_ABORTED is fine as a return status from CompareNameElementDefault (it is less misleading, and not propagated to anything up at the spec level), but indeed the Doxygen comment for this internal method should be updated as well, if it is used.

//
// Make NvConfigPtr point to the first <NvConfig> with AltConfigHdr in DefaultAltCfgResp.
//
Expand Down
2 changes: 1 addition & 1 deletion MdeModulePkg/Universal/PlatformDriOverrideDxe/PlatDriOverrideLib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ UpdateFvFileDevicePath (
// Build the shell device path
//
NewDevicePath = DevicePathFromHandle (FoundFvHandle);
if (NewDevicePath != NULL) {
if (NewDevicePath == NULL) {
return EFI_NOT_FOUND;
}

Expand Down
13 changes: 10 additions & 3 deletions MdeModulePkg/Universal/SetupBrowserDxe/IfrParse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ CreateQuestion (
NAME_VALUE_NODE *NameValueNode;
BOOLEAN Find;

Storage = NULL;
Copy link
Member

@mikebeaton mikebeaton Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is not used uninitialized.

Copy link

@khaliid2040 khaliid2040 Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLANG tend to be more stricter than GCC. I was the first one who reported the issue, until i let @apop5 to take over. here the three issues the PR mainly resolves : #11786, #11787 and #11793

Copy link
Member

@mikebeaton mikebeaton Nov 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are wrong here, about this particular line, @khaliid2040: it is not used uninitialized, clang does not complain about it, and you correctly did not change this line in your PR.

Copy link
Member

@mikebeaton mikebeaton Nov 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLANG tends to be stricter than GCC

I am well aware of the issue, I am attempting to work on integrating CLANG builds into CI, so that these issues do not keep recurring, and I had already fixed this set of issues myself, too. (And had only not already created a PR myself because I initially incorrectly assumed they were old bugs - and being worried about submitting these piecemeal, as I have been.)

Copy link

@khaliid2040 khaliid2040 Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are wrong here, about this particular line, @khaliid2040: it is not used uninitialized, clang does not complain about it, and you correctly did not change this line in your PR.

You are right. There were alot of issues in my original PR.

The reason CLANG complaining about the two variables is because of goto statement which is used early before the two variables are initialized so if goto executed then FreePool is handed to garbage address.

And still build fails with the log i provided.

Are you sure you are using latest CLANG version. Maybe the issue is specific to clang versions.

I am new here and still learning project workflow standards, but i hope i answered your question.

Copy link
Member

@mikebeaton mikebeaton Nov 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you're saying that you simply agree with my comment here (and then mentioning other related - but not directly related - issues). Please let me know if not!

It it still my belief that these two specific variables that I have marked in these two new comments are not used uninitialized (if you look at the code), and are not warned about by any version of clang (as they should not be; I am using LLVM 21) - so the changes in these two places should not be made.

(I think you agree, but it's less clear because you added the other comments.)

Copy link

@khaliid2040 khaliid2040 Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, i disagree with you and the changes should happen. The variable Storage and BrowserStorage are used uninitialized at the beginning of the function. That was my point, and i told you the reason.

Please correct me if i am wrong. Thanks

Copy link
Member

@mikebeaton mikebeaton Nov 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I really wasn't clear whether you disagreed (as your longer comment started with "You are right"!).

Please can you link to the line where this particular instance of Storage (not the other one, in another function, which I agree about) is used uninitialized, please?

Copy link

@khaliid2040 khaliid2040 Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologies, you were right. I overlooked your question. I didn't look at other changes he made

I was wrong, and i agree with you now. Again sorry for that. You were right at both your points.


Statement = CreateStatement (OpCodeData, FormSet, Form);
if (Statement == NULL) {
return NULL;
Expand Down Expand Up @@ -330,6 +332,8 @@ FindStorageInList (
LIST_ENTRY *Link;
BROWSER_STORAGE *BrowserStorage;

BrowserStorage = NULL;
Copy link
Member

@mikebeaton mikebeaton Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is not used uninitialized.


Link = GetFirstNode (&gBrowserStorageList);
while (!IsNull (&gBrowserStorageList, Link)) {
BrowserStorage = BROWSER_STORAGE_FROM_LINK (Link);
Expand Down Expand Up @@ -430,8 +434,11 @@ CreateStorage (
EFI_GUID *StorageGuid;
CHAR8 *StorageName;

UnicodeString = NULL;
StorageName = NULL;
UnicodeString = NULL;
StorageName = NULL;
BrowserStorage = NULL;
Storage = NULL;

switch (StorageType) {
case EFI_HII_VARSTORE_BUFFER:
StorageGuid = (EFI_GUID *)(CHAR8 *)&((EFI_IFR_VARSTORE *)OpCodeData)->Guid;
Expand Down Expand Up @@ -2451,7 +2458,7 @@ ParseOpCodes (
//
// Insert to Option list of current Question
//
if (ParentStatement != NULL) {
if (ParentStatement == NULL) {
break;
}

Expand Down
6 changes: 3 additions & 3 deletions MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ LoadAllHiiFormset (
// Initilize FormSet Setting
//
LocalFormSet = AllocateZeroPool (sizeof (FORM_BROWSER_FORMSET));
if (LocalFormSet != NULL ) {
if (LocalFormSet == NULL ) {
ASSERT (LocalFormSet != NULL);
return;
}
Expand Down Expand Up @@ -3291,7 +3291,7 @@ ConfirmNoSubmitFail (
}

StringBuffer = AllocateZeroPool (256 * sizeof (CHAR16));
if (StringBuffer != NULL) {
if (StringBuffer == NULL) {
ASSERT (StringBuffer != NULL);
return RetVal;
}
Expand Down Expand Up @@ -4423,7 +4423,7 @@ GetQuestionDefault (
if (!EFI_ERROR (Status)) {
if (HiiValue->Type == EFI_IFR_TYPE_STRING) {
NewString = GetToken (Question->HiiValue.Value.string, FormSet->HiiHandle);
if (NewString != NULL) {
if (NewString == NULL) {
ASSERT (NewString != NULL);
return EFI_NOT_FOUND;
}
Expand Down
Loading