Fast forward UEFI before pulling latest 202502 submodules#49
Merged
mebersol merged 50 commits intomicrosoft:mainfrom Mar 27, 2026
Merged
Fast forward UEFI before pulling latest 202502 submodules#49mebersol merged 50 commits intomicrosoft:mainfrom
mebersol merged 50 commits intomicrosoft:mainfrom
Conversation
`_MSC_VER` may or may not be defined by Clang, depending on how compatible it is trying to be and how compatible it is. So check `__clang__` first. ---- #### AI description (iteration 1) #### PR Classification This pull request is a bug fix aimed at improving compiler compatibility by reversing _MSC_VER checks for alignment attributes. #### PR Summary The change updates the macro in `/MsvmPkg/Include/DeclspecAlign.h` to ensure that when compiling with Clang or GNUC (or when _MSC_VER is not defined), the `__attribute__((aligned(n)))` is used instead of the MSVC-specific `__declspec(align(n))`. - `/MsvmPkg/Include/DeclspecAlign.h`: Reversed the condition to correctly select the alignment attribute based on the compiler. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #54710354, #57987809
Related work items: #57987809, #60275790
Change memcpy to CopyMem, so it links. ---- #### AI description (iteration 1) #### PR Classification This pull request performs a code cleanup to improve clang compatibility. #### PR Summary The changes update a unit test in the LegacyMtrrLib by replacing a call to memcpy with CopyMem, aligning with clang-specific requirements for building hyperv.uefi on AARCH64. - `MsvmPkg/Library/LegacyMtrrLib/UnitTest/Support.c`: Replaced `memcpy` with `CopyMem` in the GetEffectiveMemoryRanges function to support clang builds. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #57987809, #60275790
…which does not link. Clang: Assigning structs sometimes calls memcpy, which does not link. Call CopyMem inside ASSIGN_STRUCT. ``` (mu1) Q:\src\hyperv.uefi>"C:\Program Files\LLVM\bin\lld-link" /OUT:Q:\src\hyperv.uefi\Build\MsvmAARCH64\DEBUG_CLANGPDB\AARCH64\MsvmPkg\AziHsmDxe\AziHsmDxe\DEBUG\AziHsmDxe.dll /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT:ICF=10 /ALIGN:32 /Machine:ARM64 /DLL /ENTRY:_ModuleEntryPoint /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /SAFESEH:NO /BASE:0 /DEBUG:GHASH /MLLVM:-exception-model=wineh /lldmap @q:\src\hyperv.uefi\Build\MsvmAARCH64\DEBUG_CLANGPDB\AARCH64\MsvmPkg\AziHsmDxe\AziHsmDxe\OUTPUT\static_library_files.lst lld-link: warning: /align specified without /driver; image may not run lld-link: error: undefined symbol: memcpy >>> referenced by Q:\src\hyperv.uefi\MsvmPkg\AziHsmDxe\AziHsmDxe.c:330 >>> Q:\src\hyperv.uefi\Build\MsvmAARCH64\DEBUG_CLANGPDB\AARCH64\MsvmPkg\AziHsmDxe\AziHsmDxe\DEBUG\AziHsmDxe.dll.lto.obj:(AziHsmDriverBindingStart) >>> referenced by Q:\src\hyperv.uefi\MsvmPkg\AziHsmDxe\AziHsmCp.c:127 >>> Q:\src\hyperv.uefi\Build\MsvmAARCH64\DEBUG_CLANGPDB\AARCH64\MsvmPkg\AziHsmDxe\AziHsmDxe\DEBUG\AziHsmDxe.dll.lto.obj:(AziHsmFireHsmCmd) >>> referenced by Q:\src\hyperv.uefi\MsvmPkg\AziHsmDxe\AziHsmAdmin.c:88 >>> Q:\src\hyperv.uefi\Build\MsvmAARCH64\DEBUG_CLANGPDB\AARCH64\MsvmPkg\AziHsmDxe\AziHsmDxe\DEBUG\AziHsmDxe.dll.lto.obj:(AziHsmAdminIssueCmd) ``` Related work items: #57987809, #60275790
The "pdb" that /Zi makes is NOT what debuggers use. It is to pool types among compilations, since they tend to be duplicated, to save the linker the later work. /Zi does produce smaller objs, less I/O, and less work for the linker, however, there is no impact on the resulting dll or pdb (output by the linker), and the result is more deterministic, and this helps cut out the dreaded mspdbsrv.exe. This is what SQL does, for build reliability in a much larger, repository, and what Windows either mostly does or entirely does. Windows was so keen on using /Z7, despite a splattered partial legacy of /Zi around the tree, that a special flag /forceZ7 got added to silently override /Z7. People are often confused so I'll state and restate: - This has nothing to do with pdbs people use all the time for debugging. This is a temporary file produced by the compiler and consumed by the linker. - While /Z7 means version 7 as in very old (we are on version 19 now, in the relevant version sequence, see the compiler banner and _MSC_VER), it is not bad old, it is just different old. Actually even this version 19 is probably not accurate, but is frozen in time a few or 10 years ago. (VS2022 is 19.3-19.4, VS2025 is 19.5, VS2019 is up to 19.29..so I am guessing we are really on major version 24+, if it didn't have to freeze.) ---- #### AI description (iteration 1) #### PR Classification This PR is a code cleanup that updates the debugging compiler flag for consistency with the clang cleanup work item. #### PR Summary The PR replaces the `/Zi` flag with `/Z7` in configuration files for both x64 and AARCH64 builds, ensuring consistent debugging information generation. - Updated compiler flag in `MsvmPkg/MsvmPkgX64.dsc` - Updated compiler flag in `MsvmPkg/MsvmPkgAARCH64.dsc` <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #60275790
We can define forceinline for compatibility/portability, most of the time. We should favor "always", not "force", for inclusivity. Also to match gcc/clang terminology. We should, later, move all code out of headers, and __forceinline will go away. There is not much, really. There will only be extern ALWAYS_INLINE. Plain inline, and static inline, are really, mostly, historical accidents we can avoid now. Now that we have LTCG and extern ALWAYS_INLINE. We don't need code in headers, to make it inlinable. However code in headers has the advantage of not needing to be built for every client combination. Look at what WIL does, where it changes its namespace for every define it is sensitive to. Related work items: #54710354, #57987809
clang: Change ReadNoFence to just volatile. volatile_accessors presents a slight porting problem and duplicated code. This fixes part of it, by changing NoFence to just be volatile. No casts are required. It is how NoFence works, except on ARM64EC with default compiler flags, or ARM64 with non-default compiler flags, where volatile might be slower, but still correct. UEFI will never target ARM64EC, and again, it works either way, just possibly faster or slower. This was Oliver's recommendation. Cleanup and repair of ReadAcquire/WriteRelease, which will be further dealt with, in a later PR. ---- This pull request is a code cleanup aimed at improving clang compatibility by replacing custom non-fence read/write functions with direct volatile accesses. The PR removes redundant helper functions (ReadNoFence, WriteNoFence, etc.) in favor of direct volatile accesses and marks several structure fields as volatile to ensure correct memory semantics under clang. It also adds documentation for clang-specific build instructions. - **`MsvmPkg/Include/Private/EfiNt.h` and `MsvmPkg/Include/Synchronization.h`**: Removed non-fence inline functions and simplified read/write operations. - **`MsvmPkg/Include/Vmbus/VmbusPacketFormat.h` and `VmbusPacketInterface.h`**: Updated struct field definitions by adding volatile qualifiers. - **`MsvmPkg/EmclDxe/RingBuffer.c` and `Emcl.c`**: Replaced calls to ReadNoFence/WriteNoFence with direct volatile field accesses. - **`MsvmPkg/Docs/Clang.txt`**: Added documentation outlining clang build setup and commands. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #54710354, #57987809
Clang: Braces initializing subobjects (and maybe not scalars, though that is allowed).
ExcludeMainFvFromMeasurementLib.c:19:
suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
(EFI_PHYSICAL_ADDRESS) FixedPcdGet64 (PcdFvBaseAddress),
VariableDxe.c: suggest braces around initialization of subobject
[-Werror,-Wmissing-braces]
EFI_GUID hyperVGuid = HYPERV_PRIVATE_NAMESPACE;
^~~~~~~~~~~~~~~~~~~~~~~~
VariableDxe.c: expanded from macro 'HYPERV_PRIVATE_NAMESPACE'
{ 0x610b9e98, 0xc6f6, 0x47f8, 0x8b, 0x47, 0x2d, 0x2d, 0xa0, 0xd5, 0x2a, 0x91 }
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
VpcivscDxe.c: error: suggest braces around initialization of subobject
[-Werror,-Wmissing-braces]
0, // Slot
^
{}
etc.
I think this is a nice feature of the language and Clang is too warning-prone, but oh well.
Related work items: #57987809
…ported. This is a separate library to easily ensure there is one everything, and no duplicates. ---- Temporary stubbing of assembly routines to support Clang builds until the port is completed. This PR adds a temporary library with stub functions for assembly routines, enabling Clang compilation support and updating various module manifests to integrate it. - `MsvmPkg/Library/ClangTemporaryLib/ClangTemporaryLib.c`: Introduces dummy implementations for multiple assembly entry points under a Clang-specific flag. - `MsvmPkg/Library/ClangTemporaryLib/ClangTemporaryLib.inf`: Provides the module descriptor for the new temporary library. - Multiple project manifest files (e.g., `HostVisibilityLib.inf`, `CpuDxe.inf`, `EfiHvDxe.inf`, `BdDebugLib.inf`, `DxeBdLib.inf`, and DSC files) are updated to reference `ClangTemporaryLib`. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #54710354, #57987809, #60275790
Clang: Fix `ui64` suffix.
```
HvTimerLib.c: error: invalid suffix 'ui64' on integer constant
Stall100ns(MicroSeconds * 10ui64);
^
```
Use portable `stdint.h` and `UINT64_C` for constant of type `uint64_t`, presumed to be like `UINT64`.
There are 2-3 ways to define `uint64_t` and `UINT64`, so they are hypothetically different.
- unsigned __int64, very old option in Microsoft and Digital compilers
- unsigned long long, very old option in other compilers, and portable since C99
- unsigned long on all Unix 64bit targets
`__int64` and `long long` are likely transparent aliases of each other on systems with `__int64`, so this might be two options, not three. (i.e. see if the mangle the same or not in C++, they probably do).
Related work items: #57987809
… And update ARMv7 to ARMv8.0 ReadAcquire/WriteRelease are duplicated in EfiNt.h and Synchronization.h and not portable. Fix both. **NOTE: This also changes the code sequences.** The original Windows ARM64 code was copied from ARM32. I reported this a few years ago, spurring Windows to update. That is where the `__dmb` came from. ARM64 v1.0 is ARMv8.0 and offers ldar/stlr. Use that instead. vmbus.sys on the other side, has already moved ahead similarly, and beyond (to `ldapr`).
A few months ago, either because of ARM64 port, or more likely, compiler update, a use of: _InterlockedExchange64 got replaced by InterlockedCompareExchange64. Because _InterlockedExchange64 was no longer linking. These are not drop-in replacements, and the resulting code is not convincingly correct. The reason this would change from linking to not, is because this was inlining a sequence that ARM says not to use going forward, on newer hardware. The library this calls, on mainstream Windows, dynamically detects hardware and choses ARMv8.0 instructions or ARMv8.1. UEFI does not presently have this library. Our interlocked code, upstream, including InterlockedExchangeCompare64 is largely assembly, unchanged for years, and not following ARM's recent guidance. It does not matter if we inline or call EFI, we are not following guidance either way. None of the three states follow guidance: - main before this PR - this PR - before the PR this is basically undoing In this PR we ask the compiler to inline the code, so it links. Sticking with ARMv8.0 code. We need to visit this more. i.e. to be adaptive at runtime to 8.0 vs. 8.1, in the upstream code, and here. Upstream should probably add InterlockedExchange64 as well. Note that the problem was added to fix the ARM64 build, but the problem is portable. This could be a shipping race in AMD64. This will require a little more clang porting work also. Thank @<Oliver Smith-Denny> again for showing me where to set compiler/linker options. ---- #### AI description (iteration 2) #### PR Classification Bug fix addressing atomic operation usage and ARM build configuration. #### PR Summary This pull request corrects the implementation of atomic flag exchanges in the Vmbus driver and updates ARM compilation settings. - `MsvmPkg/VmbusDxe/VmbusRoot.c`: Changed the flag pointer type from UINT64* to INT64* and replaced the use of InterlockedCompareExchange64 with _InterlockedExchange64 for proper atomic operations. - `MsvmPkg/MsvmPkgAARCH64.dsc`: Updated the compiler flags by adding `/d2overrideInterlockedIntrinsArm64-` to override inline interlocked sequences on ARM. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #57987809, #60275790
…tor.asm. So that CpuAsm.nasm can be unmodified from upstream.
…arrierWithoutFence. Renamed MemoryBarrierWithoutFence to CompilerBarrier. We might consider also calling it CompilerReadWriteBarrier. There's no great answer here, as it is officially deprecated, does not do what people think, and has a portable replacement, that tries to have a clearer meaning, but does not work on Msvc. (I'm not sure the meaning is clearer. I had to look deeper. But ultimately, stdatomic provides same functionality, with portable standard names..except on Msvc, that does not work.) Moving it to assembly is probably a good idea, to strengthen it. Or rather, both _ReadWriteBarrier, and call AsmCompilerBarrier, asm being, really really, compiler cannot see through it. However that is not hte current semantic. This PR is "cleanup and port", not "strengthen". I'll PR "strengthen" separately, later, maybe. Related work items: #57987809, #60275790
Types `long` and `unsigned long` should be avoided when either specific size or pointer-sized is needed. And then arguably avoid others such as long long. The best types are in `stdint.h`. Second past are the NT/EFI types. There are few places to use long, mainly with the Msvc intrinsics. This also fixes the two definitions of `NTSTATUS` to be the same. Later we will single instance them. ---- This pull request is a code cleanup that enhances portability by updating legacy integer type definitions to fixed-width types. The changes standardize integer types across the codebase to improve consistency and support clang cleanup efforts. - `MsvmPkg/Include/Private/EfiNt.h`: Replaces legacy type definitions with fixed-width types from <stdint.h>, updates include directives, and redefines NTSTATUS. - `MsvmPkg/Sec/X64/SecP.h`: Changes function return types from "long long" to "int64_t" for better type consistency. - `MsvmPkg/Include/Vmbus/NtStatus.h`: Updates NTSTATUS definition to use int32_t. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #60275790
… over-read. In this case the warning points out where the code really does not do what the author intended, including an array over-read. Related work items: #60275790
This was interesting. Back when I worked for a few weeks with EFI on Xbox, I removed all EFIAPI. Surely nobody would again make the NT/x86 mistake of having multiple calling conventions. I recall at the time, gcc for EFI was, like, Cygwin gcc. But now I see, gcc for EFI on Linux/amd64, is just regular gcc, targeting SysV with `-DEFIAPI=attribute((ms))` (not the exact spelling) on the command line. Therefore keeping the default calling convention but selectively changing it only as required by EFI. As long as all functions and function pointers are declared correctly, no problem (same as NT/x86). This leads to errors when you mix them up -- function pointer parameter (callback) with function with different calling convention. Clang does not have this problem, as it uses a different target instead, therefore the default calling convention matches EFIAPI. Msvc of course does not have this problem. We need to then be mindful to declare all assembly functions correctly, i.e. generally with EFIAPI, assuming they all work with Msvc and Clang, and none were interfacing only with gcc. Gcc is really a wildcard. This could be Cygwin/Msys gcc also, with a different default.
This is only to change the comment character from semicolon to pound, as the rest of the file is already doing. We will soon delete this file and use .nasm instead. Related work items: #57987809
This is a _little_ heavy handed, but, really, the extra size is very little, like maybe 12K per PE. If you figure maybe 6 sections: text data rdata reloc rsrc pdata, which really could be just text data rdata (the rest can be merged into rdata, depending on discardability), and on average 2K lost per section. And then even if you do not have execute-in-place, loading is just copying, which is super nice. This is how probably all other load formats work, i.e. Linux and Mac, where kernel doesn't participate so much in user loading (yeah I know, there's no kernel or user here). Thanks to @<Oliver Smith-Denny> for the answers here, seeing the module in the error and knowing where we can put compiler/linker flags, and knowing why we have the redundant 4K. Error fixed by this was: ``` stuart_build -c MsvmPkg\PlatformBuild.py BLD_*_LEGACY_DEBUGGER=1 TARGET=RELEASE TOOL_CHAIN_TAG=CLANGPDB or, same result: stuart_build -c MsvmPkg\PlatformBuild.py TOOL_CHAIN_TAG=CLANGPDB ERROR - Compiler #7000 from : Failed to generate FV ERROR - Compiler #7000 from : Failed to execute command INFO - GenFv: ERROR 3000: Invalid INFO - PE image Section-Alignment and File-Alignment do not match : Q:\src\hyperv.uefi\Build\MsvmX64\RELEASE_CLANGPDB\FV\Ffs\52C05B14-0B98-496c-BC3B-04B50211D680PeiCore\52C05B14-0B98-496c-BC3B-04B50211D680.ffs. INFO - GenFv: ERROR 3000: Invalid INFO - Could not rebase Q:\src\hyperv.uefi\Build\MsvmX64\RELEASE_CLANGPDB\FV\Ffs\52C05B14-0B98-496c-BC3B-04B50211D680PeiCore\52C05B14-0B98-496c-BC3B-04B50211D680.ffs. ``` And, look closely, for "PeiCore" in there, then `dir /s/b PeiCore.dll PeiCore.efi` and `link /dump /headers`, but still, I didn't see the module name buried, and was not hopeful I'd figure out where to put the linker flags, and didn't know why Msft worked and Clang did not -- all addressed by Oliver and obvious once you see it. For reference, in os.2020, you'd either change makefile.def (seriously, this is the right setting, for everyone), or: `LINKER_FLAGS=$(LINKER_FLAGS) ...` MSBuild users can enquire, as SQL and SQLPAL also do this globally. Related work items: #54710354, #57987809
…tant.
Gcc rejects 4 character constants.
Use `SIGNATURE_32` like the rest of UEFI.
Tested like this:
```
C:\s>type 1.c
int a = 'GBDK';
int d = SIGNATURE_32('G','B','D','K');
int e = SIGNATURE_32('K','D','B','G');
C:\s>cl /c /FAsc 1.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.43.34810 for x64
C:\s>more 1.cod
a DD 04742444bH
d DD 04b444247H
e DD 04742444bH
```
Thus showing which form is equivalent.
I pondered if "DataBlock" was in there, 'DB' that is not likely it, it is "Kernel DeBuGGer".
Related work items: #60275790
Related work items: #57987809
Gcc warns about every pragma warning. Clang silently ignores them, but there is an area (not in this PR) where it sorta matters. Related work items: #57987809
#### AI description (iteration 1) #### PR Classification This PR makes an API change by removing the EFIAPI calling convention from a function pointer typedef to ensure consistent behavior with GCC. #### PR Summary The pull request removes the EFIAPI modifier from the FILTER_ROUTINE typedef in `MsvmPkg/Library/DeviceBootManagerLib/DeviceBootManagerLib.c`, aligning the calling convention with GCC requirements and supporting virtual UEFI operation at the intended privilege levels. - `MsvmPkg/Library/DeviceBootManagerLib/DeviceBootManagerLib.c`: Removed EFIAPI from the FILTER_ROUTINE typedef declaration. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #41399366, #57987809
Related work items: #57987809
…ded. Related work items: #60275790
The code is attempting to use space at the end of buffer as scratch, and sets it to 0xff. Or rather, it computes size it might want to use, but then uses a different size and ignores the computed size. This PR does not "fix", does not make it do what it might intend. It keeps the existing behavior and throws out the computation of other data, that it does not really use. Gcc warns about essentially unused locals. This is separated from other cases, because those probably did what the author meant, and this one probably does not. Related work items: #57987809
…ure. Related work items: #57987809
… not needed." This reverts commit 3ba9392041a0412fe0e84def09e284491cf53e02. It seems unnecessarily risky. While is true, the barrier is redundant given its stated purpose, the barrier does more than is stated. And it is both _extremely_ cheap, and costly in risk if we need it but don't do it. "extremely cheap": it impedes compiler optimization, very little. Msvc is very conservative, and assume aliasing everywhere, so quite likely the barrier never did anything for Msvc. But clang/gcc are less conservative -- we should probably build with whatever is the flag that makes them like Msvc, really. There's too much legacy code and thinking and too little stress testing, and too little to gain. `-fno-strict-aliasing` I think it is. We should ponder this, seriously, unless upstream already does it, which will be clear in the logs. Related work items: #57987809
This is small debt I recently knowingly introduced, in the interest of fixing a possible bug, in all Msvc builds. Related work items: #57987809
Gcc/Clang: STATIC_ASSERT repair and cleanup. - Repair: Earlier I missed some of the single parameter STATIC_ASSERTs. - Cleanup: Introduce STATIC_ASSERT_1 that repeats its first parameter, switch STATIC_ASSERT with second empty parameter and C_ASSERT to it. Tested at least with one assert and one compiler, where before they did not show any message and after they nicely rendered the expression. Imho they should be showing the expression, but I guess that is what the message is for. Low tech and terse: STATIC_ASSERT_1, '1' means one parameter. Alternatives, not taken but not clearly worse or better: STATIC_ASSERT one parameter STATIC_ASSERT_MSG with second parameter, message. STATIC_ASSERT_MESSAGE spelled out name STATIC_ASSERT_2 for two parameters, perhaps the rare case STATIC_ASSERT variadic implemented in the compiler maybe in newer standard. In an alternate universe you can overload C preprocessor macros _easily_ on number of parameters. Actually, I think you already can, but not easily, via dispatching within an a variadic one. We can come back to this later, except, `STATIC_ASSERT` is taken by upstream. We'd have to work with them or make a new name. ---- This PR focuses on code cleanup and build fix by replacing and standardizing static assertion macros for improved Clang/GCC compatibility. The changes replace calls to the old STATIC_ASSERT (and C_ASSERT) macros with the new STATIC_ASSERT_1 macro across multiple modules to address Clang build issues for hyperv.uefi AARCH64. - `MsvmPkg/PlatformPei/Hv.c`, `StorvscDxe/VstorageProtocol.h`, and related files now use STATIC_ASSERT_1 for type and size checks. - Assertion macros have been updated in `kdnetinterface.h`, `SmbiosPlatform.c`, and `ChannelMessages.h` to ensure consistent static assertions. - A new header file, `StaticAssert1.h`, is introduced to define the STATIC_ASSERT_1 macro for unified usage. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #57987809
Gcc is even pickier than Clang, at least with whatever flags we are using.
```
C:\s>type 1.c
typedef struct { int ProcessorSteppingId : 4; } PROCESSOR_SIGNATURE;
typedef struct { int ProcessorFpu : 1; } PROCESSOR_FEATURE_FLAGS;
typedef struct {
PROCESSOR_SIGNATURE Signature;
PROCESSOR_FEATURE_FLAGS FeatureFlags;
} PROCESSOR_ID_DATA;
typedef struct {
int i;
PROCESSOR_ID_DATA ProcessorId;
} SMBIOS_TABLE_TYPE4;
struct { SMBIOS_TABLE_TYPE4 Formatted; } clangWarn = {0, 0};
struct { SMBIOS_TABLE_TYPE4 Formatted; } gccWarn = {{0, {0}}};
struct { SMBIOS_TABLE_TYPE4 Formatted; } noWarn = {{0, {{0}}}};
C:\s>cl /c /Wall 1.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.43.34810 for x64
1.c
C:\s>"C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\Llvm\x64\bin\clang.exe" -c 1.c -Wall
1.c:14:60: warning: suggest braces around initialization of subobject [-Wmissing-braces]
14 | struct { SMBIOS_TABLE_TYPE4 Formatted; } clangWarn = {0, 0};
| ^
| {}
1.c:14:60: warning: suggest braces around initialization of subobject [-Wmissing-braces]
14 | struct { SMBIOS_TABLE_TYPE4 Formatted; } clangWarn = {0, 0};
| ^
| {}
2 warnings generated.
$ gcc -c -Wall 1.c
1.c:14:56: warning: missing braces around initializer [-Wmissing-braces]
14 | struct { SMBIOS_TABLE_TYPE4 Formatted; } clangWarn = {0, 0};
| ^
| { -
| {{0}}}
1.c:15:56: warning: missing braces around initializer [-Wmissing-braces]
15 | struct { SMBIOS_TABLE_TYPE4 Formatted; } gccWarn = {{0, {0}}};
| ^
| {}
1.c:15:56: warning: missing braces around initializer [-Wmissing-braces]
15 | struct { SMBIOS_TABLE_TYPE4 Formatted; } gccWarn = {{0, {0}}};
| ^
| {}
```
Related work items: #57987809
It is useful to search for "===" to find merge conflicts. Not to find comments. Nor to find DebugPrint. In this PR, change the comments to "---". Leave DebugPrint alone. ---- #### AI description (iteration 1) #### PR Classification This pull request is a code cleanup that removes redundant apparent merge markers from comment blocks. #### PR Summary The changes standardize comment divider styles across multiple files to improve clarity and maintain code consistency. - In `MsvmPkg/Include/Hv/HvGuestCpuid.h`, replaced "====" markers with "----" for clarity. - In `MsvmPkg/AziHsmDxe/AziHsmBKS3.c`, updated internal TPM comments to use "----" markers. - In `MsvmPkg/Include/Guid/FrontPageEventDataStruct.h`, changed "====" dividers to "----" for a cleaner look. - In `MsvmPkg/Library/MsPlatBdsLib/BdsPlatform.c`, revised comment delimitation from "====" to "----". <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #60275790
``` lld-link: error: duplicate symbol: gEventIndexDiscarded >>> defined at Q:\src\hyperv.uefi\MsvmPkg\VmbfsDxe\VmbfsDxe.c >>> VmbfsDxe.lib(VmbfsDxe.obj) >>> defined at Q:\src\hyperv.uefi\MsvmPkg\VmbfsDxe\VmbfsFile.c >>> VmbfsDxe.lib(VmbfsFile.obj) ld-link: warning: /align specified without /driver; image may not run lld-link: error: duplicate symbol: mInternalEventServices >>> defined at Q:\src\hyperv.uefi\MsvmPkg\Library\EmclLib\EmclLib.c >>> EmclLib.lib(EmclLib.obj) >>> defined at Q:\src\hyperv.uefi\MsvmPkg\NetvscDxe\NetvscDxe.c >>> NetvscDxe.lib(NetvscDxe.obj) ``` etc. One thing may be controversial is I used `__declspec(selectany)` instead of perhaps `static`. This is probably more like the original, and more efficient. There's only of each, any set, gets them "all", instead of getting them multiple times. Oh and it should be in some common header to ensure it always has the same type. Alternatively, we could sprinkle `__declspec (selectany)` all over for these fixes. Alternatively, put these all into a library and remove the non-standard `__declspec (selectany)`. Call is MsGlobals? MsGlobalVariables? I am surprised this did not build with Clang. Related work items: #57987809, #60275790
Rename `__cpuid` to `MsCpuid` and implement it on top of `AsmCpuid`. Related work items: #57987809, #60275790
…n warning/error for padding. 1. Gcc warns for every pragma warning. 2. Clang ignores pragma warning. We are using pragma warning, to enable (not disable) a warning, to catch problems. This will not catch problems on Gcc/Clang. Use `STATIC_ASSERT` instead. I made errors writing this, showing that it works, at least with Gcc, but it is convincing for all three. Related work items: #57987809
```` INFO - /home/jay/hyperv.uefi/MsvmPkg/Include/MsvmBase.h:19:45: error: backslash-newline at end of file [-Werror] INFO - 19 | #define CONTAINS_FIELD(Struct, Size, Field) \ INFO - | ``` Related work items: #57987809
`_BitScanForward64` and `_bittestandset` are Visual C++ intrinsics. Replace them either portable code, or intrinsics in the other compilers. Unit tests are added, manually built and run -- all the versions return the same results. One might quibble, that I changed the Msvc form, slowed it down.
By inspection: - There are only two parameters, not three. - You can directly b or bl to jump or call. That is what the C compiler does. ChatGPT says the range is plus or minus 128MB, so presumably the linker makes "branch islands" if that is not enough. Related work items: #57987809
Related work items: #57987809
…RT_EXPRESSION). ``` INFO - /home/jay/hyperv.uefi/MsvmPkg/Include/StaticAssertExpression.h:8:37: error: left-hand operand of comma expression has no effect [-Werror=unused-value] INFO - 8 | (sizeof (char [(expr) ? 1 : -1]), (value)) INFO - | ^ INFO - /home/jay/hyperv.uefi/MsvmPkg/Include/AssignStruct.h:17:9: note: in expansion of macro STATIC_ASSERT_EXPRESSION ``` ---- #### AI description (iteration 1) #### PR Classification This PR performs a code cleanup by adding an explicit cast to void in a static assertion macro. #### PR Summary The pull request updates the `STATIC_ASSERT_EXPRESSION` macro in `MsvmPkg/Include/StaticAssertExpression.h` to explicitly cast the `sizeof` expression to void, eliminating compiler warnings and supporting the AARCH64 Clang build as outlined in the linked work items. - `MsvmPkg/Include/StaticAssertExpression.h`: Modified the macro to cast the `sizeof` expression to void. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #57987809
Try it. Later we can do more.
… the way of ARM64 work. #### AI description (iteration 1) #### PR Classification This pull request performs a configuration update to isolate x64-specific assembly stubs, facilitating ARM64 support. #### PR Summary The PR updates the build configuration by moving the assembly stubs to be x64-only, thereby preventing conflicts with ARM64 work. - `ClangTemporaryLib.inf`: Renamed the `[Sources]` section to `[Sources.X64]` to restrict compilation to x64 platforms. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #57987809
The default calling convention for gcc is SysV. Functions implemented in assembly or called from assembly therefore need to specify EFIAPI, else have multiple implementations just for calling convention. Related work items: #57987809
…B_CONTEXT. Every pragma warning is a warning or error with gcc. I fixed most of the, but somehow missed some. ---- #### AI description (iteration 2) #### PR Classification This PR fixes pragma warning handling across compilers and updates PACKET_LIB_CONTEXT to improve cross-compiler compatibility. #### PR Summary The pull request standardizes warning suppression by introducing new macros for Clang, GCC, and MSVC, while also updating the PACKET_LIB_CONTEXT structure with reserved fields. - `/MsvmPkg/Include/Pragma.h`: Added and conditionally defined `CLANG_PRAGMA`, `GCC_PRAGMA`, and `MS_PRAGMA` macros. - `/MsvmPkg/Include/WarningDisable.h`: Updated warning disable macros to use the new pragma macros. - `/MsvmPkg/Include/Vmbus/VmbusPacketInterface.h`: Replaced direct pragma directives with standardized macros and added `Reserved[2]` to `PACKET_LIB_CONTEXT`. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #57987809
…y static. Modern gcc/clang default to "no-common". I am not sure I love this, but we have by and large accepted it. We could flip the switch, I later realized. `mInternalEventServices` is more trouble than the others, because it is in multiple places. Mostly we can use our own library to single instance it, but not always. Make this one `static`. Related work items: #41399366, #57987809
…via ArmGetPhysicalAddressBits. The function to get the memory manager feature 0 register changed names. "Aa64" was added or removed. Our .masm one name, their .S has the other. Upstream has the functionality all in one, but I think that is poorly factored -- too much assembly. Also our C deals with invalid values better. The upstream functions is not in .h file. Use the upstream, to have less code, and not edit .h file. Related work items: #57987809
…o CpuAsm.nasm. Port CpuAsm.nasm. I think I copied this from the masm code in fact, and then change directives and remove `ptr`. ChatGPT helped (esp. to write the loops and "push low") CpuAsm.S did not work. There is a very small amount of diveregence, when you disassemble the resulting .objs: - Sometimes the disassembled instructions are the same, but encoding (code bytes) is different, like around mov rsp, rbp. - SetCodeSegement, It appears our masm lags upstream, though both presumably work. This throws off the offsets in the interrupt handling by 0x10 bytes, hurting diffability. - Nasm will use short jmps sometimes. - Masm has xdata. According to ChatGPT, we could build that manually, or use clang-mc. The manual build seems like a bad choice. clang-mc seems like a good choice. xdata is helpful for debugging, but it seems UEFI generally eschews it. I omitted it. We should probably update one or the other regarding SetCodeSegment, so the diff is easier to do. Or split this into a few files, really, so the size of one function does not affect the "addresses" (.obj relative) in another. This appears to be boot debugger code. Related work items: #54710354, #57987809, #60275790
Masm to GNU as Some is from upstream MU (detailed therein). All the instructions are the same. What changes is labels and function start/end. Related work items: #57987809
For clang/am64 we have to use .nasm, which then works with all of msvc, clang, gcc. Tested by disassembling the object file and comparing with Msvc. Related work items: #57987809
mebersol
approved these changes
Mar 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As of making this PR, pulls roughly 56% of the latest changes from closed source, but not including the latest 202502 submodule ingestion -- that needs to be done separate and manually to pull from the open source submodules