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

UpdateProcThreadAttribute should use u32 for 'attribute' argument #872

Closed
zaddach opened this issue Apr 6, 2022 · 6 comments
Closed

UpdateProcThreadAttribute should use u32 for 'attribute' argument #872

zaddach opened this issue Apr 6, 2022 · 6 comments
Labels
help wanted Extra attention is needed usability Touch-up to improve the user experience for a language projection

Comments

@zaddach
Copy link

zaddach commented Apr 6, 2022

The argument attribute to UpdateProcThreadAttribute is a usize, even though the attributes (such as PROC_THREAD_ATTRIBUTE_SECURITY_CAPABILITIES are u32.

@mikebattista mikebattista self-assigned this Apr 7, 2022
@mikebattista mikebattista added the usability Touch-up to improve the user experience for a language projection label Apr 7, 2022
@mikebattista
Copy link
Contributor

I'm not sure how to handle this one.

The API is defined as accepting a DWORD_PTR so the function definition in the metadata is true to the headers.

The constants are defined in https://github.com/microsoft/win32metadata/blob/main/generation/WinSDK/Partitions/Threading/main.cpp if someone can recommend how they should be changed to align with the API.

@mikebattista mikebattista removed their assignment Apr 17, 2023
@mikebattista mikebattista added the help wanted Extra attention is needed label Apr 17, 2023
@riverar
Copy link
Collaborator

riverar commented Apr 17, 2023

There's a mismatch but I don't think we can do much here, due to ABI compatibility constraints.

@ChrisDenton
Copy link
Contributor

Are PROC_THREAD_ATTRIBUTE_SECURITY_CAPABILITIES used anywhere else? If not then I guess it could be expanded to being pointer-sized?

More generally there are places where the same constants are used for different sized types. Which doesn't play well with strong typing, so there's not really a solution in the general case.

@riverar
Copy link
Collaborator

riverar commented Apr 17, 2023

I think we had a similar discussion in #1502 (comment) where we discussed how various structures, enumerations, etc. may be used in third party code (e.g. libs) and seemingly innocuous deviations from the definitions may cause unintended side effects. (There were some godbolt examples in there demonstrating some optimizer shortcuts that would fail.)

@ChrisDenton
Copy link
Contributor

I absolutely agree we shouldn't be changing the ABI. Both of functions and structs. I'm less clear on integer widths of #defines though. They're already a bit weird (preprocessor magic) and when combined with C/C++'s integer promotion rules I'm not sure how meaningful the size is on its own. I mean, purely in terms of bitwidth. Signed/unsigned might be a bit more meaningful.

@mikebattista
Copy link
Contributor

Closing for now. Reactivate if there's something actionable here.

@mikebattista mikebattista closed this as not planned Won't fix, can't repro, duplicate, stale Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed usability Touch-up to improve the user experience for a language projection
Projects
None yet
Development

No branches or pull requests

4 participants