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

Misc changes to StableMIR required to Kani use case. #117688

Merged
merged 6 commits into from Nov 17, 2023

Conversation

celinval
Copy link
Contributor

@celinval celinval commented Nov 7, 2023

First, I wanted to say that I can split this review into multiple if it makes reviewing easier. I bundled them up, since I've been testing them together (See rust-lang/project-stable-mir#51 for the set of more thorough checks).

So far, this review includes 3 commits:

  1. Add more APIs and fix Instance::body
    • Add more APIs to retrieve information about types.
    • Add a few more instance resolution options. For the drop shim, we return None if the drop body is empty. Not sure it will be enough.
    • Make Instance::body() return an Option, since not every instance might have an available body. For example, foreign instances, virtual instances, dependencies.
  2. Fix a bug on MIRVisitor
    • We were not iterating over all local variables due to a typo.
  3. Add more SMIR internal impl and callback return value
    • In cases like Kani, we will invoke the rustc_internal run command directly for now. It would be handly to be able to have a callback that can return a value.
    • We also need extra methods to convert stable constructs into internal ones, so we can break down the transition into finer grain commits.
    • For the internal implementation of Region, we're always returning ReErased for now.

@rustbot
Copy link
Collaborator

rustbot commented Nov 7, 2023

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

@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 Nov 7, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 7, 2023

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino, @ouz-a

@celinval
Copy link
Contributor Author

celinval commented Nov 7, 2023

r? spastorino

compiler/stable_mir/src/mir/mono.rs Outdated Show resolved Hide resolved
compiler/stable_mir/src/mir/mono.rs Outdated Show resolved Hide resolved
compiler/rustc_smir/src/rustc_smir/mod.rs Outdated Show resolved Hide resolved
@rustbot
Copy link
Collaborator

rustbot commented Nov 10, 2023

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino, @ouz-a

@celinval
Copy link
Contributor Author

@compiler-errors let me know if there's anything that you would like me to address. Also, should I reassign this PR for you, or should I wait for @spastorino's review?

@celinval celinval mentioned this pull request Nov 14, 2023
In cases like Kani, we will invoke the rustc_internal run command
directly for now. It would be handly to be able to have a callback
that can return a value.

We also need extra methods to convert stable constructs into internal
ones, so we can break down the transition into finer grain commits.
We were not iterating over all local variables due to a typo.
@compiler-errors
Copy link
Member

r=me after rebasing

Add more APIs to retrieve information about types, and add more instance
resolution options.

Make `Instance::body()` return an Option<Body>, since not every instance
might have an available body. For example, foreign instances, virtual
instances, dependencies.
We were previously skipping it for non-generic functions, but this was
leaving some constants unevaluated.
Fixed the `has_body()` function operator. Before that, this function was
returning false for all shims.

Change resolve_drop_in_place() to also return an instance for empty
shims, since they may still be required for vtable construction.
@celinval
Copy link
Contributor Author

@compiler-errors sorry to bother. I added one extra commit to fix 2 APIs. Do you want to take a look at it first?

@compiler-errors
Copy link
Member

I just read them

@bors r+

@bors
Copy link
Contributor

bors commented Nov 16, 2023

📌 Commit 8e81fc0 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 Nov 16, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 17, 2023
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#115476 (document ABI compatibility)
 - rust-lang#117688 (Misc changes to StableMIR required to Kani use case.)
 - rust-lang#117998 (On resolve error of `[rest..]`, suggest `[rest @ ..]`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c188486 into rust-lang:master Nov 17, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Nov 17, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 17, 2023
Rollup merge of rust-lang#117688 - celinval:smir-kani-reach, r=compiler-errors

Misc changes to StableMIR required to Kani use case.

First, I wanted to say that I can split this review into multiple if it makes reviewing easier. I bundled them up, since I've been testing them together (See rust-lang/project-stable-mir#51 for the set of more thorough checks).

So far, this review includes 3 commits:

1. Add more APIs and fix `Instance::body`
    - Add more APIs to retrieve information about types.
    - Add a few more instance resolution options. For the drop shim, we return None if the drop body is empty. Not sure it will be enough.
    - Make `Instance::body()` return an Option<Body>, since not every instance might have an available body. For example, foreign instances, virtual instances, dependencies.
2. Fix a bug on MIRVisitor
    - We were not iterating over all local variables due to a typo.
3. Add more SMIR internal impl and callback return value
    - In cases like Kani, we will invoke the rustc_internal run command directly for now. It would be handly to be able to have a callback that can return a value.
    - We also need extra methods to convert stable constructs into internal ones, so we can break down the transition into finer grain commits.
    - For the internal implementation of Region, we're always returning `ReErased` for now.
celinval added a commit to model-checking/kani that referenced this pull request Nov 24, 2023
We got a bit delayed on the nightly, so this PR closes the gap.

The related Rust changes were:
  - rust-lang/rust#117876
  - rust-lang/rust#117688
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.

None yet

7 participants