Skip to content
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.

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

Records in prsht.h missing packing information #1562

Closed
Jake-Shadle opened this issue May 5, 2023 · 4 comments
Closed

Records in prsht.h missing packing information #1562

Jake-Shadle opened this issue May 5, 2023 · 4 comments
Assignees

Comments

@Jake-Shadle
Copy link

The prsht.h header has

#ifdef _WIN64
#include <pshpack8.h>
#else
#include <pshpack4.h>
#endif

at the top of the file, but none of the types have that packing information in the metadata AFAICT.

  • PROPSHEETHEADERA_V1
  • PROPSHEETHEADERA_V2
  • PROPSHEETHEADERW_V1
  • PROPSHEETHEADERW_V2
  • PSHNOTIFY
@mikebattista
Copy link
Collaborator

mikebattista commented May 7, 2023

There's some history here with ClangSharp not being able to detect packing. See dotnet/ClangSharp#233.

@tannergooding is there anything we can do here to detect this? I've tested pshpack1, 2, 4, and 8 in a manually defined header file, and 1 and 2 result in the expected StructLayout while 4 and 8 result in no StructLayout. I believe one of those latter two is the default so no StructLayout is expected, but the other should emit a StructLayout. In my test I didn't use any conditionals.

@tannergooding
Copy link
Member

Overpacking has no impact on struct layout (unlike an __align attribute).

Clang itself doesn't surface any packing information in this case and so ClangSharp can't do any inference based on the fact. The best we could do is emit some attribute that annotates the computed natural packing and then Win32Metadata could factor that into its post processing, if necessary.

Looking at the 5 called out types, the packing has no impact and they are all naturally packed in both scenarios and so the attribute is pshpack headers are unnecessary. Now, if they had a field that was naturally aligned to 8 bytes (on 32-bit) or 16-bytes (on 64-bit) then they would actually have some impact.

@Jake-Shadle
Copy link
Author

Ahh that's a fair point, I'll close this then since the lack of the packing metadata doesn't materially impact bindings.

@mikebattista
Copy link
Collaborator

Sounds good. Thanks Tanner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants