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

Try statx for all linux-gnu target. #67774

Merged
merged 1 commit into from
Jan 8, 2020
Merged

Conversation

oxalica
Copy link
Contributor

@oxalica oxalica commented Jan 1, 2020

After rust-lang/libc#1577, which is contained in libc 0.2.66, provides SYS_statx for all Linux platform, so we can try to use statx for all Linux target all linux-gnu targets.

Unfortunately, struct statx and fn statx is not a part of public interface of musl (currently), we still need to invoke it through syscall we does not support statx for musl or other libc impls currently.

Previous PR: #65094

cc @alexcrichton

@rust-highfive
Copy link
Collaborator

r? @rkruppe

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 1, 2020
@hanna-kruppe
Copy link
Contributor

This looks vaguely plausible to me, but I am not familiar with this code or the platforms this change affects, so I'd like to pass the buck to someone who (presumably) is more familiar: r? @alexcrichton

// We need to port these structs here, since libc does not always contain
// them, eg. on musl.
#[repr(C)]
struct statx {
Copy link
Member

@nagisa nagisa Jan 6, 2020

Choose a reason for hiding this comment

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

This is scary. libc has CI in place to verify that we don’t end up with wrong struct definitions, but that’s not true for std… I think it makes more sense overall to just not use statx on the environments that do not want to expose this type.

Copy link
Member

Choose a reason for hiding this comment

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

Yes this should be using the definitions in libc rather than duplicating them here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But musl does not export these structs currently... So we should only enable this when using glibc on Linux?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mati865 No yet. It never exposes statx function wrapper and related structs. It just use it internally.
There's no way to get file birth time from public API, except invoking syscall with manual defined struct statx.

Copy link
Member

Choose a reason for hiding this comment

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

To land, this must be using the statx structure from a location that is verified to be correct. The libc crate is one source which is verified correct, but it doesn't have to be the only one. Linux structures are notorious for changing across architectures and across libraries, so having an unverified copy here won't be maintainable over time.

@bors
Copy link
Contributor

bors commented Jan 6, 2020

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

@oxalica
Copy link
Contributor Author

oxalica commented Jan 8, 2020

@alexcrichton Rebased. Now it is limited to linux-gnu targets.

@oxalica oxalica changed the title Try statx for all linux target. Try statx for all linux-gnu target. Jan 8, 2020
@alexcrichton
Copy link
Member

@bors: r+

FWIW I think it's worthwhile to get these structures into libc for musl targets as well, but that's an issue for upstream libc. As-is this PR looks good to me!

@bors
Copy link
Contributor

bors commented Jan 8, 2020

📌 Commit f5baa03 has been approved by alexcrichton

@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 8, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 8, 2020
Try statx for all linux-gnu target.

After rust-lang/libc#1577, which is contained in `libc` 0.2.66,  provides `SYS_statx` for all Linux platform, so we can try to use `statx` for ~all Linux target~ all linux-gnu targets.

Unfortunately, `struct statx` and `fn statx` is not a part of public interface of musl (currently), ~we still need to invoke it through `syscall`~ we does **not** support statx for musl or other libc impls currently.

Previous PR: rust-lang#65094

cc @alexcrichton
bors added a commit that referenced this pull request Jan 8, 2020
Rollup of 10 pull requests

Successful merges:

 - #67774 (Try statx for all linux-gnu target.)
 - #67781 (Move `is_min_const_fn` query to librustc_mir.)
 - #67798 (Remove wrong advice about spin locks from `spin_loop_hint` docs)
 - #67849 (Add a check for swapped words when we can't find an identifier)
 - #67875 (Distinguish between private items and hidden items in rustdoc)
 - #67887 (`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` have `#[track_caller]`)
 - #67955 (rustdoc: Remove more `#[doc(cfg(..))]` duplicates)
 - #67977 (Updates for VxWorks)
 - #67985 (Remove insignificant notes from CStr documentation)
 - #68003 (ci: fix wrong shared.sh import for publish_toolstate)

Failed merges:

 - #67820 (Parse the syntax described in RFC 2632)
 - #67979 (Move `intravisit` => `rustc_hir` + misc cleanup)

r? @ghost
bors added a commit that referenced this pull request Jan 8, 2020
Rollup of 10 pull requests

Successful merges:

 - #67774 (Try statx for all linux-gnu target.)
 - #67781 (Move `is_min_const_fn` query to librustc_mir.)
 - #67798 (Remove wrong advice about spin locks from `spin_loop_hint` docs)
 - #67849 (Add a check for swapped words when we can't find an identifier)
 - #67875 (Distinguish between private items and hidden items in rustdoc)
 - #67887 (`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` have `#[track_caller]`)
 - #67955 (rustdoc: Remove more `#[doc(cfg(..))]` duplicates)
 - #67977 (Updates for VxWorks)
 - #67985 (Remove insignificant notes from CStr documentation)
 - #68003 (ci: fix wrong shared.sh import for publish_toolstate)

Failed merges:

 - #67820 (Parse the syntax described in RFC 2632)
 - #67979 (Move `intravisit` => `rustc_hir` + misc cleanup)

r? @ghost
@bors bors merged commit f5baa03 into rust-lang:master Jan 8, 2020
@oxalica oxalica deleted the more-statx branch January 9, 2020 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants