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

Rollup of 8 pull requests #65483

Closed
wants to merge 34 commits into from
Closed

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Oct 16, 2019

Successful merges:

Failed merges:

r? @ghost

oconnor663 and others added 30 commits October 11, 2019 14:23
File handles shouldn't be inheritable in general.
`std::process::Command` takes care of making them inheritable when child
processes are spawned, and the `CREATE_PROCESS_LOCK` protects against
races in that section on Windows. But `File::try_clone` has been
creating inheritable file descriptors outside of that lock, which could
be leaking into other child processes unintentionally.

See also rust-lang#31069 (comment).
ONCE_INIT is deprecated, and so suggesting it as not only being on par with, but before `Once::new` is a bad idea.
Prefer statx on linux if available

This PR make `metadata`-related functions try to invoke `statx` first on Linux if available,
making `std::fs::Metadata::created` work on Linux with `statx` supported.

It follows the discussion in rust-lang#61386 , and will fix rust-lang#59743

The implementation of this PR is simply converting `struct statx` into `struct stat64` with
extra fields for `btime` if `statx` succeeds, since other fields are not currently used.

---

I also did a separated benchmark for `fs::metadata`, `stat64`, `statx`, and `statx` with conversion to `stat64`.
It shows that `statx` with conversion is even more faster than pure `statx`.
I think it's due to `sizeof stat64 == 114` but `sizeof statx == 256`.

Anyway, the bare implementation of `statx` with conversion is only about 0.2% slower than the original impl (`stat64`-family).
With heap-allocation counted (~8.5% of total cost), the difference between `stat` and `statx` (with or without conversion) is just nothing.

Therefore, I think it is not urgent to use bare `struct statx` as underlying representation now.
There is no need to break `std::os::linux::fs::MetadataExt::as_raw_stat` (rust-lang#61386 (comment))

[Separated bare benchmarks](https://gist.github.com/oxalica/c4073ecb202c599fe41b7f15f86dc79c):
```
metadata_ok             time:   [529.41 ns 529.77 ns 530.19 ns]
metadata_err            time:   [538.71 ns 539.39 ns 540.35 ns]
stat64_ok               time:   [484.32 ns 484.53 ns 484.75 ns]
stat64_err              time:   [481.77 ns 482.00 ns 482.24 ns]
statx_ok                time:   [488.07 ns 488.35 ns 488.62 ns]
statx_err               time:   [487.74 ns 488.00 ns 488.27 ns]
statx_cvt_ok            time:   [485.05 ns 485.28 ns 485.53 ns]
statx_cvt_err           time:   [485.23 ns 485.45 ns 485.67 ns]
```

r? @alexcrichton
…ichton

make File::try_clone produce non-inheritable handles on Windows

~**NOT READY FOR REVIEW.** This PR is currently mainly to trigger CI so that I can see what happens. (Is there a better way to trigger CI?) I don't know whether this change makes sense yet.~ (Edit: @Mark-Simulacrum clarified that CI doesn't currently run on Windows.)

---

File handles shouldn't be inheritable in general.
`std::process::Command` takes care of making them inheritable when child
processes are spawned, and the `CREATE_PROCESS_LOCK` protects against
races in that section on Windows. But `File::try_clone` has been
creating inheritable file descriptors outside of that lock, which could
be leaking into other child processes unintentionally.

See also rust-lang#31069 (comment).
InterpCx: make memory field public

I made this field private forever ago because I thought sealing things might be nice. But with the `memory_mut` getter it doesn't actually seal anything, and it's not like we need to invalidate caches on writes to memory or so. And moreover, having to use the getters leads to some annoying borrow checking interactions.

So, let's just make it public (again).

r? @oli-obk
Don't recommend ONCE_INIT in std::sync::Once

ONCE_INIT is deprecated, and so suggesting it as not only being on par with, but before `Once::new` is a bad idea.
Move syntax::ext to a syntax_expand and refactor some attribute logic

Part of rust-lang#65324.

r? @petrochenkov
Update libc to 0.2.64

Passed local tests.

cc potentially interested people: @gnzlbg @tlively
add example for type_name

So users of this function could at least expect what its output for current compiler version.
fmt::Write is about string slices, not byte slices

No idea why the docs talk about bytes, maybe a copy-paste error?
@Centril
Copy link
Contributor Author

Centril commented Oct 16, 2019

@bors r+ p=8 rollup=never

@bors
Copy link
Contributor

bors commented Oct 16, 2019

📌 Commit c10e5c4 has been approved by Centril

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 16, 2019
@bors
Copy link
Contributor

bors commented Oct 17, 2019

⌛ Testing commit c10e5c4 with merge bd0b4f6...

bors added a commit that referenced this pull request Oct 17, 2019
Rollup of 8 pull requests

Successful merges:

 - #65094 (Prefer statx on linux if available)
 - #65316 (make File::try_clone produce non-inheritable handles on Windows)
 - #65319 (InterpCx: make memory field public)
 - #65461 (Don't recommend ONCE_INIT in std::sync::Once)
 - #65465 (Move syntax::ext to a syntax_expand and refactor some attribute logic)
 - #65469 (Update libc to 0.2.64)
 - #65475 (add example for type_name)
 - #65478 (fmt::Write is about string slices, not byte slices)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Oct 17, 2019

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 17, 2019
@rust-highfive
Copy link
Collaborator

The job asmjs of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-10-17T01:55:39.8202146Z 
2019-10-17T01:55:42.5319437Z error[E0308]: mismatched types
2019-10-17T01:55:42.5320092Z    --> src/libstd/sys/unix/fs.rs:790:52
2019-10-17T01:55:42.5320751Z     |
2019-10-17T01:55:42.5322271Z 790 |         let n = cvt(unsafe { lseek64(self.0.raw(), pos, whence) })?;
2019-10-17T01:55:42.5323454Z     |                                                    |
2019-10-17T01:55:42.5324056Z     |                                                    expected i64, found i32
2019-10-17T01:55:42.5324056Z     |                                                    expected i64, found i32
2019-10-17T01:55:42.5324689Z     |                                                    help: you can convert an `i32` to `i64`: `pos.into()`
2019-10-17T01:55:42.7080448Z error: aborting due to previous error
2019-10-17T01:55:42.7080661Z 
2019-10-17T01:55:42.7081710Z For more information about this error, try `rustc --explain E0308`.
2019-10-17T01:55:42.7561945Z [RUSTC-TIMING] std test:false 3.155
---
2019-10-17T01:55:42.7683064Z == clock drift check ==
2019-10-17T01:55:42.7701865Z   local time: Thu Oct 17 01:55:42 UTC 2019
2019-10-17T01:55:42.8760618Z   network time: Thu, 17 Oct 2019 01:55:42 GMT
2019-10-17T01:55:42.8761469Z == end clock drift check ==
2019-10-17T01:55:47.2666837Z ##[error]Bash exited with code '1'.
2019-10-17T01:55:47.2705667Z ##[section]Starting: Upload CPU usage statistics
2019-10-17T01:55:47.2719620Z ==============================================================================
2019-10-17T01:55:47.2719754Z Task         : Bash
2019-10-17T01:55:47.2719825Z Description  : Run a Bash script on macOS, Linux, or Windows

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@oxalica oxalica mentioned this pull request Oct 17, 2019
@Centril Centril closed this Oct 17, 2019
@Centril Centril deleted the rollup-txu60ss branch October 17, 2019 11:44
@Centril Centril added the rollup A PR which is a rollup label Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rollup A PR which is a rollup S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants