MdeModulePkg: Fix clang build regressions. #11803

apop5 · 2025-11-25T23:16:15Z

Description

Address the build regressions, introduced in #11724, #11688, #11686 #11685.

These build regressions are for uninitialized variables before use.

Breaking change?
- Breaking change - Does this PR cause a break in build or boot behavior?
- Examples: Does it add a new library class or move a module to a different repo.
Impacts security?
- Security - Does this PR have a direct security impact?
- Examples: Crypto algorithm change or buffer overflow fix.
Includes tests?
- Tests - Does this PR include any explicit test code?
- Examples: Unit tests or integration tests.

How This Was Tested

Local CI using clangdwarf as tool chain target.

Integration Instructions

No integration necessary.

MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c

leiflindholm · 2025-11-26T15:16:29Z

I mean, maybe we need to merge this in order to get the build back to working. But what started this was an attempt to (presumably) improve code quality, and now we're back to "make the compiler go quiet".

The underlying problem of horrendously badly structured code is still there, and this sweeps it under the rug.

khaliid2040 · 2025-11-26T15:45:38Z

wouldn't this PR conflict with #11788. They are resolving same issue.

I think i will close #11788. @apop5 can you please link your PR to this following issues. #11793
,#11787 and #11786 so it your PR closes them.

Address the build regressions, introduced in tianocore#11724, tianocore#11688, tianocore#11686 tianocore#11685. These build regressions are for uninitialized variables before use. Signed-off-by: Aaron Pop <[email protected]>

apop5 · 2025-11-26T19:59:34Z

wouldn't this PR conflict with #11788. They are resolving same issue.

I think i will close #11788. @apop5 can you please link your PR to this following issues. #11793 ,#11787 and #11786 so it your PR closes them.

There are changing the same thing, but some additional files that were not covered in your original PR also needed some changes.

khaliid2040 · 2025-11-27T07:24:10Z

wouldn't this PR conflict with #11788. They are resolving same issue.
I think i will close #11788. @apop5 can you please link your PR to this following issues. #11793 ,#11787 and #11786 so it your PR closes them.

There are changing the same thing, but some additional files that were not covered in your original PR also needed some changes.

Should i close mine then, basically if yours resolving it.

pierregondois · 2025-11-27T09:01:21Z

MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c

            // ignore the selection and go back to reading keys.
            //
-            if (MenuOption != NULL ) {
+            if (MenuOption == NULL ) {


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 ?

That fact that only one of these has been fixed, when the others occur in the same PR, is actually careless IMO. I'm certainly not flawless - we all make mistakes - but I try hard not to be careless.

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!

11686, 11687, 11688, 11689 included some inverted conditionals during the refactor. While the system booted, some behavior was incorrect based on the inverted conditionals. Signed-off-by: Aaron Pop <[email protected]>

mikebeaton · 2025-11-28T23:41:56Z

Is it possible to request that this PR be split into two, the first commit staying in this PR and matching the existing PR title, the second commit being split off into its own PR.

The two commits address rather different kinds of issues, and I think splitting would make it easier to review, and easier to see (now and in the future) what is going on/went on with these changes and fixes.

mikebeaton · 2025-11-29T05:58:43Z

MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c


  AppendString  = NULL;
  NvConfigExist = NULL;
+  Status        = EFI_OUT_OF_RESOURCES;


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

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.

mikebeaton · 2025-11-29T06:02:38Z

MdeModulePkg/Universal/SetupBrowserDxe/IfrParse.c

  NAME_VALUE_NODE          *NameValueNode;
  BOOLEAN                  Find;

+  Storage = NULL;


This one is not used uninitialized.

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

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.

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.)

mikebeaton · 2025-11-29T06:14:30Z

MdeModulePkg/Universal/SetupBrowserDxe/IfrParse.c

  LIST_ENTRY       *Link;
  BROWSER_STORAGE  *BrowserStorage;

+  BrowserStorage = NULL;


This one is not used uninitialized.

mikebeaton · 2025-11-29T06:25:49Z

As a minor but valid point, commit message subject lines should not end with period.

More substantively, for myself I would prefer commit messages which show what is actually being dealt with, rather than vaguer wording about regressions. So for the first commit, I would prefer: MdeModulePkg: Fix uninitialized variable usage, and for the second I would prefer MdeModulePkg: Fix code flow errors introduced in #11687. Please also add omitted # before the PR numbers in the body of the second commit message.

Additionally, in the commit message of the second commit, please add: Fixes: #11814.

As mentioned above, I think it would definitely help to split the two commits into two PRs, please.

tianocore-assign-reviewers bot requested review from dandanbi, jiaxinwu, lgao4 and niruiyu November 25, 2025 23:16

leiflindholm requested changes Nov 26, 2025

View reviewed changes

MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c Show resolved Hide resolved

MdeModulePkg: Fix clang build regressions.

d0adb89

Address the build regressions, introduced in tianocore#11724, tianocore#11688, tianocore#11686 tianocore#11685. These build regressions are for uninitialized variables before use. Signed-off-by: Aaron Pop <[email protected]>

apop5 force-pushed the personal/apop5/codeqlclangfixes branch from 8f0b651 to dbc3503 Compare November 26, 2025 19:58

apop5 mentioned this pull request Nov 26, 2025

MdeModulePkg: DisplayEngineDxe Static analysis changes. #11687

Merged

3 tasks

pierregondois reviewed Nov 27, 2025

View reviewed changes

khaliid2040 mentioned this pull request Nov 27, 2025

MdeModulePkg/Universal: Fix several build issues for clang #11788

Closed

3 tasks

MdeModulePkg: DisplayEngineDxe fix regression from 11687.

3d0fad5

11686, 11687, 11688, 11689 included some inverted conditionals during the refactor. While the system booted, some behavior was incorrect based on the inverted conditionals. Signed-off-by: Aaron Pop <[email protected]>

apop5 force-pushed the personal/apop5/codeqlclangfixes branch from dbc3503 to 3d0fad5 Compare November 28, 2025 18:33

mikebeaton reviewed Nov 29, 2025

View reviewed changes

mikebeaton mentioned this pull request Nov 29, 2025

[Bug]: 901c4e3 breaks response to Enter key in Arm platform firmware menus #11814

Open

4 tasks

MdeModulePkg: Fix clang build regressions. #11803

Are you sure you want to change the base?

MdeModulePkg: Fix clang build regressions. #11803

Conversation

apop5 commented Nov 25, 2025

Description

How This Was Tested

Integration Instructions

Uh oh!

Uh oh!

leiflindholm commented Nov 26, 2025

Uh oh!

khaliid2040 commented Nov 26, 2025

Uh oh!

apop5 commented Nov 26, 2025

Uh oh!

khaliid2040 commented Nov 27, 2025

Uh oh!

pierregondois Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

mikebeaton Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

apop5 Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

mikebeaton commented Nov 28, 2025

Uh oh!

mikebeaton Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khaliid2040 Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

mikebeaton Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

khaliid2040 Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

mikebeaton Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebeaton Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebeaton Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

mikebeaton commented Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mikebeaton Nov 29, 2025 •

edited

Loading

mikebeaton Nov 29, 2025 •

edited

Loading

mikebeaton Nov 29, 2025 •

edited

Loading

mikebeaton commented Nov 29, 2025 •

edited

Loading