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

interp: pass TyCtxt to Machine methods that do not take InterpCx #95693

Merged
merged 1 commit into from
Apr 6, 2022

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 5, 2022

This just seems like something you might need, so let's consistently have it.

One day we might have to add ParamEnv as well, though that seems less likely (and in Miri you can always use reveal_all anyway). It might make sense to have a type that packages TyCtxt and ParamEnv, this pairing occurs quite frequently in rustc...

r? @oli-obk

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2022
@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Apr 5, 2022

Uh... without miri or CTFE needing it, I think we should not add it. The only thing this gains us is changing the body of such a function to use tcx in CTFE without having to bump miri.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 5, 2022

It also gains us being able to start using tcx in Miri without having to go through a rustc bump (which is a big hurdle for most contributors).

I thought it might be needed for rust-lang/miri#2030 but @saethlin might know better; maybe having access to the Machine (and therefore the current call stack) is enough for that.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 5, 2022

Ah ok, well, experiments on the miri side is enough motivation for me, that just wasn't obvious to me

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 5, 2022

📌 Commit fcdfc3e has been approved by oli-obk

@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 Apr 5, 2022
@saethlin
Copy link
Member

saethlin commented Apr 5, 2022

It occurred to me when I finished my first pass at rust-lang/miri#2030 that Stacked Borrows errors in that PR are substantially more helpful than a use-after-free error. I looked at what would be required to do the similar sort of thing for use after free, and it wasn't clear to me that the required context was available at the site where the error was being discovered in order to craft the better diagnostic.

I don't know if this PR helps that situation, just some thoughts.

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2022
Rollup of 4 pull requests

Successful merges:

 - rust-lang#95659 (Rely on #[link] attribute for unwind on Fuchsia.)
 - rust-lang#95684 (rustdoc: Fix item info display overflow)
 - rust-lang#95693 (interp: pass TyCtxt to Machine methods that do not take InterpCx)
 - rust-lang#95699 (fix: Vec leak when capacity is 0)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit acdba55 into rust-lang:master Apr 6, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 6, 2022
@RalfJung RalfJung deleted the more-context branch April 6, 2022 18:38
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. 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