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.

Sign up for GitHub

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

Jump to bottom

Abstract ProcThreadAttributeList into its own struct #123604

Merged
merged 1 commit into from
Dec 21, 2024
Merged

Abstract ProcThreadAttributeList into its own struct #123604

merged 1 commit into from
Dec 21, 2024

Conversation

michaelvanstraten
Copy link
Contributor

@michaelvanstraten michaelvanstraten commented Apr 7, 2024 •
edited by ChrisDenton
Loading

As extensively discussed in issue #114854, the current implementation of the unstable windows_process_extensions_raw_attribute features lacks support for passing a raw pointer.

This PR wants to explore the opportunity to abstract away the ProcThreadAttributeList into its own struct to for one improve safety and usability and secondly make it possible to maybe also use it to spawn new threads.

try-job: x86_64-mingw

@rustbot
Copy link
Collaborator

rustbot commented Apr 7, 2024

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 7, 2024
@michaelvanstraten michaelvanstraten mentioned this pull request Apr 7, 2024
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@michaelvanstraten michaelvanstraten force-pushed the proc_thread_attribute_list branch from 4134d3e to 3e6c21a Compare April 7, 2024 19:24
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cre4ture cre4ture mentioned this pull request Apr 19, 2024
Closed
@michaelvanstraten michaelvanstraten marked this pull request as ready for review May 4, 2024 11:50
@michaelvanstraten
Copy link
Contributor Author

r? @Dylan-DPC

@rustbot rustbot assigned Dylan-DPC and unassigned ChrisDenton May 4, 2024
@Dylan-DPC
Copy link
Member

Assigning it back to Chris giving them more time to review it :) (and well i am not the right person to review it :P)

r? @ChrisDenton

@rustbot rustbot assigned ChrisDenton and unassigned Dylan-DPC May 4, 2024
ChrisDenton
ChrisDenton reviewed May 19, 2024
View reviewed changes
Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The public struct should be defined and documented in std/src/os. Internally it can just be a simple wrapper around the std/src/sys types but nothing in sys should be directly exposed to users.

library/std/src/os/windows/process.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented May 19, 2024

☔ The latest upstream changes (presumably #125280) made this pull request unmergeable. Please resolve the merge conflicts.

@michaelvanstraten
Copy link
Contributor Author

michaelvanstraten commented May 22, 2024 •
edited
Loading

@rustbot label +S-waiting-on-author -S-waiting-on-review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2024
@michaelvanstraten michaelvanstraten mentioned this pull request May 27, 2024
Closed
@michaelvanstraten
Copy link
Contributor Author

The public struct should be defined and documented in std/src/os. Internally it can just be a simple wrapper around the std/src/sys types but nothing in sys should be directly exposed to users.

Would https://github.com/rust-lang/rust/blob/master/library/std/src/os/windows/process.rs, even be the right location for it considering it can be used to create a thread? Maybe std/src/os/windows/attribute_list.rs would be appropriate?

@michaelvanstraten
Copy link
Contributor Author

What I don't understand is that there exists a CreateRemoteThreadEx but no CreateThreadEx, or at least I could not find one. The remote-thread function is probably uninteresting to the standard library, while things such as setting the processor group affinity are.

Is it possible to create a thread for the current process with CreateRemoteThreadEx?

@tim-weis
Copy link

Is it possible to create a thread for the current process with CreateRemoteThreadEx?

Yes. Indeed, the OS doesn't expose a special function to create a thread in the current process. NtCreateThread() always requires a process handle to be supplied, which closely resembles the Win32 API's CreateRemoteThread[Ex]() function. The Win32 API's CreateThread() function is a convenience that passes the (pseudo-)handle returned by GetCurrentProcess() to the Native API to create a "remote" thread in the process of the calling thread.

@michaelvanstraten
Copy link
Contributor Author

@ChrisDenton is there something I should do?

@michaelvanstraten michaelvanstraten force-pushed the proc_thread_attribute_list branch from 47927bd to 15a9f45 Compare November 19, 2024 13:45
@michaelvanstraten
Copy link
Contributor Author

@ChrisDenton, I've resolved the two comments I made my self, from my perspective this should be ready to be merged.

@michaelvanstraten michaelvanstraten force-pushed the proc_thread_attribute_list branch from 15a9f45 to 51590a8 Compare November 19, 2024 13:57
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@michaelvanstraten michaelvanstraten force-pushed the proc_thread_attribute_list branch from 51590a8 to 0f24485 Compare November 19, 2024 17:11
@michaelvanstraten
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 21, 2024
@michaelvanstraten michaelvanstraten force-pushed the proc_thread_attribute_list branch from 0f24485 to 09c9f94 Compare November 29, 2024 08:08
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@michaelvanstraten michaelvanstraten force-pushed the proc_thread_attribute_list branch from 09c9f94 to f371952 Compare November 30, 2024 09:18
@michaelvanstraten
Copy link
Contributor Author

@Dylan-DPC do you know somebody who could review this?

@michaelvanstraten
Copy link
Contributor Author

r? @joboet

@rustbot rustbot assigned joboet and unassigned ChrisDenton Dec 16, 2024
@michaelvanstraten
Copy link
Contributor Author

r? @ChrisDenton

@rustbot rustbot assigned ChrisDenton and unassigned joboet Dec 16, 2024
@ChrisDenton
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 20, 2024

📌 Commit f371952 has been approved by ChrisDenton

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 21, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2024
@bors
Auto merge of rust-lang#134590 - matthiaskrgr:rollup-8lz2s62, r=matth…
13170cd 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#123604 (Abstract `ProcThreadAttributeList` into its own struct)
 - rust-lang#128780 (Add `--doctest-compilation-args` option to add compilation flags to doctest compilation)
 - rust-lang#133782 (Precedence improvements: closures and jumps)
 - rust-lang#134509 (Arbitrary self types v2: niche deshadowing test)
 - rust-lang#134524 (Arbitrary self types v2: no deshadow pre feature.)
 - rust-lang#134539 (Restrict `#[non_exaustive]` on structs with default field values)
 - rust-lang#134586 (Also lint on option of function pointer comparisons)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 758ad53 into rust-lang:master Dec 21, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2024
@rust-timer
Unrolled build for rust-lang#123604
b2f2011 
Rollup merge of rust-lang#123604 - michaelvanstraten:proc_thread_attribute_list, r=ChrisDenton

Abstract `ProcThreadAttributeList` into its own struct

As extensively discussed in issue rust-lang#114854, the current implementation of the unstable `windows_process_extensions_raw_attribute` features lacks support for passing a raw pointer.

This PR wants to explore the opportunity to abstract away the `ProcThreadAttributeList` into its own struct to for one improve safety and usability and secondly make it possible to maybe also use it to spawn new threads.

try-job: x86_64-mingw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisDenton ChrisDenton Awaiting requested review from ChrisDenton

Assignees

@ChrisDenton ChrisDenton

Labels
O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.85.0
Development

Successfully merging this pull request may close these issues.
8 participants
@michaelvanstraten @rustbot @rust-log-analyzer @Dylan-DPC @bors @tim-weis @ChrisDenton @joboet