Allow crossarch merge for structs with attributes other than [StructLayout]#2217
Allow crossarch merge for structs with attributes other than [StructLayout]#2217DoctorKrolic wants to merge 1 commit intomicrosoft:mainfrom
[StructLayout]#2217Conversation
DoctorKrolic
requested review from
a team,
jevansaks and
vineeththomasalex
as code owners
| Windows.Win32.System.Diagnostics.Debug.MINIDUMP_USER_STREAM_INFORMATION(X86) removed | ||
| Windows.Win32.System.Diagnostics.Debug.MINIDUMP_USER_STREAM(X64, Arm64) removed | ||
| Windows.Win32.System.Diagnostics.Debug.MINIDUMP_USER_STREAM(X86) removed | ||
| Windows.Win32.UI.Controls.RichEdit.CLIPBOARDFORMAT added |
There was a problem hiding this comment.
I was spot-checking some of these. It looks like [Unicode] or [Documentation] was causing splitting before. But CLIPBOARDFORMAT has StructLayout, so it shouldn't get merged... right?
There was a problem hiding this comment.
CLIPBOARDFORMAT has alignment of 4 on both x86 and x64/Arm64:
I would argue we are in a better place now. In current version of metadata (69.0.7) there are 2 versions of that struct defined: for x86 there is no StructLayout at all and on x64/Arm64 there is StructLayout with Pack = 4. In this PR we get single struct with [StructLayout(LayoutKind.Sequential, Pack = 4)], which precisely matches definition in the C header file
There was a problem hiding this comment.
Reading the updates to the logic in the tool, if 64-bit has structlayout but x86 doesn't then you pick up structlayout from x64. That doesn't seem correct? Shouldn't the logic should be "if StructLayout isn't explicitly defined, the default is 4 on x86 and 8 on x64". You can fold if they're both unspecified (using pointer-sized defaults) or if there's an explicit StructLayout attribute that makes them the same.
| Windows.Win32.UI.Controls.RichEdit.REQRESIZE added | ||
| Windows.Win32.UI.Controls.RichEdit.REQRESIZE(X64, Arm64) removed | ||
| Windows.Win32.UI.Controls.RichEdit.REQRESIZE(X86) removed | ||
| Windows.Win32.UI.Controls.RichEdit.RICHEDIT_IMAGE_PARAMETERS added |
There was a problem hiding this comment.
RichEdit header all-up seems to have the dreaded pragma push:
, so I don't think these can be merged.Double check the logic please! :)
There was a problem hiding this comment.
Yes, but the directive is unconditional and applies both for 32 and 64 bits. See CLIPBOARDFORMAT as an example in comment above. Similar thing happens to other structs from that header.
Double check the logic please! :)
I did. The x86 struct didn't have dedicated StructLayout attribute (I guess, 4 is treated as default on x86 thus not specified explicitly), so the merge was able to happen. The opposite can be observed e.g. for NOTIFYICONDATAW struct (#2126), which has different alignment for x86 vs x64/Arm64 (1 vs 4), so the merge cannot happen
| Windows.Win32.System.Diagnostics.Debug.MINIDUMP_USER_STREAM_INFORMATION(X86) removed | ||
| Windows.Win32.System.Diagnostics.Debug.MINIDUMP_USER_STREAM(X64, Arm64) removed | ||
| Windows.Win32.System.Diagnostics.Debug.MINIDUMP_USER_STREAM(X86) removed | ||
| Windows.Win32.UI.Controls.RichEdit.CLIPBOARDFORMAT added |
There was a problem hiding this comment.
Reading the updates to the logic in the tool, if 64-bit has structlayout but x86 doesn't then you pick up structlayout from x64. That doesn't seem correct? Shouldn't the logic should be "if StructLayout isn't explicitly defined, the default is 4 on x86 and 8 on x64". You can fold if they're both unspecified (using pointer-sized defaults) or if there's an explicit StructLayout attribute that makes them the same.
2 participants
Fixes: #2203