Rollup of 7 pull requests #66259
Merged
Rollup of 7 pull requests #66259
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
Instead coerce to `*const [T; N]` and then cast.
Correct statement in doctests on line 539 of `is_global` method of the `Ipv4Addr` object, which falsely attributed the tests to the broadcast address.
This method does currently not return false for the `site_local` unicast address space. The documentation of the `is_unicast_global` method on lines 1352 - 1382 suggests that this is intentional as the site-local prefix must no longer be supported in new implementations, thus the documentation can safely be updated to reflect that information. If not so, either the `is_unicast_global` method should be updated to exclude the unicast site-local address space, or the `is_global` method itself.
I assumed some sort of Oxford-comma case here, bit have to admit English is not my first language. Co-Authored-By: kennytm <[email protected]>
…nieu Refactor sync::Once `std::sync::Once` contains some tricky code to park and unpark waiting threads. [once_cell](https://github.com/matklad/once_cell) has very similar code copied from here. I tried to add more comments and refactor the code to make it more readable (at least in my opinion). My PR to `once_cell` was rejected, because it is an advantage to remain close to the implementation in std, and because I made a mess of the atomic orderings. So now a PR here, with similar changes to `std::sync::Once`! The initial goal was to see if there is some way to detect reentrant initialization instead of deadlocking. No luck there yet, but you first have to understand and document the complexities of the existing code :smile:. *Maybe not this entire PR will be acceptable, but I hope at least some of the commits can be useful.* Individual commits: #### Rename state to state_and_queue Just a more accurate description, although a bit wordy. It helped me on a first read through the code, where before `state` was use to encode a pointer in to nodes of a linked list. #### Simplify loop conditions in RUNNING and add comments In the relevant loop there are two things to be careful about: - make sure not to enqueue the current thread only while still RUNNING, otherwise we will never be woken up (the status may have changed while trying to enqueue this thread). - pick up if another thread just replaced the head of the linked list. Because the first check was part of the condition of the while loop, the rest of the parking code also had to live in that loop. It took me a while to get the subtlety here, and it should now be clearer. Also call out that we really have to wait until signaled, otherwise we leave a dangling reference. #### Don't mutate waiter nodes Previously while waking up other threads the managing thread would `take()` out the `Thread` struct and use that to unpark the other thread. It is just as easy to clone it, just 24 bytes. This way `Waiter.thread` does not need an `Option`, `Waiter.next` does not need to be a mutable pointer, and there is less data that needs to be synchronised by later atomic operations. #### Turn Finish into WaiterQueue In my opinion these changes make it just a bit more clear what is going on with the thread parking stuff. #### Move thread parking to a seperate function Maybe controversial, but with this last commit all the thread parking stuff has a reasonably clean seperation from the state changes in `Once`. This is arguably the trickier part of `Once`, compared to the loop in `call_inner`. It may make it easier to reuse parts of this code (see rust-lang/rfcs#2788 (comment)). Not sure if that ever becomes a reality though. #### Reduce the amount of comments in call_inner With the changes from the previous commits, the code pretty much speaks for itself, and the amount of comments is hurting readability a bit. #### Use more precise atomic orderings Now the hard one. This is the one change that is not anything but a pure refactor or change of comments. I have a dislike for using `SeqCst` everywhere, because it hides what the atomics are supposed to do. the rationale was: > This cold path uses SeqCst consistently because the performance difference really does not matter there, and SeqCst minimizes the chances of something going wrong. But in my opinion, having the proper orderings and some explanation helps to understand what is going on. My rationale for the used orderings (also included as comment): When running `Once` we deal with multiple atomics: `Once.state_and_queue` and an unknown number of `Waiter.signaled`. * `state_and_queue` is used (1) as a state flag, (2) for synchronizing the data that is the result of the `Once`, and (3) for synchronizing `Waiter` nodes. - At the end of the `call_inner` function we have to make sure the result of the `Once` is acquired. So every load which can be the only one to load COMPLETED must have at least Acquire ordering, which means all three of them. - `WaiterQueue::Drop` is the only place that may store COMPLETED, and must do so with Release ordering to make result available. - `wait` inserts `Waiter` nodes as a pointer in `state_and_queue`, and needs to make the nodes available with Release ordering. The load in its `compare_and_swap` can be Relaxed because it only has to compare the atomic, not to read other data. - `WaiterQueue::Drop` must see the `Waiter` nodes, so it must load `state_and_queue` with Acquire ordering. - There is just one store where `state_and_queue` is used only as a state flag, without having to synchronize data: switching the state from INCOMPLETE to RUNNING in `call_inner`. This store can be Relaxed, but the read has to be Acquire because of the requirements mentioned above. * `Waiter.signaled` is both used as a flag, and to protect a field with interior mutability in `Waiter`. `Waiter.thread` is changed in `WaiterQueue::Drop` which then sets `signaled` with Release ordering. After `wait` loads `signaled` with Acquire and sees it is true, it needs to see the changes to drop the `Waiter` struct correctly. * There is one place where the two atomics `Once.state_and_queue` and `Waiter.signaled` come together, and might be reordered by the compiler or processor. Because both use Aquire ordering such a reordering is not allowed, so no need for SeqCst. cc @matklad
…-obk Don't cast directly from &[T; N] to *const T Split out from rust-lang#64588 r? @oli-obk
Correct error in documentation for Ipv4Addr method Correct statement in doctests on line 539 of `is_global` method of the `Ipv4Addr` object, which falsely attributed the tests to the broadcast address.
Correct deprecated `is_global` IPv6 documentation This method does currently not return false for the `site_local` unicast address space. The documentation of the `is_unicast_global` method on lines 1352 - 1382 suggests that this is intentional as the site-local prefix must no longer be supported in new implementations, thus the documentation can safely be updated to reflect that information. If not so, either the `is_unicast_global` method should be updated to exclude the unicast site-local address space, or the `is_global` method itself.
…ovements, r=oli-obk [mir-opt] Handle return place in ConstProp and improve SimplifyLocals pass Temporarily rebased on top of rust-lang#66074. The top 2 commits are new. r? @oli-obk
invalid_value lint: use diagnostic items This adjusts the invalid_value lint to use diagnostic items. @Centril @oli-obk For some reason, this fails to recognize `transmute` -- somehow the diagnostic item is not found. Any idea why? r? @Centril Cc rust-lang#66075
rustc_metadata: don't let LLVM confuse rmeta blobs for COFF object files. This has likely been a silent issue since 1.10 but only caused trouble recently (see rust-lang#65536 (comment)), when recent changes to the `rmeta` schema introduced more opportunities for COFF parse errors. To prevent any undesired interactions with old compilers, I've renamed the file inside `rlib`s from `rust.metadata.bin` to `lib.rmeta` (not strongly attached to it, suggestions welcome). Fixes rust-lang#65536. <hr/> Before: ``` $ llvm-objdump -all-headers build/*/stage1-std/*/release/deps/libcore-*.rmeta build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/libcore-6b9e8b5a59b79a1d.rmeta: file format COFF-<unknown arch> architecture: unknown start address: 0x00000000 Sections: Idx Name Size VMA Type SYMBOL TABLE: ``` After: ``` $ llvm-objdump -all-headers build/*/stage1-std/*/release/deps/libcore-*.rmeta llvm-objdump: error: 'build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/libcore-6b9e8b5a59b79a1d.rmeta': The file was not recognized as a valid object file ```
|
@bors r+ p=7 rollup=never |
|
📌 Commit 0fec5ab has been approved by |
bors
added
the
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Nov 10, 2019
Rollup of 7 pull requests Successful merges: - #65719 (Refactor sync::Once) - #65831 (Don't cast directly from &[T; N] to *const T) - #66048 (Correct error in documentation for Ipv4Addr method) - #66058 (Correct deprecated `is_global` IPv6 documentation) - #66216 ([mir-opt] Handle return place in ConstProp and improve SimplifyLocals pass) - #66217 (invalid_value lint: use diagnostic items) - #66235 (rustc_metadata: don't let LLVM confuse rmeta blobs for COFF object files.) Failed merges: r? @ghost
|
☀️ Test successful - checks-azure |
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:
is_globalIPv6 documentation #66058 (Correct deprecatedis_globalIPv6 documentation)Failed merges:
r? @ghost