-
Notifications
You must be signed in to change notification settings - Fork 1k
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_like: Unify statx definitions #3978
base: main
Are you sure you want to change the base?
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
src/unix/linux_like/mod.rs
Outdated
mod linux_statx; | ||
pub use self::linux_statx::*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me, but could you inline the definitions here rather than creating an API-specific module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried, but I wasn't able to use s!
and apply cfg!
conditions. Maybe there's some trick that I missed, but otherwise, I'd leave it for a later refactoring, if at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, what exactly is the error? This should work okay, see e.g.
libc/src/unix/linux_like/linux/mod.rs
Lines 1041 to 1043 in c3bc406
cfg_if! { | |
if #[cfg(not(target_arch = "sparc64"))] { | |
s!{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tgross35 not sure what I previously did wrong, but it seems to work now. I'll update the PR, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, the problems are back, the style check isn't happy:
src/unix/linux_like/mod.rs:1892: constant found after extern function when it belongs before
src/unix/linux_like/mod.rs:1933: struct found after constant when it belongs before
src/unix/linux_like/mod.rs:1933: multiple s! macros in one module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wound up disabling that multiple s!
check because it's kind of false-positive-y, so this should be good with a rebase after #4107 lands (~1 hour)
Sorry for all the conflicts, been doing a lot of branch cleanup work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tgross35 thanks a lot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, the other two ordering errors still occur. Perhaps I could move the constants/structs to the other constants/structs. Should I try that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is required that things are ordered typedefs->types->functions->consts, it looks like your cfg_if
block is reversed.
It it still complains after you correct the ordering, I think you might just need to split into three invocations of cfg_if!
and sort it with the rest of the file.
Also cc @maurer regarding android changes |
@rustbot author for the above. Just comment |
Thanks for reviewing! @rustbot review |
Unifying this should be fine, as our There are also at least four new members on this struct, but they ate the padding so what's written here is still right. |
c3bc406
to
2bec769
Compare
2bec769
to
5e9dc4e
Compare
While I'm at it, I am also rebasing onto main. |
☔ The latest upstream changes (presumably #4094) made this pull request unmergeable. Please resolve the merge conflicts. |
5e9dc4e
to
6bdc655
Compare
The statx system call and corresponding constants are defined by the Linux kernel and don't depend on the libc or architecture. The only difference is whether a libc exports the statx syscall wrapper or not. We can thus unify the statx definitions for all Linux "like" platforms: GNU (glibc), Android (bionic), and (in a later commit) musl. Plain u64 (or uint64_t in C) can't be used for the statx fields because bionic defines them as __u64, and provides incompatible definitions of uint64_t and __u64.
6bdc655
to
f333c2a
Compare
Description
The statx system call and corresponding constants are defined by the Linux kernel and don't depend on the libc or architecture. The only difference is whether a libc exports the statx syscall wrapper or not.
We can thus unify the statx definitions for all Linux "like" platforms: GNU (glibc), Android (bionic), and (in a later commit) musl.
The statx struct is in a separate file because the style check doesn't allow multiple s! macros in one file, and #[cfg()] doesn't work in s!.
Plain u64 (or uint64_t in C) can't be used for the statx fields because bionic defines them as __u64, and provides incompatible definitions of uint64_t and __u64.
Sources
none
Checklist
libc-test/semver
have been updated*LAST
or*MAX
areincluded (see #3131)
cd libc-test && cargo test --target mytarget
);especially relevant for platforms that may not be checked in CI (failed due to unrelated error, linux/errqueue.h)