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

Revert "Use OS thread name by default" #123505

Merged
merged 1 commit into from
Apr 6, 2024

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Apr 5, 2024

This reverts #121666 (Use the OS thread name by default if THREAD_INFO has not been initialized) due to #123495 (Thread names are not always valid UTF-8).

It's not a direct revert because there have been other changes since that PR.

@rustbot
Copy link
Collaborator

rustbot commented Apr 5, 2024

r? @joboet

rustbot has assigned @joboet.
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-hermit Operating System: Hermit O-itron Operating System: ITRON O-SGX Target: SGX O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-wasm Target: WASM (WebAssembly), http://webassembly.org/ 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 5, 2024
@ChrisDenton ChrisDenton linked an issue Apr 5, 2024 that may be closed by this pull request
Comment on lines -238 to -301
pub fn get_name() -> Option<CString> {
#[cfg(target_os = "linux")]
const TASK_COMM_LEN: usize = 16;
#[cfg(target_os = "freebsd")]
const TASK_COMM_LEN: usize = libc::MAXCOMLEN + 1;
#[cfg(any(target_os = "netbsd", target_os = "solaris", target_os = "illumos"))]
const TASK_COMM_LEN: usize = 32;
let mut name = vec![0u8; TASK_COMM_LEN];
let res = unsafe {
libc::pthread_getname_np(libc::pthread_self(), name.as_mut_ptr().cast(), name.len())
};
if res != 0 {
return None;
}
name.truncate(name.iter().position(|&c| c == 0)?);
CString::new(name).ok()
}

#[cfg(any(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos"))]
pub fn get_name() -> Option<CString> {
let mut name = vec![0u8; libc::MAXTHREADNAMESIZE];
let res = unsafe {
libc::pthread_getname_np(libc::pthread_self(), name.as_mut_ptr().cast(), name.len())
};
if res != 0 {
return None;
}
name.truncate(name.iter().position(|&c| c == 0)?);
CString::new(name).ok()
}

#[cfg(target_os = "haiku")]
pub fn get_name() -> Option<CString> {
unsafe {
let mut tinfo = mem::MaybeUninit::<libc::thread_info>::uninit();
// See BeOS teams group and threads api.
// https://www.haiku-os.org/legacy-docs/bebook/TheKernelKit_ThreadsAndTeams_Overview.html
let thread_self = libc::find_thread(ptr::null_mut());
let res = libc::get_thread_info(thread_self, tinfo.as_mut_ptr());
if res != libc::B_OK {
return None;
}
let info = tinfo.assume_init();
let name =
core::slice::from_raw_parts(info.name.as_ptr() as *const u8, info.name.len());
CStr::from_bytes_until_nul(name).map(CStr::to_owned).ok()
}
}

#[cfg(not(any(
target_os = "linux",
target_os = "freebsd",
target_os = "netbsd",
target_os = "macos",
target_os = "ios",
target_os = "tvos",
target_os = "watchos",
target_os = "haiku",
target_os = "solaris",
target_os = "illumos"
)))]
pub fn get_name() -> Option<CString> {
None
}
Copy link
Member

Choose a reason for hiding this comment

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

...tbh, this complexity alone makes me question the decision to start down this path, but you could not have foreseen this.

@workingjubilee
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 5, 2024

📌 Commit 7d00826 has been approved by workingjubilee

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 Apr 5, 2024
@lqd
Copy link
Member

lqd commented Apr 5, 2024

IIUC this will also need a beta backport

@ChrisDenton
Copy link
Member Author

Oh right, yeah beta forked after that PR.

I'll need to create a new PR though as this one won't backport cleanly. I'll open a PR against the beta branch and beta nominate it.

@workingjubilee workingjubilee changed the title Revert #121666 Revert "Use OS thread name by default" Apr 5, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Apr 5, 2024
…ngjubilee

Revert "Use OS thread name by default"

This reverts rust-lang#121666 (Use the OS thread name by default if `THREAD_INFO` has not been initialized) due to rust-lang#123495 (Thread names are not always valid UTF-8).

It's not a direct revert because there have been other changes since that PR.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 5, 2024
…llaumeGomez

Rollup of 8 pull requests

Successful merges:

 - rust-lang#121419 (Add aarch64-apple-visionos and aarch64-apple-visionos-sim tier 3 targets)
 - rust-lang#123159 (Fix target-cpu fpu features on Arm R/M-profile)
 - rust-lang#123487 (CFI: Restore typeid_for_instance default behavior)
 - rust-lang#123500 (Revert removing miri jobserver workaround)
 - rust-lang#123505 (Revert "Use OS thread name by default")
 - rust-lang#123509 (Add jieyouxu to compiler review rotation and as a reviewer for `tests/run-make`, `src/tools/run-make-support` and `src/tools/compiletest`)
 - rust-lang#123514 (Fix typo in `compiler/rustc_middle/src/traits/solve/inspect.rs`)
 - rust-lang#123515 (Use `include` command to reduce code duplication)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1bffe75 into rust-lang:master Apr 6, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 6, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2024
Rollup merge of rust-lang#123505 - ChrisDenton:revert-121666, r=workingjubilee

Revert "Use OS thread name by default"

This reverts rust-lang#121666 (Use the OS thread name by default if `THREAD_INFO` has not been initialized) due to rust-lang#123495 (Thread names are not always valid UTF-8).

It's not a direct revert because there have been other changes since that PR.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 14, 2024
…mulacrum

Add missing `unsafe` to some internal `std` functions

Adds `unsafe` to a few internal functions that have safety requirements but were previously not marked as `unsafe`. Specifically:

- `std::sys::pal::unix::thread::min_stack_size` needs to be `unsafe` as `__pthread_get_minstack` might dereference the passed pointer. All callers currently pass a valid initialised `libc::pthread_attr_t`.
- `std::thread::Thread::new` (and `new_inner`) need to be `unsafe` as it requires the passed thread name to be valid UTF-8, otherwise `Thread::name` will trigger undefined behaviour. I've taken the opportunity to split out the unnamed thread case into a separate `new_unnamed` function to make the safety requirement clearer. All callers meet the safety requirement now that rust-lang#123505 has been merged.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 14, 2024
…mulacrum

Add missing `unsafe` to some internal `std` functions

Adds `unsafe` to a few internal functions that have safety requirements but were previously not marked as `unsafe`. Specifically:

- `std::sys::pal::unix::thread::min_stack_size` needs to be `unsafe` as `__pthread_get_minstack` might dereference the passed pointer. All callers currently pass a valid initialised `libc::pthread_attr_t`.
- `std::thread::Thread::new` (and `new_inner`) need to be `unsafe` as it requires the passed thread name to be valid UTF-8, otherwise `Thread::name` will trigger undefined behaviour. I've taken the opportunity to split out the unnamed thread case into a separate `new_unnamed` function to make the safety requirement clearer. All callers meet the safety requirement now that rust-lang#123505 has been merged.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2024
Rollup merge of rust-lang#123879 - beetrees:missing-unsafe, r=Mark-Simulacrum

Add missing `unsafe` to some internal `std` functions

Adds `unsafe` to a few internal functions that have safety requirements but were previously not marked as `unsafe`. Specifically:

- `std::sys::pal::unix::thread::min_stack_size` needs to be `unsafe` as `__pthread_get_minstack` might dereference the passed pointer. All callers currently pass a valid initialised `libc::pthread_attr_t`.
- `std::thread::Thread::new` (and `new_inner`) need to be `unsafe` as it requires the passed thread name to be valid UTF-8, otherwise `Thread::name` will trigger undefined behaviour. I've taken the opportunity to split out the unnamed thread case into a separate `new_unnamed` function to make the safety requirement clearer. All callers meet the safety requirement now that rust-lang#123505 has been merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-hermit Operating System: Hermit O-itron Operating System: ITRON O-SGX Target: SGX O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-wasm Target: WASM (WebAssembly), http://webassembly.org/ 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
Development

Successfully merging this pull request may close these issues.

Thread names are not always valid UTF-8
6 participants