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

Make some types and methods related to Polonius + Miri public #134191

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

willcrichton
Copy link
Contributor

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

@rustbot
Copy link
Collaborator

rustbot commented Dec 12, 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 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
@rustbot
Copy link
Collaborator

rustbot commented 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

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented 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

/// OpTy from a local.
pub fn local_to_op(
pub fn local_at_frame_to_op(
Copy link
Member

Choose a reason for hiding this comment

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

This calls after_local_read which has no way of knowing that the local is from a different frame... so things might go wrong in Miri here.

Please extend after_local_read to also receive the &Frame.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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

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?

Copy link
Contributor Author

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.

@lqd
Copy link
Member

lqd commented Dec 12, 2024

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.

@rustbot
Copy link
Collaborator

rustbot commented Dec 12, 2024

The Miri subtree was changed

cc @rust-lang/miri

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants