Improve heuristics for struct size field detection and annotate several structs by hand#2206
Improve heuristics for struct size field detection and annotate several structs by hand#2206DoctorKrolic wants to merge 3 commits intomicrosoft:mainfrom
Conversation
| Windows.Win32.NetworkManagement.Rras.RAS_STATS : [Documentation(https://learn.microsoft.com/windows/win32/api/ras/ns-ras-ras_stats)] => [Documentation(https://learn.microsoft.com/windows/win32/api/ras/ns-ras-ras_stats),StructSizeField(dwSize)] | ||
| Windows.Win32.NetworkManagement.Rras.RASADPARAMS removed | ||
| Windows.Win32.NetworkManagement.Rras.RASADPARAMS(X64, Arm64) added | ||
| Windows.Win32.NetworkManagement.Rras.RASADPARAMS(X86) added |
There was a problem hiding this comment.
This is a bug BTW. Adding [StructSizeField] attribute has nothing to do with CPU architecture and should not cause a split. This is at least somewhat related to #2203
There was a problem hiding this comment.
Hmmm yeah that's weird. I don't know how the tooling is determining there's a layout difference. I haven't had time to think about or look into solving the architecture layout discrepancies.
There was a problem hiding this comment.
I debugged the project a little bit and found out the problem is here:
win32metadata/sources/ClangSharpSourceToWinmd/CrossArchSyntaxMap.cs
Lines 324 to 331 in f06337c
Since
RASADPARAMS didn't have any attributes before this PR and we are adding [StructSizeField], this branch is no longer taken for that struct. It appears to be the actual cause of #2203 as well, since RASDIALPARAMSW has a [Unicode] attribute which also breaks this code path. I am not sure why the logic is as it is though. I don't understand why any unrelated attribute makes the cross-arch merge unable to be performed. Can you please take a look?
There was a problem hiding this comment.
Looks like the intent here was to look for StructLayout or other attributes that would affect cross-arch. Now that safe attributes exist, I agree that we need to tighten things up here.
There was a problem hiding this comment.
StructLayout or other attributes that would affect cross-arch
We should white-list those. Can you list all of them please?
|
This is a great improvement, thanks for taking this on. Before we merge this in we should figure out why the attribute is causing that struct to become architecture specific, since devs get upset when that happens. |
Let's say this fixes #2202