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

Linux: add getitimer()/setitimer() #3847

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nathaniel-bennett
Copy link
Contributor

Resolves #1347

@rustbot
Copy link
Collaborator

rustbot commented Aug 17, 2024

r? @JohnTitor

rustbot has assigned @JohnTitor.
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

cfg_if! {
if #[cfg(target_env = "musl")] {
extern "C" {
pub fn getitimer(which: ::c_int, value: *mut ::itimerval) -> ::c_int;
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure I follow, why not using the typedef you created above ?

Copy link
Contributor Author

@nathaniel-bennett nathaniel-bennett Aug 19, 2024

Choose a reason for hiding this comment

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

This is the result of a strange combination of issues. glibc and most other libc implementations typedef __itimer_which_t as an enum when _GNU_SOURCE is set, or as an int otherwise. musl unfortunately opts to just use an int directly within the function definitions for getitimer and setitimer. As a result of this:

  • Using __itimer_whiich_t directly in function definitions causes musl CI to fail as it's not a defined type
  • Removing __itimer_which_t altogether for musl builds still causes CI to fail, as there is currently no way of opting out of enum semver checks (you can opt out for structs, types, everything else but enums)

By defining __itimer_which_t as a type definition in musl and then opting out of it, CI passes.

A more clean solution to this may be to develop enum opt-out functions to the ctest2 crate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another possible approach is not to bother with enums at all.

pub type __itimer_which_t = ::c_int;
...
pub const ITIMER_REAL: __itimer_which_t = 0;
...

There are various places in this crate were C enums are handled as simple constants. Should pass in theory.

@nathaniel-bennett nathaniel-bennett force-pushed the linux-getitimer-api branch 6 times, most recently from c1c2250 to 97f85ed Compare August 19, 2024 06:06
@nathaniel-bennett nathaniel-bennett force-pushed the linux-getitimer-api branch 6 times, most recently from 54197ef to 2e751ed Compare August 20, 2024 19:01
@nathaniel-bennett nathaniel-bennett force-pushed the linux-getitimer-api branch 4 times, most recently from 92adcf7 to f121cd5 Compare August 21, 2024 22:19
@nathaniel-bennett
Copy link
Contributor Author

r? @tgross35

@rustbot rustbot assigned tgross35 and unassigned JohnTitor Aug 31, 2024
Comment on lines +76 to +82
cfg_if! {
if #[cfg(target_env = "musl")] {
pub type __itimer_which_t = ::c_int;
} else {
pub type __itimer_which_t = ::c_uint;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

glibc has int when it is not defined as an enum, and musl doesn't seem to define this type at all - why is the definition done this way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah nevermind, I see the reasoning below. I think we need to work with one of the ways that doesn't expose this on musl.

In general it is probably better to avoid Rust enums to represent C types at all, given how easy it is to cause UB. So some sort of solution that just typedefs an int on glibc seems preferable to me.

@tgross35
Copy link
Contributor

tgross35 commented Nov 6, 2024

@rustbot author based on the above

@bors
Copy link
Contributor

bors commented Nov 20, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getitimer()/setitimer() are missing on Linux
6 participants