Skip to content

Conversation

YohDeadfall
Copy link
Contributor

pthread_getname_np and pthread_setname_np received a wider adoption in past years and was added to:

There's not so much advantage except that the result can be checked in debug builds. Ideally it should be unified with Linux' implementation, but it trims the input.

@rustbot
Copy link
Collaborator

rustbot commented Nov 4, 2024

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
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-unix Operating system: Unix-like 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 Nov 4, 2024
@ibraheemdev
Copy link
Member

ibraheemdev commented Nov 7, 2024

Is there any change here except the debug assertion? I'm not sure I see the benefit of this change, will it fail on older versions of FreeBSD/DragonFly?

@YohDeadfall
Copy link
Contributor Author

While pthread_setname_np isn't well standardized (the np suggests that) and was supported only by GNU originally, there's a movement to make it more portable. NuttX supports it too, but it will take some time to land it in the libc crate.

Yes, it will fail on non-maintained versions of FreeBSD/DragonFly, but:

  • the oldest version FreeBSD maintains is 13, and it already has the function in it (source);
  • for DragonFlyBSD the function is included into 6.0.0 (source), and any prior versions are unmaintained.

I would unify the code between Linux GNU, DragonFlyBSD and FreeBSD, but there's a small implementation difference. The Linux kernel requires thread names being no longer than 16 bytes including a null terminator. Otherwise, it returns an error. Other platforms just truncate the provided name.

// Available since glibc 2.12, musl 1.1.16, and uClibc 1.0.20.
let name = truncate_cstr::<{ TASK_COMM_LEN }>(name);
let res = libc::pthread_setname_np(libc::pthread_self(), name.as_ptr());
// We have no good way of propagating errors here, but in debug-builds let's check that this actually worked.
debug_assert_eq!(res, 0);

@ibraheemdev
Copy link
Member

I see, but the only difference between the FreeBSD/DragonFly and OpenBSD/NuttX code is the debug assertion? So the idea here is to assert that the call is valid on FreeBSD and DragonFly?

@YohDeadfall
Copy link
Contributor Author

Yes, a bit of safety in debug builds instead of having muted errors.

@ibraheemdev
Copy link
Member

I'm not sure this is worth it for a debug assertion, but I don't know our policy on support for these platforms.

@tgross35
Copy link
Contributor

@asomers would you be able to review this?

I'm not sure this is worth it for a debug assertion, but I don't know our policy on support for these platforms.

That is just consistent with the other targets, seems reasonable here.

Copy link
Contributor

@asomers asomers left a comment

Choose a reason for hiding this comment

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

LGTM, from FreeBSD perspective. pthread_setname_np supported on FreeBSD releases as old as 13.0 and 12.2.

Comment on lines 135 to 143
let name = {
cfg_if::cfg_if! {
if #[cfg(target_os = "linux")] {
const TASK_COMM_LEN: usize = 16;
&truncate_cstr::<{ TASK_COMM_LEN }>(name)
} else {
name.to_bytes()
}
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit to get rid of one level of nesting

Suggested change
let name = {
cfg_if::cfg_if! {
if #[cfg(target_os = "linux")] {
const TASK_COMM_LEN: usize = 16;
&truncate_cstr::<{ TASK_COMM_LEN }>(name)
} else {
name.to_bytes()
}
}
};
cfg_if::cfg_if! {
if #[cfg(target_os = "linux")] {
const TASK_COMM_LEN: usize = 16;
let name = &truncate_cstr::<{ TASK_COMM_LEN }>(name);
} else {
let name = name.to_bytes();
}
}

Otherwise lgtm, could you please squash?

@YohDeadfall YohDeadfall force-pushed the pthread-name-fn-with-result branch from c0c6b81 to ce4fd90 Compare January 7, 2025 13:11
@tgross35
Copy link
Contributor

tgross35 commented Jan 7, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 7, 2025

📌 Commit ce4fd90 has been approved by tgross35

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 Jan 7, 2025
@tgross35 tgross35 assigned tgross35 and unassigned ibraheemdev Jan 7, 2025
@tgross35
Copy link
Contributor

tgross35 commented Jan 7, 2025

Sorry, I just noticed the // Linux limits the allowed length of name, other platforms always truncate it. comment must have gotten dropped at some point, could you add it back and resquash?

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 7, 2025
@YohDeadfall
Copy link
Contributor Author

Sorry, I just noticed the // Linux limits the allowed length of name, other platforms always truncate it.

Don't remember seeing it there at all. Some other platforms like macOS or Solaris like truncate the length too, so that's why there's truncate_cstr function that is used in a few places. So, I can add the comment, but it's not very correct. Should I still add it?

@tgross35
Copy link
Contributor

tgross35 commented Jan 7, 2025

Oh, that was me that wrote the comment, was thinking it was in source at some point #132607 (comment). A comment would still be good to explain why we treat linux differently in this block.

Optionally for more defensive programming, change the else block to else if #[cfg(any(target_os = "freebsd", target_os = "dragonfly"))] and add a third else { compile_error!("platform must choose whether to truncate thread names") }.

@YohDeadfall
Copy link
Contributor Author

Ah, okay, then I will include the comment, but mentioning by name that the other two do not enforce any limits in a separate line. It should be a bit shorter than the defensive option and clear what the other platforms are, kinda a mix of the two options. Is it fine?

@tgross35
Copy link
Contributor

tgross35 commented Jan 8, 2025

Yeah anything reasonable is fine here - just a hint for anyone reading as to why we truncate the path on Linux but not other platforms.

@YohDeadfall
Copy link
Contributor Author

@RalfJung, should I also adjust Miri's behavior here or go with a separate PR to miri repo?

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 9, 2025
@scottmcm
Copy link
Member

scottmcm commented Jan 9, 2025

@bors retry

EDIT: was hoping that might reset the status, but nope

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 9, 2025
@scottmcm
Copy link
Member

scottmcm commented Jan 9, 2025

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 9, 2025
@YohDeadfall YohDeadfall force-pushed the pthread-name-fn-with-result branch from aebc703 to 84a4d58 Compare January 9, 2025 18:02
@YohDeadfall YohDeadfall force-pushed the pthread-name-fn-with-result branch from 84a4d58 to 8795750 Compare January 9, 2025 18:32
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

CI is broken currently but I'll reapprove after that is back to normal.

Comment on lines +140 to +142
} else {
// FreeBSD and DragonFly BSD do not enforce length limits.
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just drop the else block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to leave it for clarity, I guess. In Miri it's a common thing to explain the behavior, so anyone can come and get why.

Copy link
Contributor

Choose a reason for hiding this comment

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

Figured this was the reasoning but just wanted to check, fair enough 👍

@tgross35
Copy link
Contributor

CI is still closed but

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 10, 2025

📌 Commit 8795750 has been approved by tgross35

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jan 10, 2025

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 10, 2025
@bors bors merged commit 5eec2b0 into rust-lang:master Jan 10, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 10, 2025
@RalfJung RalfJung mentioned this pull request Jan 10, 2025
@YohDeadfall YohDeadfall deleted the pthread-name-fn-with-result branch January 12, 2025 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like 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.

8 participants