Skip to content

Conversation

@willcrichton
Copy link
Contributor

For my static analysis tool, I need to access the set of outlives-constraints. Recently, #86977 merged a way to access these facts via Polonius. However, the merged implementation requires -Z polonius to be provided to use this feature. This uses Polonius for borrow checking on the entire crate, which as described here, is very slow.

This PR allows get_body_with_borrowck_facts to be called without -Z polonius. This is essential for my tool to run in a sensible length of time. This is a temporary patch as the Polonius-related APIs develop -- I can update my code as future changes happen.

Additionally, this PR also makes public two APIs that were previously public but then became private after rustc_mir got broken up: rustc_mir_dataflow::framework::graphviz and rustc_mir_transform::MirPass. I need both of these for my analysis tool. (I can break this change into a separate PR if necessary.)

@rust-highfive
Copy link
Contributor

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2021
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Sep 15, 2021

r? @ecstatic-morse
cc @lqd

Could you squash this and remove the unrelated changes?

edit: Ah, I see why you need the re-exports now. That's fine.

Copy link
Contributor

@ecstatic-morse ecstatic-morse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but let's wait for @lqd to concur.

@lqd
Copy link
Member

lqd commented Sep 15, 2021

LGTM, but let's wait for @lqd to concur.

I concur !

Additionally, this PR also makes public two APIs that were previously public but then became private after rustc_mir got broken up: rustc_mir_dataflow::framework::graphviz and rustc_mir_transform::MirPass. I need both of these for my analysis tool. (I can break this change into a separate PR if necessary.)

No need to put the change in a separate PR, however pulling it into a separate dedicated commit would be nice.

r=us or either of us, with the comment added, the polonius-related commits squashed, and the API-related changes pulled into a commit of their own.

@willcrichton willcrichton force-pushed the allow-single-polonius-call branch from edeee75 to 4f37150 Compare September 15, 2021 18:42
@willcrichton willcrichton force-pushed the allow-single-polonius-call branch from 4f37150 to 47104a3 Compare September 15, 2021 18:45
@willcrichton
Copy link
Contributor Author

@ecstatic-morse @lqd I've updated the history and commits to address your comments.

/// the body because it needs to regenerate the region identifiers.
/// the body because it needs to regenerate the region identifiers. This function
/// should never be invoked during a typical compilation session due to performance
/// issues with Polonius.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tough, but fair.

@ecstatic-morse
Copy link
Contributor

Thanks!

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Sep 15, 2021

📌 Commit 47104a3 has been approved by ecstatic-morse

@bors
Copy link
Collaborator

bors commented Sep 15, 2021

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 15, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 16, 2021
…call, r=ecstatic-morse

Allow calling `get_body_with_borrowck_facts` without `-Z polonius`

For my [static analysis tool](https://github.com/willcrichton/flowistry), I need to access the set of outlives-constraints. Recently, rust-lang#86977 merged a way to access these facts via Polonius. However, the merged implementation requires `-Z polonius` to be provided to use this feature. This uses Polonius for borrow checking on the entire crate, which as described [here](https://rust-lang.zulipchat.com/#narrow/stream/186049-t-compiler.2Fwg-polonius/topic/Polonius.20performance.20in.20a.20rustc.20plugin/near/251301631), is very slow.

This PR allows `get_body_with_borrowck_facts` to be called without `-Z polonius`. This is essential for my tool to run in a sensible length of time. This is a temporary patch as the Polonius-related APIs develop -- I can update my code as future changes happen.

Additionally, this PR also makes public two APIs that were previously public but then became private after `rustc_mir` got broken up: `rustc_mir_dataflow::framework::graphviz` and `rustc_mir_transform::MirPass`. I need both of these for my analysis tool. (I can break this change into a separate PR if necessary.)
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 16, 2021
…call, r=ecstatic-morse

Allow calling `get_body_with_borrowck_facts` without `-Z polonius`

For my [static analysis tool](https://github.com/willcrichton/flowistry), I need to access the set of outlives-constraints. Recently, rust-lang#86977 merged a way to access these facts via Polonius. However, the merged implementation requires `-Z polonius` to be provided to use this feature. This uses Polonius for borrow checking on the entire crate, which as described [here](https://rust-lang.zulipchat.com/#narrow/stream/186049-t-compiler.2Fwg-polonius/topic/Polonius.20performance.20in.20a.20rustc.20plugin/near/251301631), is very slow.

This PR allows `get_body_with_borrowck_facts` to be called without `-Z polonius`. This is essential for my tool to run in a sensible length of time. This is a temporary patch as the Polonius-related APIs develop -- I can update my code as future changes happen.

Additionally, this PR also makes public two APIs that were previously public but then became private after `rustc_mir` got broken up: `rustc_mir_dataflow::framework::graphviz` and `rustc_mir_transform::MirPass`. I need both of these for my analysis tool. (I can break this change into a separate PR if necessary.)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 16, 2021
…arth

Rollup of 10 pull requests

Successful merges:

 - rust-lang#88292 (Enable --generate-link-to-definition for rustc's docs)
 - rust-lang#88729 (Recover from `Foo(a: 1, b: 2)`)
 - rust-lang#88875 (cleanup(rustc_trait_selection): remove vestigial code from rustc_on_unimplemented)
 - rust-lang#88892 (Move object safety suggestions to the end of the error)
 - rust-lang#88928 (Document the closure arguments for `reduce`.)
 - rust-lang#88976 (Clean up and add doc comments for CStr)
 - rust-lang#88983 (Allow calling `get_body_with_borrowck_facts` without `-Z polonius`)
 - rust-lang#88985 (Update clobber_abi list to include k[1-7] regs)
 - rust-lang#88986 (Update the backtrace crate)
 - rust-lang#89009 (Fix typo in `break` docs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4601a71 into rust-lang:master Sep 16, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 16, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 25, 2022
…facts-flag, r=ecstatic-morse

Populate liveness facts when calling `get_body_with_borrowck_facts` without `-Z polonius`

For a new feature of [Flowistry](https://github.com/willcrichton/flowistry), a static-analysis tool, we need to obtain a `mir::Body`'s liveness facts using `get_body_with_borrowck_facts` (added in rust-lang#86977). We'd like to do this without passing `-Z polonius` as a compiler arg to avoid borrow checking the entire crate.

Support for doing this was added in rust-lang#88983, but the Polonius input facts used for liveness analysis are empty. This happens because the liveness input facts are populated in `liveness::generate` depending only on the value of `AllFacts::enabled` (which is toggled via compiler args).

This PR propagates the [`use_polonius`](https://github.com/rust-lang/rust/blob/8b09ba6a5d5c644fe0f1c27c7f9c80b334241707/compiler/rustc_borrowck/src/nll.rs#L168) flag to `liveness::generate` to support populating liveness facts without requiring the `-Z polonius` flag.

This fix is somewhat patchy - if it'd be better to add more widely-accessible state (like `AllFacts::enabled`) I'd be open to ideas!
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 25, 2022
…facts-flag, r=ecstatic-morse

Populate liveness facts when calling `get_body_with_borrowck_facts` without `-Z polonius`

For a new feature of [Flowistry](https://github.com/willcrichton/flowistry), a static-analysis tool, we need to obtain a `mir::Body`'s liveness facts using `get_body_with_borrowck_facts` (added in rust-lang#86977). We'd like to do this without passing `-Z polonius` as a compiler arg to avoid borrow checking the entire crate.

Support for doing this was added in rust-lang#88983, but the Polonius input facts used for liveness analysis are empty. This happens because the liveness input facts are populated in `liveness::generate` depending only on the value of `AllFacts::enabled` (which is toggled via compiler args).

This PR propagates the [`use_polonius`](https://github.com/rust-lang/rust/blob/8b09ba6a5d5c644fe0f1c27c7f9c80b334241707/compiler/rustc_borrowck/src/nll.rs#L168) flag to `liveness::generate` to support populating liveness facts without requiring the `-Z polonius` flag.

This fix is somewhat patchy - if it'd be better to add more widely-accessible state (like `AllFacts::enabled`) I'd be open to ideas!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants