Rollup of 10 pull requests #52989
Closed
Rollup of 10 pull requests #52989
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
The regression check is to make beta promotion easier, so it makes more sense to use the Tuesday of the release week (T-2) as the end point of the regression prevention, instead of Thursday (T-0). But since the beta promotion PR is sent at Tuesday evening at UTC, the protection should include the whole Tuesday as well, meaning the 6-week cycle will start from Wednesdays. This will also move the start of the regression protection week one day earlier.
This should address issue 45696. Since we know dropping a box will not access any `&mut` or `&` references, it is safe to model its destructor as only touching the contents *owned* by the box. Note: At some point we may want to generalize this machinery to other reference and collection types that are "pure" in the same sense as box. If we add a `&move` reference type, it would probably also fall into this branch of code. But for the short term, we will be conservative and restrict this change to `Box<T>` alone. The code works by recursively descending a deref of the `Box`. We prevent `visit_terminator_drop` infinite-loop (which can arise in a very obscure scenario) via a linked-list of seen types. Note: A similar style stack-only linked-list definition can be found in `rustc_mir::borrow_check::places_conflict`. It might be good at some point in the future to unify the two types and put the resulting definition into `librustc_data_structures/`. ---- One final note: Review feedback led to significant simplification of logic here. During review, eddyb RalfJung and I uncovered the heart of why I needed a so-called "step 2" aka the Shallow Write to the Deref of the box. It was because the `visit_terminator_drop`, in its base case, will not emit any write at all (shallow or deep) to a place unless that place has a need_drop. So I was encoding a Shallow Write by hand for a `Box<T>`, as a separate step from recursively descending through `*a_box` (which was at the time known as "step 1"; it is now the *only* step, apart from the change to the base case for `visit_terminator_drop` that this commit now has encoded). eddyb aruged that *something* should be emitting some sort of write in the base case here (even a shallow one), of the dropped place, since by analogy we also emit a write when you *move* a place. That led to the revision here in this commit. * (Its possible that this desired write should be attached in some manner to StorageDead instead of Drop. But in this PR, I tried to leave the StorageDead logic alone and focus my attention solely on how Drop(x) is modelled in MIR-borrowck.)
…erations of loops.
(Presumably the place that borrow_check ends up reporting for the error about is no longer the root `Local` itself, and thus the note diagnostic here stops firing.)
…ve cases. After talking about the PR with eddyb, I decided it was best to try to have some test cases that simplify the problem down to its core, so that people trying to understand what the issue is here will see those core examples first.
This optionally adds lldb (and clang, which it needs) to the build. Because rust uses LLVM 7, and because clang 7 is not yet released, a recent git master version of clang is used. The lldb that is used includes the Rust plugin. lldb is only built when asked for, or when doing a nightly build on macOS. Only macOS is done for now due to difficulties with the Python dependency.
Add lldb to the build This optionally adds lldb (and clang, which it needs) to the build. Because rust uses LLVM 7, and because clang 7 is not yet released, a recent git master version of clang is used. The lldb that is used includes the Rust plugin. lldb is only built when asked for, or when doing a nightly build on macOS. Only macOS is done for now due to difficulties with the Python dependency.
…or-box, r=eddyb
[NLL] Dangly paths for box
Special-case `Box` in `rustc_mir::borrow_check`.
Since we know dropping a box will not access any `&mut` or `&` references, it is safe to model its destructor as only touching the contents *owned* by the box.
----
There are three main things going on here:
1. The first main thing, this PR is fixing a bug in NLL where `rustc` previously would issue a diagnostic error in a case like this:
```rust
fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
```
such code was accepted by the AST-borrowck in the past, but NLL was rejecting it with the following message ([playground](https://play.rust-lang.org/?gist=13c5560f73bfb16d6dab3ceaad44c0f8&version=nightly&mode=release&edition=2015))
```
error[E0597]: `**x` does not live long enough
--> src/main.rs:3:40
|
3 | fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
| ^^^^^^^^ - `**x` dropped here while still borrowed
| |
| borrowed value does not live long enough
|
note: borrowed value must be valid for the anonymous lifetime rust-lang#1 defined on the function body at 3:1...
--> src/main.rs:3:1
|
3 | fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to previous error
```
2. The second main thing: The reason such code was previously rejected was because NLL (MIR-borrowck) incorporates a fix for issue rust-lang#31567, where it models a destructor's execution as potentially accessing any borrows held by the thing being destructed. The tests with `Scribble` model this, showing that the compiler now catches such unsoundness.
However, that fix for issue rust-lang#31567 is too strong, in that NLL (MIR-borrowck) includes `Box` as one of the types with a destructor that potentially accesses any borrows held by the box. This thus was the cause of the main remaining discrepancy between AST-borrowck and MIR-borrowck, as documented in issue rust-lang#45696, specifically in [the last example of this comment](rust-lang#45696 (comment)), which I have adapted into the `fn foo` shown above.
We did close issue rust-lang#45696 back in December of 2017, but AFAICT that example was not fixed by PR rust-lang#46268. (And we did not include a test, etc etc.)
This PR fixes that case, by trying to model the so-called `DerefPure` semantics of `Box<T>` when we traverse the type of the input to `visit_terminator_drop`.
3. The third main thing is that during a review of the first draft of this PR, @matthewjasper pointed out that the new traversal of `Box<T>` could cause the compiler to infinite loop. I have adjusted the PR to avoid this (by tracking what types we have previously seen), and added a much needed test of this somewhat odd scenario. (Its an odd scenario because the particular case only arises for things like `struct A(Box<A>);`, something which cannot be constructed in practice.)
Fix rust-lang#45696.
cleanup: Remove `Def::GlobalAsm` Global asm is not something that needs to have a `Def` or `DefId`.
…=alexcrichton Disable debug sections when optimization flags is set for LLD. Currently LLD does not error when optimization is set and debugging information sections are present. (See discussion at https://reviews.llvm.org/D47901) Using `--strip-debug` along with the `-O` option.
check_const: use the same ParamEnv as codegen for statics Fixes at least part of rust-lang#52849 (my CTFE-stress benchmark). Note that I do not know what I am doing here, this is just based on hints from @oli-obk. r? @oli-obk
… r=varkor Crate store cleanup Each commit mostly stands on its own. Most of the diff is lifetime-related and uninteresting.
…eek, r=alexcrichton Align 6-week cycle check with beta promotion instead of stable release. The regression check is to make beta promotion easier, so it makes more sense to use the Tuesday of the release week (T-2) as the end point of the regression prevention, instead of Thursday (T-0). But since the beta promotion PR is sent at Tuesday evening at UTC, the protection should include the whole Tuesday as well, meaning the 6-week cycle will start from Wednesdays. This will also move the start of the regression protection week one day earlier.
…eration, r=pnkfelix NLL: Better Diagnostic When "Later" means "A Future Loop Iteration" Part of rust-lang#52663. r? @pnkfelix
Update LLVM submodule Fix rust-lang#52884 r? @alexcrichton
…-included-in-span, r=pnkfelix NLL mentions lifetimes that are not included in printed span(s). Part of rust-lang#52663. r? @pnkfelix
|
@bors r+ p=10 |
|
📌 Commit 52d4e68 has been approved by |
bors
added
the
S-waiting-on-bors
|
⌛ Testing commit 52d4e68 with merge 9cc53270ef74bd935730b7721bbfac0e87798219... |
|
💔 Test failed - status-travis |
|
The job Click to expand the log.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 |
bors
added
S-waiting-on-review
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:
Def::GlobalAsm#52886 (cleanup: RemoveDef::GlobalAsm)Failed merges:
r? @ghost