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

[MIR-borrowck] Two phase borrows #46537

Merged
merged 23 commits into from
Dec 15, 2017
Merged

Commits on Dec 13, 2017

  1. Revised graphviz rendering API to avoid requiring borrowed state.

    Made `do_dataflow` and related API `pub(crate)`.
    pnkfelix committed Dec 13, 2017
    Configuration menu
    Copy the full SHA
    93c4ffe View commit details
    Browse the repository at this point in the history
  2. Expanded HIR --unpretty hir,identified to include HIR local id.

    Having the HIR local id is useful for cases like understanding the
    ReScope identifiers, which are now derived from the HIR local id, and
    thus one can map an ReScope back to the HIR node, once one knows what
    those local ids are.
    pnkfelix committed Dec 13, 2017
    Configuration menu
    Copy the full SHA
    171c2ae View commit details
    Browse the repository at this point in the history
  3. Refactoring: pull bitvector initialization out from other parts of da…

    …taflow.
    
    This is meant to ease development of multi-stage dataflow analyses
    where the output from one analysis is used to initialize the state
    for the next; in such a context, you cannot start with `bottom_value`
    for all the bits.
    pnkfelix committed Dec 13, 2017
    Configuration menu
    Copy the full SHA
    d4add5d View commit details
    Browse the repository at this point in the history
  4. Refactoring: Allow BlockSets.on_entry to denote locally accumulated…

    … intrablock state.
    
    (Still musing about whether it could make sense to revise the design
    here to make these constraints on usage explicit.)
    pnkfelix committed Dec 13, 2017
    Configuration menu
    Copy the full SHA
    e123117 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    39e126c View commit details
    Browse the repository at this point in the history
  6. Configuration menu
    Copy the full SHA
    e437e49 View commit details
    Browse the repository at this point in the history
  7. Configuration menu
    Copy the full SHA
    ef64ace View commit details
    Browse the repository at this point in the history
  8. New ActiveBorrows dataflow for two-phase &mut; not yet borrowed-c…

    …hecked.
    
    High-level picture: The old `Borrows` analysis is now called
    `Reservations` (implemented as a newtype wrapper around `Borrows`);
    this continues to compute whether a `Rvalue::Ref` can reach a
    statement without an intervening `EndRegion`. In addition, we also
    track what `Place` each such `Rvalue::Ref` was immediately assigned
    to in a given borrow (yay for MIR-structural properties!).
    
    The new `ActiveBorrows` analysis then tracks the initial use of any of
    those assigned `Places` for a given borrow. I.e. a borrow becomes
    "active" immediately after it starts being "used" in some way. (This
    is conservative in the sense that we will treat a copy `x = y;` as a
    use of `y`; in principle one might further delay activation in such
    cases.)
    
    The new `ActiveBorrows` analysis needs to take the `Reservations`
    results as an initial input, because the reservation state influences
    the gen/kill sets for `ActiveBorrows`. In particular, a use of `a`
    activates a borrow `a = &b` if and only if there exists a path (in the
    control flow graph) from the borrow to that use. So we need to know if
    the borrow reaches a given use to know if it really gets a gen-bit or
    not.
    
     * Incorporating the output from one dataflow analysis into the input
       of another required more changes to the infrastructure than I had
       expected, and even after those changes, the resulting code is still
       a bit subtle.
    
     * In particular, Since we need to know the intrablock reservation
       state, we need to dynamically update a bitvector for the
       reservations as we are also trying to compute the gen/kills
       bitvector for the active borrows.
    
     * The way I ended up deciding to do this (after also toying with at
       least two other designs) is to put both the reservation state and
       the active borrow state into a single bitvector. That is why we now
       have separate (but related) `BorrowIndex` and
       `ReserveOrActivateIndex`: each borrow index maps to a pair of
       neighboring reservation and activation indexes.
    
    As noted above, these changes are solely adding the active borrows
    dataflow analysis (and updating the existing code to cope with the
    switch from `Borrows` to `Reservations`). The code to process the
    bitvector in the borrow checker currently just skips over all of the
    active borrow bits.
    
    But atop this commit, one *can* observe the analysis results by
    looking at the graphviz output, e.g. via
    
    ```rust
     #[rustc_mir(borrowck_graphviz_preflow="pre_two_phase.dot",
                 borrowck_graphviz_postflow="post_two_phase.dot")]
    ```
    
    Includes doc for `FindPlaceUses`, as well as `Reservations` and
    `ActiveBorrows` structs, which are wrappers are the `Borrows` struct
    that dictate which flow analysis should be performed.
    pnkfelix committed Dec 13, 2017
    Configuration menu
    Copy the full SHA
    ced5a70 View commit details
    Browse the repository at this point in the history
  9. Configuration menu
    Copy the full SHA
    658ed79 View commit details
    Browse the repository at this point in the history
  10. Configuration menu
    Copy the full SHA
    1334638 View commit details
    Browse the repository at this point in the history
  11. Incorporate active-borrows dataflow into MIR borrow check, yielding

    two-phase `&mut`-borrow support.
    
    This (new) support sits under `-Z two-phase-borrows` debugflag.
    
    (Still needs tests. That's coming next.)
    pnkfelix committed Dec 13, 2017
    Configuration menu
    Copy the full SHA
    3621645 View commit details
    Browse the repository at this point in the history
  12. Configuration menu
    Copy the full SHA
    5f759a9 View commit details
    Browse the repository at this point in the history
  13. Configuration menu
    Copy the full SHA
    dbbec4d View commit details
    Browse the repository at this point in the history
  14. Configuration menu
    Copy the full SHA
    db5420b View commit details
    Browse the repository at this point in the history
  15. Configuration menu
    Copy the full SHA
    9cb92ac View commit details
    Browse the repository at this point in the history
  16. Configuration menu
    Copy the full SHA
    18aedf6 View commit details
    Browse the repository at this point in the history
  17. Check activation points as the place where mutable borrows become rel…

    …evant.
    
    Since we are now checking activation points, I removed one of the
    checks at the reservation point. (You can see the effect this had on
    two-phase-reservation-sharing-interference-2.rs)
    
    Also, since we now have checks at both the reservation point and the
    activation point, we sometimes would observe duplicate errors (since
    either one independently interferes with another mutable borrow).  To
    deal with this, I used a similar strategy to one used as discussed on
    issue rust-lang#45360: keep a set of errors reported (in this case for
    reservations), and then avoid doing the checks for the corresponding
    activations. (This does mean that some errors could get masked, namely
    for conflicting borrows that start after the reservation but still
    conflict with the activation, which is unchecked when there was an
    error for the reservation. But this seems like a reasonable price to
    pay.)
    pnkfelix committed Dec 13, 2017
    Configuration menu
    Copy the full SHA
    5cae7a0 View commit details
    Browse the repository at this point in the history

Commits on Dec 14, 2017

  1. Address review comment: use .get instead of indexing to cope w/ ter…

    …minators.
    
    (Same net effect as code from before; just cleaner way to get there.)
    pnkfelix committed Dec 14, 2017
    Configuration menu
    Copy the full SHA
    3c7d9ff View commit details
    Browse the repository at this point in the history
  2. After discussion with ariel, replacing a guard within kill_loans_out_…

    …of_scope_at_location.
    
    Instead we are "just" careful to invoke it (which sets up a bunch of kill bits)
    before we go into the code that sets up the gen bits.
    
    That way, when the gen bits are set up, they will override any
    previously set kill-bits for those reservations or activations.
    pnkfelix committed Dec 14, 2017
    Configuration menu
    Copy the full SHA
    f96777c View commit details
    Browse the repository at this point in the history
  3. Address review note: AccessErrorsReported meant to track whether er…

    …ror reported at *any* point in past.
    pnkfelix committed Dec 14, 2017
    Configuration menu
    Copy the full SHA
    b75248e View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    b0421fa View commit details
    Browse the repository at this point in the history
  5. Review feedback: Added test with control flow merge of two borrows "b…

    …efore activation"
    
    In reality the currently generated MIR has at least one of the activations
    in a copy that occurs before the merge. But still, good to have a test,
    in anticipation of that potentially changing...
    pnkfelix committed Dec 14, 2017
    Configuration menu
    Copy the full SHA
    d654cd3 View commit details
    Browse the repository at this point in the history
  6. Address review feedback: don't treat "first" activation special.

    Instead, filter out (non-)conflicts of activiations with themselves in
    the same manner that we filter out non-conflict between an activation
    and its reservation.
    pnkfelix committed Dec 14, 2017
    Configuration menu
    Copy the full SHA
    159037e View commit details
    Browse the repository at this point in the history