Fixes to mir dataflow #33667
Fixes to mir dataflow #33667
Conversation
(bug was cut/pasted into `rustc_borrowck::bitslice`, so I fixed it there as well.)
…outines. (The crucial thing these changes are working toward (but are not yet in this commit) is a way to pretty-print MIR without having the `NodeId` for that MIR in hand.)
…nreferenced. includes misc bug fixes and removal of useless code.
Incorporates many fixes contributed by arielb1. ---- revise borrowck::mir::dataflow code to allow varying domain for bitvectors. This particular code implements the `BitDenotation` trait for three analyses: * `MovingOutStatements`, which, like `borrowck::move_data`, maps each bit-index to a move instruction, and a 1 means "the effect of this move reaches this point" (and the assigned l-value, if a scoped declaration, is still in scope). * `MaybeInitializedLvals`, which maps each bit-index to an l-value. A 1 means "there exists a control flow path to this point that initializes the associated l-value." * `MaybeUninitializedLvals`, which maps each bit-index to an l-value A 1 means "there exists a control flow path to this point that de-initializes the associated l-value." ---- Revised `graphviz` dataflow-rendering support in `borrowck::mir`. One big difference is that this code is now parameterized over the `BitDenotation`, so that it can be used to render dataflow results independent of how the dataflow bitvectors are interpreted; see where reference to `MoveOut` is replaced by the type parameter `D`. ---- Factor out routine to query subattributes in `#[rustc_mir(..)]`. (Later commits build upon this for some unit testing and instrumentation.) ---- thread through a tcx so that I can query types of lvalues as part of analysis. ---- Revised `BitDenotation::Ctxt`, allowing variation beyond `MoveData`. The main motivation is to ease threading through a `TyCtxt`. (In hindsight it might have been better to instead attach the `TyCtxt` to each of the different dataflow implementations, but that would require e.g. switching away from having a `Default` impl, so I am leaving that experiment for another time.)
its own needs based on attributes attached to the function where it appears.
(The static semantics of `rustc_peek` is derived from attributes attached to the function being compiled; in this case, `rustc_peek(&expr)` observes the dataflow state for the l-value `expr`.)
These use new `rustc_peek` (whose semantics is driven by attribute attached to fn).
…UninitializedLvals`)
|
r? @Aatch (rust_highfive has picked a reviewer for you, use r? to override) |
|
cc @arielb1 |
| /// attached to the function. | ||
| /// | ||
| /// For example, dataflow uses this to inject static assertions so | ||
| /// that `rustc_oeek(potentially_uninitialized)` would actually |
| // it will always appear like Lvalues are initialized; e.g. in | ||
| // a case like: | ||
| // | ||
| // <bitset maps var1 to 0> |
There was a problem hiding this comment.
What is this talking about? rustc_peek(&var1) should be translated to
tmp1 = &var0;
tmp0 = rustc_peek(tmp1) -> bb1;
If it copies the lvalue before taking a reference, that would be totally broken (consider Cell). So it doesn't. Unless something funny is going on here.
There was a problem hiding this comment.
Oh you're right; I didn't update that comment correctly (it was originally authored for a different version of rustc_peek that looked like rustc_peek(var1) (instead of rustc_peek(&var1)), and I erroneously translated its content without thinking about what the actual translation of rustc_peek(&var1) is).
(The original version of rustc_peek had a more fragile API, where one would write rustc_peek(expr) (instead of rustc_peek(&expr)).)
The main point I'm trying to make is this: even with the corrected translation that you wrote:
tmp1 = &var0;
tmp0 = rustc_peek(tmp1) -> bb1;
The normal (naive) dataflow GEN/KILL sets are going to say that tmp1 is initialized. If the semantics of rustc_peek were a naive "look directly at your argument's dataflow bit state", then rustc_peek would query the state of tmp1, which would always indicate that it has been initialized under the naive rules.
So I had written a comment about a work-around I was using for the naive system.
With the new API the story is somewhat related, but the example I gave in the comment isn't helping explain things.
I will update it.
There was a problem hiding this comment.
I guessed. Yeah, you want the state of var0, not of tmp1. I think you could just go to the initialization of tmp1 and use that.
There was a problem hiding this comment.
(I also expected to find the definition of tmp1 when @pnkfelix and I discussed in the work week; but this solution works too.)
There was a problem hiding this comment.
I guess the reason not to do never mind, I see you are doing rustc_peek(&x) is that this code runs before borrowck anyway, so e.g. moves don't matter?&x
|
I gave this a brief review and it seems quite reasonable. I'll try to read a bit more in detail tomorrow though. |
…n code. (it was an artifact of an earlier design of the `rustc_peek` API, but its totally unnecessary now.)
… presentation. As a drive-by: unified pair of match arms that flowed to `bug!`, and replaced `bug!` invocation with a diagnostic `span_err` invocation.
…a client-provided closure.
Also got rid of the `trait HasMoveData`, since I am now just imposing the constraint that `BitDenotation<Ctxt=MoveData<'tcx>>` where necessary instead.
… associated Ctxt item. used this to address a long-standing wart/bug in how filtering-out of values with type impl'ing `Copy` was done.
|
@arielb1 @nikomatsakis The one thing I'd like someone to take a look at is the new My goal was to have a breakdown analogous to My analogous types in My problem is that I used a little bit of nasty unsafe code to accomplish this goal. :( (Namely, I did not see a way to turn a Maybe the right answer here is to not attempt to provide an unsized |
| /// "transfer-function" represnting the overall-effect of the | ||
| /// block, represented via GEN and KILL sets. | ||
| /// | ||
| /// The statement here is `idx_stmt.1`; `idx_stmt.0` is just |
There was a problem hiding this comment.
(note to self: this, like many comments, is out of date and needs to be fixed to reflect changes to the API here.)
| // requires a transmute relying on representation guarantees that may | ||
| // not hold in the future. | ||
|
|
||
| pub struct IdxSet<T: Idx> { |
There was a problem hiding this comment.
I'd have appreciated a comment explaining what T is.
|
My feeling is that we should land this PR -- it seems like it contains many clear improvements and bug fixes, and it's big enough that we could keep iterating forever with nitpicks. I'd rather land it and then address further problems in follow-up PRs. @arielb1 -- thoughts? |
|
I'll go for that @bors r+ |
|
📌 Commit df5c116 has been approved by |
Fixes to mir dataflow Fixes to mir dataflow This collects a bunch of changes to `rustc_borrowck::borrowck::dataflow` (which others have pointed out should probably migrate to some crate that isn't tied to the borrow-checker -- but I have not attempted that here, especially since there are competing approaches to dataflow that we should also evaluate). These changes: 1. Provide a family of related analyses: MovingOutStatements (which is what the old AST-based dataflo computed), as well as MaybeInitialized, MaybeUninitalized, and DefinitelyInitialized. * (The last two are actually inverses of each other; we should pick one and drop the other.) 2. Fix bugs in the pre-existing analysis implementation, which was untested and thus some obvious bugs went unnoticed, which brings us to the third point: 3. Add a unit test infrastructure for the MIR dataflow analysis. * The tests work by adding a new intrinsic that is able to query the analysis state for a particular expression (technically, a particular L-value). * See the examples in compile-fail/mir-dataflow/inits-1.rs and compile-fail/mir-dataflow/uninits-1.rs * These tests are only checking the results for MaybeInitialized, MaybeUninitalized, and DefinitelyInitialized; I am not sure if it will be feasible to generalize this testing strategy to the MovingOutStatements dataflow operator.
Fixes to mir dataflow
This collects a bunch of changes to
rustc_borrowck::borrowck::dataflow(which others have pointed out should probably migrate to some crate that isn't tied to the borrow-checker -- but I have not attempted that here, especially since there are competing approaches to dataflow that we should also evaluate).These changes: