Abstract `ProcThreadAttributeList` into its own struct #123604

michaelvanstraten · 2024-04-07T19:15:57Z

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 · 2024-04-07T19:16:04Z

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

michaelvanstraten · 2024-05-01T10:08:59Z

library/std/src/os/windows/process.rs

+#[unstable(feature = "windows_process_extensions_raw_attribute", issue = "114854")]
+pub use sys::process::{ProcThreadAttributeList, ProcThreadAttributeListBuilder};


I don't exactly know the right way to export this here. @Dylan-DPC can you guide me on this?

As with the imports above, you need to start the import with crate::. But see my review comment.

michaelvanstraten · 2024-05-04T11:51:39Z

r? @Dylan-DPC

Dylan-DPC · 2024-05-04T13:09:37Z

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

ChrisDenton

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.

ChrisDenton · 2024-05-19T15:35:46Z

library/std/src/os/windows/process.rs

+#[unstable(feature = "windows_process_extensions_raw_attribute", issue = "114854")]
+pub use sys::process::{ProcThreadAttributeList, ProcThreadAttributeListBuilder};


As with the imports above, you need to start the import with crate::. But see my review comment.

michaelvanstraten · 2024-05-19T16:30:18Z

library/std/src/sys/pal/windows/process.rs

+    /// # Safety
+    ///
+    /// This function is marked as `unsafe` because it deals with raw pointers and sizes.
+    /// It is the responsibility of the caller to ensure the value lives longer than the


Suggested change

/// It is the responsibility of the caller to ensure the value lives longer than the

/// It is the responsibility of the caller to ensure the value pointed to by the `value_ptr` lives longer than the

michaelvanstraten · 2024-05-19T16:30:56Z

library/std/src/sys/pal/windows/process.rs

+    ///
+    /// This function is marked as `unsafe` because it deals with raw pointers and sizes.
+    /// It is the responsibility of the caller to ensure the value lives longer than the
+    /// [`ProcThreadAttributeListBuilder`] as well as the validity of the size parameter.


Suggested change

/// [`ProcThreadAttributeListBuilder`] as well as the validity of the size parameter.

/// [`ProcThreadAttributeList`] as well as the validity of the size parameter.

michaelvanstraten · 2024-05-19T16:34:07Z

library/std/src/sys/pal/windows/process.rs

            si_ex = c::STARTUPINFOEXW {
                StartupInfo: si,
-                lpAttributeList: proc_thread_attribute_list.0.as_mut_ptr() as _,
+                lpAttributeList: proc_thread_attribute_list.as_mut_ptr().cast::<c_void>(),


I don't really understand why the win API require a *mut to the attribute list here.

Because the C headers do not distinguish. between const and mut so, when automatically generating the bindings, mut is the most reasonable option. You can open an issue on the win32metadata repo if you'd like them to manually add some metadata so the Rust bindings know which one to use.

Though it doesn't matter too much. In rust, const and mut on pointers is just a lint. You can do, e.g. proc_thread_attribute_list.as_ptr().cast::<c_void>().cast_mut() to go from a reference to a pointer.

I thought that was highly illegal because of some llvm attributes that rustc sets when dealing with shared references.

Pointers and references are different. You cannot turn a & into an &mut. You can turn an & into a *mut so long as you don't ever write to the memory.

Yeah, my concern was that with the win api having an interface that explicitly expects a *mut I thought it might be the case that they actually write to it, but good to know. I will add a safety comment and open an issue over at the win32 crate repo.

bors · 2024-05-19T17:49:36Z

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

michaelvanstraten · 2024-05-22T21:05:30Z

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

michaelvanstraten · 2024-05-27T21:45:59Z

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 · 2024-05-27T22:02:09Z

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 · 2024-05-29T15:44:02Z

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.

bors · 2024-09-09T17:30:58Z

💔 Test failed - checks-actions

michaelvanstraten · 2024-09-09T20:06:36Z

@bors try

bors · 2024-09-09T20:06:38Z

@michaelvanstraten: 🔑 Insufficient privileges: not in try users

michaelvanstraten · 2024-09-09T20:06:59Z

@ChrisDenton try again :).

ChrisDenton · 2024-09-09T20:40:04Z

@bors try

…ute_list, r=<try> 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

bors · 2024-09-09T20:41:19Z

⌛ Trying commit 4575e71 with merge d4d9527...

bors · 2024-09-09T21:53:49Z

💔 Test failed - checks-actions

michaelvanstraten · 2024-09-10T07:11:11Z

@ChrisDenton fourth time is the charm.

ChrisDenton · 2024-09-10T08:29:30Z

@bors try

…ute_list, r=<try> 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

bors · 2024-09-10T08:30:44Z

⌛ Trying commit 9d0a42c with merge 7d87654...

michaelvanstraten · 2024-09-10T09:19:37Z

If this should fail, I will set up a Windows machine to run the doc-tests locally.

bors · 2024-09-10T09:49:31Z

💔 Test failed - checks-actions

michaelvanstraten · 2024-09-10T11:24:40Z

Okay, let me fix this locally before I waste more CI time.

michaelvanstraten · 2024-09-10T15:21:53Z

@ChrisDenton, looks good on my local win11 build. I think you can push to try again once more.

ChrisDenton · 2024-09-11T08:23:02Z

@bors try

bors · 2024-09-11T08:24:18Z

⌛ Trying commit 47927bd with merge c5b2675...

…ute_list, r=<try> 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

bors · 2024-09-11T10:13:02Z

☀️ Try build successful - checks-actions
Build commit: c5b2675 (c5b26757a5741cdad535e0266119ca93331b8f22)

michaelvanstraten · 2024-09-11T17:18:25Z

library/std/src/os/windows/process.rs

+#[unstable(feature = "windows_process_extensions_raw_attribute", issue = "114854")]
+pub struct ProcThreadAttributeListBuilder<'a> {
+    attributes: alloc::collections::BTreeMap<usize, ProcThreadAttributeValue>,
+    _lifetime_marker: marker::PhantomData<&'a ()>,


@ChrisDenton, maybe we could move this into the ProcThreadAttributeValue.

michaelvanstraten · 2024-09-11T21:09:31Z

library/std/src/os/windows/process.rs

+    ///
+    /// This function is marked as `unsafe` because it deals with raw pointers
+    /// and sizes. It is the responsibility of the caller to ensure the value
+    /// lives longer than the [`ProcThreadAttributeListBuilder`] as well as


I think this should be ProcThreadAttributeList, although it does not really matter that much.

rustbot assigned ChrisDenton Apr 7, 2024

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 mentioned this pull request Apr 7, 2024

Support for Raw Pointers in windows_process_extensions_raw_attribute Implementation #121951

Closed

This comment has been minimized.

Sign in to view

michaelvanstraten force-pushed the proc_thread_attribute_list branch from 4134d3e to 3e6c21a Compare April 7, 2024 19:24

This comment has been minimized.

Sign in to view

cre4ture mentioned this pull request Apr 19, 2024

proposal to support raw_attribute with raw pointer #124180

Open

michaelvanstraten commented May 1, 2024

View reviewed changes

michaelvanstraten marked this pull request as ready for review May 4, 2024 11:50

rustbot assigned Dylan-DPC and unassigned ChrisDenton May 4, 2024

rustbot assigned ChrisDenton and unassigned Dylan-DPC May 4, 2024

ChrisDenton reviewed May 19, 2024

View reviewed changes

michaelvanstraten commented May 19, 2024

View reviewed changes

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 mentioned this pull request May 27, 2024

How to use LPPROC_THREAD_ATTRIBUTE_LIST? microsoft/windows-rs#771

Closed

michaelvanstraten force-pushed the proc_thread_attribute_list branch from acbac23 to 4575e71 Compare September 9, 2024 20:06

This comment has been minimized.

Sign in to view

michaelvanstraten force-pushed the proc_thread_attribute_list branch from 4575e71 to 9d0a42c Compare September 10, 2024 07:07

This comment has been minimized.

Sign in to view

michaelvanstraten force-pushed the proc_thread_attribute_list branch 3 times, most recently from 3c14165 to d9e74ac Compare September 10, 2024 15:06

Abstract ProcThreadAttributeList into its own struct

47927bd

michaelvanstraten force-pushed the proc_thread_attribute_list branch from d9e74ac to 47927bd Compare September 10, 2024 15:09

michaelvanstraten commented Sep 11, 2024

View reviewed changes

		#[unstable(feature = "windows_process_extensions_raw_attribute", issue = "114854")]
		pub use sys::process::{ProcThreadAttributeList, ProcThreadAttributeListBuilder};

	/// It is the responsibility of the caller to ensure the value lives longer than the
	/// It is the responsibility of the caller to ensure the value pointed to by the `value_ptr` lives longer than the

	/// [`ProcThreadAttributeListBuilder`] as well as the validity of the size parameter.
	/// [`ProcThreadAttributeList`] as well as the validity of the size parameter.

Abstract ProcThreadAttributeList into its own struct #123604

Are you sure you want to change the base?

Abstract ProcThreadAttributeList into its own struct #123604

Conversation

michaelvanstraten commented Apr 7, 2024 • edited by ChrisDenton Loading

rustbot commented Apr 7, 2024

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelvanstraten commented May 4, 2024

Dylan-DPC commented May 4, 2024

ChrisDenton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelvanstraten May 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 19, 2024

michaelvanstraten commented May 22, 2024 • edited Loading

michaelvanstraten commented May 27, 2024

michaelvanstraten commented May 27, 2024

tim-weis commented May 29, 2024

bors commented Sep 9, 2024

michaelvanstraten commented Sep 9, 2024

bors commented Sep 9, 2024

michaelvanstraten commented Sep 9, 2024

ChrisDenton commented Sep 9, 2024

bors commented Sep 9, 2024

This comment has been minimized.

bors commented Sep 9, 2024

michaelvanstraten commented Sep 10, 2024

ChrisDenton commented Sep 10, 2024

bors commented Sep 10, 2024

michaelvanstraten commented Sep 10, 2024

This comment has been minimized.

bors commented Sep 10, 2024

michaelvanstraten commented Sep 10, 2024

michaelvanstraten commented Sep 10, 2024

ChrisDenton commented Sep 11, 2024

bors commented Sep 11, 2024

bors commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Abstract `ProcThreadAttributeList` into its own struct #123604

Abstract `ProcThreadAttributeList` into its own struct #123604

michaelvanstraten commented Apr 7, 2024 •

edited by ChrisDenton

Loading

michaelvanstraten May 19, 2024 •

edited

Loading

michaelvanstraten commented May 22, 2024 •

edited

Loading