Rollup of 6 pull requests #127924
Merged
Rollup of 6 pull requests #127924
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In particular, remove the note saying cdylibs can't link against dylibs — that hasn't been true for over four years. * 2019-11-07: note is written: rust-lang@b54e8ec * 2020-01-23: restriction is lifted (without updating docs): rust-lang@72aaa3a
It can fail when in git is in detached HEAD mode.
This changes `ReentrantLock` to use `ThreadId` for the thread ownership check instead of the address of a thread local. Unlike TLS blocks, `ThreadId` is guaranteed to be unique across the lifetime of the process, so if any thread ever terminates while holding a `ReentrantLockGuard`, no other thread may ever acquire that lock again. On platforms with 64-bit atomics, this is a very simple change. On other platforms, the approach used is slightly more involved, as explained in the module comment. This also adds a `CURRENT_ID` thread local in addition to the already existing `CURRENT`. This allows us to access the current `ThreadId` without the relatively heavy machinery used by `thread::current().id()`.
Use ThreadId instead of TLS-address in `ReentrantLock` Fixes rust-lang#123458 `ReentrantLock` currently uses the address of a thread local variable as an ID that's unique across all currently running threads. This can lead to uninituitive behavior as in rust-lang#123458 if TLS blocks get reused. This PR changes `ReentrantLock` to instead use the `ThreadId` provided by `std` as the unique ID. `ThreadId` guarantees uniqueness across the lifetime of the whole process, so we don't need to worry about reusing IDs of terminated threads. The main appeal of this PR is thus the possibility of changing the `ReentrantLock` API to guarantee that if a thread leaks a lock guard, no other thread may ever acquire that lock again. This does entail some complications: - previously, the only way to retrieve the current thread ID would've been using `thread::current().id()` which creates a temporary `Arc` and which isn't available in TLS destructors. As part of this PR, the thread ID instead gets cached in its own thread local, as suggested [here](rust-lang#123458 (comment)). - `ThreadId` is always 64-bit whereas the current implementation uses a usize-sized ID. Since this ID needs to be updated atomically, we can't simply use a single atomic variable on 32 bit platforms. Instead, we fall back to using a (sound) seqlock on 32-bit platforms, which works because only one thread at a time can write to the ID. This seqlock is technically susceptible to the ABA problem, but the attack vector to create actual unsoundness has to be very specific: - You would need to be able to lock+unlock the lock exactly 2^31 times (or a multiple thereof) while a thread trying to lock it sleeps - The sleeping thread would have to suspend after reading one half of the thread id but before reading the other half - The teared result from combining the halves of the thread ID would have to exactly line up with the sleeping thread's ID The risk of this occurring seems slim enough to be acceptable to me, but correct me if I'm wrong. This also means that the size of the lock increases by 8 bytes on 32-bit platforms, but this also shouldn't be an issue. Performance wise, I did some crude testing of the only case where this could lead to real slowdowns, which is the case of locking a `ReentrantLock` that's already locked by the current thread. On both aarch64 and x86-64, there is (expectedly) pretty much no performance hit. I didn't have any 32-bit platforms to test the seqlock performance on, so I did the next best thing and just forced the 64-bit platforms to use the seqlock implementation. There, the performance degraded by ~1-2ns/(lock+unlock) on x86-64 and ~6-8ns/(lock+unlock) on aarch64, which is measurable but seems acceptable to me seeing as 32-bit platforms should be a small minority anyways. cc `@joboet` `@RalfJung` `@CAD97`
…_crate, r=petrochenkov make pub_use_of_private_extern_crate show up in cargo's future breakage reports This has been a lint for many years. However, turns out that outright removing it right now would lead to [tons of crater regressions](rust-lang#127656 (comment)) due to crates depending on an ancient version of `bitflags`. So for now this PR just makes this future-compat lint show up in cargo's reports, so people are warned when they use a dependency that is affected by this. r? `@petrochenkov`
Use Option's discriminant as its size hint I was looking at this in MIR after a question on discord, and noticed that it ends up with a switch in MIR (<https://rust.godbolt.org/z/3q4cYnnb3>), which it doesn't need because (as `Option::as_slice` uses) the discriminant is already the length.
…t-lint, r=compiler-errors Add internal lint for detecting non-glob imports of `rustc_type_ir::inherent` rust-lang#127627 (comment) r? compiler-errors
Update extern linking documentation In particular, remove the note saying cdylibs can't link against dylibs — that hasn't been true for over four years. * 2019-11-07: note is written: rust-lang@b54e8ec * 2020-01-23: restriction is lifted (without updating docs): rust-lang@72aaa3a
Allow a git command for getting the current branch in bootstrap to fail Found by `@lukas-code` [here](rust-lang#127680 (comment)). The bug was introduced in rust-lang#127680 (before, the command was allowed to fail). r? `@onur-ozkan`
rustbot
added
S-waiting-on-review
|
@bors r+ rollup=never p=6 |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
📌 Perf builds for each rolled up PR:
previous master: 5753b30676 In the case of a perf regression, run the following command for each PR you suspect might be the cause: |
This was referenced Jul 18, 2024
Merged
|
Finished benchmarking commit (5affbb1): comparison URL. Overall result: ❌✅ regressions and improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary -1.5%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 2.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary 0.0%, secondary 0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 768.843s -> 768.766s (-0.01%) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Successful merges:
ReentrantLock#124881 (Use ThreadId instead of TLS-address inReentrantLock)rustc_type_ir::inherent#127854 (Add internal lint for detecting non-glob imports ofrustc_type_ir::inherent)r? @ghost
@rustbot modify labels: rollup
Create a similar rollup