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

Expand NLL MIR dumps #129711

Merged
merged 5 commits into from
Aug 31, 2024
Merged

Expand NLL MIR dumps #129711

merged 5 commits into from
Aug 31, 2024

Conversation

lqd
Copy link
Member

@lqd lqd commented Aug 28, 2024

This PR is a first step to clean up and expand NLL MIR dumps:

  • by restoring the "mir-include-spans" comments which are useful for -Zdump-mir=nll
  • by adding the list of borrows to NLL MIR dumps, where they are introduced in the CFG and in which region

Comments in MIR dumps were turned off in #112346, but as shown in #114652 they were still useful for us working with NLL MIR dumps. So this PR pulls -Z mir-include-spans into its own options struct, so that passes dumping MIR can override them if need be. The rest of the compiler is not affected, only the "nll" pass dumps have these comments enabled again. The CLI still has priority when specifying the flag, so that we can explicitly turn them off in the mir-opt tests to keep blessed dumps easier to work with (which was one of the points of #112346).

Then, as part of a couple steps to improve NLL/polonius MIR dumps and .dot visualizations, I've also added the list of borrows and where they're introduced. I'm doing all this to help debug some polonius scope issues in my prototype location-sensitive analysis :3. I'll probably add member constraints soon.

@rustbot
Copy link
Collaborator

rustbot commented Aug 28, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 28, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 28, 2024

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/compiletest

cc @jieyouxu

compiler/rustc_borrowck/src/nll.rs Outdated Show resolved Hide resolved
compiler/rustc_session/src/options.rs Outdated Show resolved Hide resolved
src/tools/compiletest/src/runtest.rs Outdated Show resolved Hide resolved
@compiler-errors
Copy link
Member

compiler-errors commented Aug 29, 2024

Also the commit history is a bit tediously granular here. pull out tcx probably doesn't need to be its own commit.

Would appreciate if you squashed this into actual significant pieces changed -- it doesn't help to know that you've separated a commit changing "it's" to "its" rather than just sneaking it into a commit lol

clarification: not saying this needs to be 1 commit though; this definitely has a few steps that are worth separating. there's just a lot of tiny nit commits peppered in between that seem unncessary :3

@lqd
Copy link
Member Author

lqd commented Aug 29, 2024

there's just a lot of tiny nit commits peppered in between that seem unncessary :3

Yeah that's true. I review per-commit and thus also tend to move unrelated commits so that they don't pollute the more important ones, not to focus attention to them. Here, it's too granular indeed. I've reworked the history, hopefully it's more to your liking.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

cool ty

@compiler-errors
Copy link
Member

r=me when green

@lqd
Copy link
Member Author

lqd commented Aug 29, 2024

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Aug 29, 2024

📌 Commit 42b1450 has been approved by compiler-errors

It is now in the queue for this repository.

@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 Aug 29, 2024
@bors

This comment was marked as resolved.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 30, 2024
initially starting with `-Z mir-include-spans` because we want them in
the NLL mir dump pass
We want to allow setting this on the CLI, override it only in MIR
passes, and disable it altogether in mir-opt tests.

The default value is "only for NLL MIR dumps", which is considered off
for all intents and purposes, except for `rustc_borrowck` when an NLL
MIR dump is requested.
explicitly disable `-Zmir-include-spans` in mir-opt tests

This will override the NLL default of true, and keep the blessed dumps
easier to work with.
@lqd
Copy link
Member Author

lqd commented Aug 30, 2024

Rebased to fix conflicts.

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Aug 30, 2024

📌 Commit dff3d35 has been approved by compiler-errors

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 30, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 31, 2024
Expand NLL MIR dumps

This PR is a first step to clean up and expand NLL MIR dumps:
- by restoring the "mir-include-spans" comments which are useful for `-Zdump-mir=nll`
- by adding the list of borrows to NLL MIR dumps, where they are introduced in the CFG and in which region

Comments in MIR dumps were turned off in rust-lang#112346, but as shown in rust-lang#114652 they were still useful for us working with NLL MIR dumps. So this PR pulls `-Z mir-include-spans` into its own options struct, so that passes dumping MIR can override them if need be. The rest of the compiler is not affected, only the "nll" pass dumps have these comments enabled again. The CLI still has priority when specifying the flag, so that we can explicitly turn them off in the `mir-opt` tests to keep blessed dumps easier to work with (which was one of the points of rust-lang#112346).

Then, as part of a couple steps to improve NLL/polonius MIR dumps and `.dot` visualizations, I've also added the list of borrows and where they're introduced. I'm doing all this to help debug some polonius scope issues in my prototype location-sensitive analysis :3. I'll probably add member constraints soon.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
 - rust-lang#129711 (Expand NLL MIR dumps)
 - rust-lang#129730 (f32 docs: define 'arithmetic' operations)
 - rust-lang#129733 (Subtree update of `rust-analyzer`)
 - rust-lang#129749 (llvm-wrapper: adapt for LLVM API changes)
 - rust-lang#129757 (Add a test for trait solver overflow in MIR inliner cycle detection)
 - rust-lang#129760 (Make the "detect-old-time" UI test more representative)
 - rust-lang#129767 (Remove `#[macro_use] extern crate tracing`, round 4)
 - rust-lang#129774 (Remove `#[macro_use] extern crate tracing` from rustdoc and rustfmt)
 - rust-lang#129785 (Miri subtree update)
 - rust-lang#129791 (mark joboet as on vacation)

Failed merges:

 - rust-lang#129777 (Add `unreachable_pub`, round 4)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2024
…iaskrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#129659 (const fn stability checking: also check declared language features)
 - rust-lang#129711 (Expand NLL MIR dumps)
 - rust-lang#129730 (f32 docs: define 'arithmetic' operations)
 - rust-lang#129733 (Subtree update of `rust-analyzer`)
 - rust-lang#129749 (llvm-wrapper: adapt for LLVM API changes)
 - rust-lang#129757 (Add a test for trait solver overflow in MIR inliner cycle detection)
 - rust-lang#129760 (Make the "detect-old-time" UI test more representative)
 - rust-lang#129767 (Remove `#[macro_use] extern crate tracing`, round 4)
 - rust-lang#129774 (Remove `#[macro_use] extern crate tracing` from rustdoc and rustfmt)
 - rust-lang#129785 (Miri subtree update)
 - rust-lang#129791 (mark joboet as on vacation)
 - rust-lang#129812 (interpret, codegen: tweak some comments and checks regarding Box with custom allocator)

Failed merges:

 - rust-lang#129777 (Add `unreachable_pub`, round 4)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2024
…iaskrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#129659 (const fn stability checking: also check declared language features)
 - rust-lang#129711 (Expand NLL MIR dumps)
 - rust-lang#129730 (f32 docs: define 'arithmetic' operations)
 - rust-lang#129733 (Subtree update of `rust-analyzer`)
 - rust-lang#129749 (llvm-wrapper: adapt for LLVM API changes)
 - rust-lang#129757 (Add a test for trait solver overflow in MIR inliner cycle detection)
 - rust-lang#129760 (Make the "detect-old-time" UI test more representative)
 - rust-lang#129767 (Remove `#[macro_use] extern crate tracing`, round 4)
 - rust-lang#129774 (Remove `#[macro_use] extern crate tracing` from rustdoc and rustfmt)
 - rust-lang#129785 (Miri subtree update)
 - rust-lang#129791 (mark joboet as on vacation)
 - rust-lang#129812 (interpret, codegen: tweak some comments and checks regarding Box with custom allocator)

Failed merges:

 - rust-lang#129777 (Add `unreachable_pub`, round 4)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1c51e5b into rust-lang:master Aug 31, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 31, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2024
Rollup merge of rust-lang#129711 - lqd:nll-mir-dumps, r=compiler-errors

Expand NLL MIR dumps

This PR is a first step to clean up and expand NLL MIR dumps:
- by restoring the "mir-include-spans" comments which are useful for `-Zdump-mir=nll`
- by adding the list of borrows to NLL MIR dumps, where they are introduced in the CFG and in which region

Comments in MIR dumps were turned off in rust-lang#112346, but as shown in rust-lang#114652 they were still useful for us working with NLL MIR dumps. So this PR pulls `-Z mir-include-spans` into its own options struct, so that passes dumping MIR can override them if need be. The rest of the compiler is not affected, only the "nll" pass dumps have these comments enabled again. The CLI still has priority when specifying the flag, so that we can explicitly turn them off in the `mir-opt` tests to keep blessed dumps easier to work with (which was one of the points of rust-lang#112346).

Then, as part of a couple steps to improve NLL/polonius MIR dumps and `.dot` visualizations, I've also added the list of borrows and where they're introduced. I'm doing all this to help debug some polonius scope issues in my prototype location-sensitive analysis :3. I'll probably add member constraints soon.
@lqd lqd deleted the nll-mir-dumps branch August 31, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants