Make some types and methods related to Polonius + Miri public by willcrichton · Pull Request #134191

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation15 Commits2 Checks6 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

We have a tool, Aquascope, which uses Polonius and Miri to visualize the compile-time and run-time semantics of a Rust program. Changes in the last few months to both APIs have hidden away details we depend upon. This PR re-exposes some of those details, specifically:

Polonius:

BorrowSet and BorrowData are added to rustc_borrowck::consumers, and their fields are made pub instead of pub(crate). We need this to interpret the BorrowIndexes generated by Polonius.
BorrowSet::build is now pub. We need this because the borrowck API doesn't provide access to the BorrowSet constructed during checking.
PoloniusRegionVid is added to rustc_borrowck::consumers. We need this because it's also contained in the Polonius facts.

Miri:

InterpCx::local_to_op is now a special case of local_at_frame_to_op, which allows querying locals in any frame. We need this because we walk the whole stack at each step to collect the state of memory.
InterpCx::layout_of_local is now pub. We need this because we need to know the layout of every local at each step.

If these changes go against some design goal for keeping certain types private, please let me know so we can hash out a better solution. Additionally, if there's a better way to document that it's important that certain types stay public, also let me know. For example, BorrowSet was previously public but was hidden in 6676cec, breaking our build.

cc @RalfJung @nnethercote @gavinleroy

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 added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Dec 12, 2024

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

This comment has been minimized.

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

/// Place to which the borrow was stored
pub(crate) assigned_place: mir::Place<'tcx>,
pub assigned_place: mir::Place<'tcx>,

Choose a reason for hiding this comment

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

Do you really need read and write access to all these fields?

If read access suffices, maybe add public getters instead?

Choose a reason for hiding this comment

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

I don't need write access. I restored the pub(crate) visibility and added getters for BorrowSet and BorrowData.

Member

lqd commented

Dec 12, 2024

• Loading

Additionally, if there's a better way to document that it's important that certain types stay public, also let me know. For example, BorrowSet was previously public

Adding a comment to each def site of interest will help: the fact that they are public is not enough, neither is the fact that they're referenced from consumers. AFAICT this code is has no stability guarantee, so things can (and will) still break but a comment explaining why things are public can help lower the chances of accidental breakage. This is already done in multiple places.

The Miri subtree was changed

cc @rust-lang/miri

r=me on the interpret and Miri parts.
It's a bit odd that reading locals now works for all frames but writing only works for the current frame... but that's crucial, Place::Local doesn't even store the frame so it would be quite dangerous to try to write to a local from a different frame. Reading is fine since we return the Op which is valid independent of what the current frame is.

r=me on the borrowck/polonius parts

I think that's all the parts.

@bors r=RalfJung,lqd rollup

📌 Commit 4d5d470 has been approved by RalfJung,lqd

It is now in the queue for this repository.

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

Dec 13, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Dec 13, 2024

Make some types and methods related to Polonius + Miri public

Polonius:

BorrowSet and BorrowData are added to rustc_borrowck::consumers, and their fields are made pub instead of pub(crate). We need this to interpret the BorrowIndexes generated by Polonius.
BorrowSet::build is now pub. We need this because the borrowck API doesn't provide access to the BorrowSet constructed during checking.
PoloniusRegionVid is added to rustc_borrowck::consumers. We need this because it's also contained in the Polonius facts.

Miri:

InterpCx::local_to_op is now a special case of local_at_frame_to_op, which allows querying locals in any frame. We need this because we walk the whole stack at each step to collect the state of memory.
InterpCx::layout_of_local is now pub. We need this because we need to know the layout of every local at each step.

cc @RalfJung @nnethercote @gavinleroy

Zalathar added a commit to Zalathar/rust that referenced this pull request

Dec 14, 2024

Make some types and methods related to Polonius + Miri public

Polonius:

BorrowSet and BorrowData are added to rustc_borrowck::consumers, and their fields are made pub instead of pub(crate). We need this to interpret the BorrowIndexes generated by Polonius.
BorrowSet::build is now pub. We need this because the borrowck API doesn't provide access to the BorrowSet constructed during checking.
PoloniusRegionVid is added to rustc_borrowck::consumers. We need this because it's also contained in the Polonius facts.

Miri:

InterpCx::local_to_op is now a special case of local_at_frame_to_op, which allows querying locals in any frame. We need this because we walk the whole stack at each step to collect the state of memory.
InterpCx::layout_of_local is now pub. We need this because we need to know the layout of every local at each step.

cc @RalfJung @nnethercote @gavinleroy

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 14, 2024

…iaskrgr

Rollup of 6 pull requests

Successful merges:

rust-lang#133221 (Add external macros specific diagnostics for check-cfg)
rust-lang#133386 (Update linux_musl base to dynamically link the crt by default)
rust-lang#134191 (Make some types and methods related to Polonius + Miri public)
rust-lang#134227 (Update wasi-sdk used to build WASI targets)
rust-lang#134279 ((Re-)return adjustment target if adjust kind is never-to-any)
rust-lang#134295 (Encode coroutine-closures in SMIR)

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 14, 2024

…iaskrgr

Rollup of 6 pull requests

Successful merges:

rust-lang#133221 (Add external macros specific diagnostics for check-cfg)
rust-lang#133386 (Update linux_musl base to dynamically link the crt by default)
rust-lang#134191 (Make some types and methods related to Polonius + Miri public)
rust-lang#134227 (Update wasi-sdk used to build WASI targets)
rust-lang#134279 ((Re-)return adjustment target if adjust kind is never-to-any)
rust-lang#134295 (Encode coroutine-closures in SMIR)

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Dec 14, 2024

Rollup merge of rust-lang#134191 - willcrichton:dev, r=RalfJung,lqd

Make some types and methods related to Polonius + Miri public

Polonius:

BorrowSet and BorrowData are added to rustc_borrowck::consumers, and their fields are made pub instead of pub(crate). We need this to interpret the BorrowIndexes generated by Polonius.
BorrowSet::build is now pub. We need this because the borrowck API doesn't provide access to the BorrowSet constructed during checking.
PoloniusRegionVid is added to rustc_borrowck::consumers. We need this because it's also contained in the Polonius facts.

Miri:

InterpCx::local_to_op is now a special case of local_at_frame_to_op, which allows querying locals in any frame. We need this because we walk the whole stack at each step to collect the state of memory.
InterpCx::layout_of_local is now pub. We need this because we need to know the layout of every local at each step.

cc @RalfJung @nnethercote @gavinleroy

Labels

S-waiting-on-bors

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

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

Make some types and methods related to Polonius + Miri public by willcrichton · Pull Request #134191 · rust-lang/rust (original) (raw)

Conversation

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment