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 retagging work even with 'unstable' places #105317

Merged
merged 2 commits into from
Dec 9, 2022

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Dec 5, 2022

This is based on top of #105301. Only the last two commits are new.

While investigating rust-lang/unsafe-code-guidelines#381 I realized that we would have caught this issue much earlier if the add_retag pass wouldn't bail out on assignments of the form *ptr = ....

So this PR changes our retag strategy:

  • When a new reference is created via Rvalue::Ref (or a raw ptr via Rvalue::AddressOf), we do the retagging as part of just executing that address-taking operation.
  • For everything else, we still insert retags -- these retags basically serve to ensure that references stored in local variables (and their fields) are always freshly tagged, so skipping this for assignments like *ptr = ... is less egregious.
    r? @oli-obk

@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 5, 2022
@rustbot
Copy link
Collaborator

rustbot commented Dec 5, 2022

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

@oli-obk
Copy link
Contributor

oli-obk commented Dec 6, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Dec 6, 2022

📌 Commit 1b43d0083976a21cd31a5000ff346b564f232055 has been approved by oli-obk

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 Dec 6, 2022
@RalfJung
Copy link
Member Author

RalfJung commented Dec 6, 2022

I did a rebase, since #105301 landed.
@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Dec 6, 2022

📌 Commit 34c58e8 has been approved by oli-obk

It is now in the queue for this repository.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 8, 2022
make retagging work even with 'unstable' places

This is based on top of rust-lang#105301. Only the last two commits are new.

While investigating rust-lang/unsafe-code-guidelines#381 I realized that we would have caught this issue much earlier if the add_retag pass wouldn't bail out on assignments of the form `*ptr = ...`.

So this PR changes our retag strategy:
- When a new reference is created via `Rvalue::Ref` (or a raw ptr via `Rvalue::AddressOf`), we do the retagging as part of just executing that address-taking operation.
- For everything else, we still insert retags -- these retags basically serve to ensure that references stored in local variables (and their fields) are always freshly tagged, so skipping this for assignments like `*ptr = ...` is less egregious.
r? `@oli-obk`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 8, 2022
make retagging work even with 'unstable' places

This is based on top of rust-lang#105301. Only the last two commits are new.

While investigating rust-lang/unsafe-code-guidelines#381 I realized that we would have caught this issue much earlier if the add_retag pass wouldn't bail out on assignments of the form `*ptr = ...`.

So this PR changes our retag strategy:
- When a new reference is created via `Rvalue::Ref` (or a raw ptr via `Rvalue::AddressOf`), we do the retagging as part of just executing that address-taking operation.
- For everything else, we still insert retags -- these retags basically serve to ensure that references stored in local variables (and their fields) are always freshly tagged, so skipping this for assignments like `*ptr = ...` is less egregious.
r? ``@oli-obk``
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2022
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#104922 (Detect long types in E0308 and write them to disk)
 - rust-lang#105120 (kmc-solid: `std::sys` code maintenance)
 - rust-lang#105255 (Make nested RPIT inherit the parent opaque's generics.)
 - rust-lang#105317 (make retagging work even with 'unstable' places)
 - rust-lang#105405 (Stop passing -export-dynamic to wasm-ld.)
 - rust-lang#105408 (Add help for `#![feature(impl_trait_in_fn_trait_return)]`)
 - rust-lang#105423 (Use `Symbol` for the crate name instead of `String`/`str`)
 - rust-lang#105433 (CI: add missing line continuation marker)
 - rust-lang#105434 (Fix warning when libcore is compiled with no_fp_fmt_parse)
 - rust-lang#105441 (Remove `UnsafetyState`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f1f7560 into rust-lang:master Dec 9, 2022
@rustbot rustbot added this to the 1.67.0 milestone Dec 9, 2022
@RalfJung RalfJung deleted the retag-rework branch December 12, 2022 08:49
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
make retagging work even with 'unstable' places

This is based on top of rust-lang#105301. Only the last two commits are new.

While investigating rust-lang/unsafe-code-guidelines#381 I realized that we would have caught this issue much earlier if the add_retag pass wouldn't bail out on assignments of the form `*ptr = ...`.

So this PR changes our retag strategy:
- When a new reference is created via `Rvalue::Ref` (or a raw ptr via `Rvalue::AddressOf`), we do the retagging as part of just executing that address-taking operation.
- For everything else, we still insert retags -- these retags basically serve to ensure that references stored in local variables (and their fields) are always freshly tagged, so skipping this for assignments like `*ptr = ...` is less egregious.
r? ```@oli-obk```
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#104922 (Detect long types in E0308 and write them to disk)
 - rust-lang#105120 (kmc-solid: `std::sys` code maintenance)
 - rust-lang#105255 (Make nested RPIT inherit the parent opaque's generics.)
 - rust-lang#105317 (make retagging work even with 'unstable' places)
 - rust-lang#105405 (Stop passing -export-dynamic to wasm-ld.)
 - rust-lang#105408 (Add help for `#![feature(impl_trait_in_fn_trait_return)]`)
 - rust-lang#105423 (Use `Symbol` for the crate name instead of `String`/`str`)
 - rust-lang#105433 (CI: add missing line continuation marker)
 - rust-lang#105434 (Fix warning when libcore is compiled with no_fp_fmt_parse)
 - rust-lang#105441 (Remove `UnsafetyState`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 21, 2023
not *all* retags might be explicit in Runtime MIR

In rust-lang#105317 I made Miri treat `Rvalue::Ref/AddrOf` as implicit retagging sites. This updates the MIR docs accordingly.

For `Rvalue::Ref` I think this makes a lot more sense: creating a new reference is their entire point, so we can avoid bloating the MIR with retags. Also this seems to be the best way to handle cases like `*ptr = &[mut] ...`, where doing a retag is somewhat questionable since maybe `*ptr` points to another place now?

For `Rvalue::AddrOf`, Stacked Borrows needs this because even raw ptrs need some retagging, but Tree Borrows doesn't do ant retagging here and I hope we'll end up with a model where raw pointers don't get retagged.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 21, 2023
not *all* retags might be explicit in Runtime MIR

In rust-lang#105317 I made Miri treat `Rvalue::Ref/AddrOf` as implicit retagging sites. This updates the MIR docs accordingly.

For `Rvalue::Ref` I think this makes a lot more sense: creating a new reference is their entire point, so we can avoid bloating the MIR with retags. Also this seems to be the best way to handle cases like `*ptr = &[mut] ...`, where doing a retag is somewhat questionable since maybe `*ptr` points to another place now?

For `Rvalue::AddrOf`, Stacked Borrows needs this because even raw ptrs need some retagging, but Tree Borrows doesn't do ant retagging here and I hope we'll end up with a model where raw pointers don't get retagged.
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Mar 21, 2023
not *all* retags might be explicit in Runtime MIR

In rust-lang#105317 I made Miri treat `Rvalue::Ref/AddrOf` as implicit retagging sites. This updates the MIR docs accordingly.

For `Rvalue::Ref` I think this makes a lot more sense: creating a new reference is their entire point, so we can avoid bloating the MIR with retags. Also this seems to be the best way to handle cases like `*ptr = &[mut] ...`, where doing a retag is somewhat questionable since maybe `*ptr` points to another place now?

For `Rvalue::AddrOf`, Stacked Borrows needs this because even raw ptrs need some retagging, but Tree Borrows doesn't do ant retagging here and I hope we'll end up with a model where raw pointers don't get retagged.
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Mar 21, 2023
not *all* retags might be explicit in Runtime MIR

In rust-lang#105317 I made Miri treat `Rvalue::Ref/AddrOf` as implicit retagging sites. This updates the MIR docs accordingly.

For `Rvalue::Ref` I think this makes a lot more sense: creating a new reference is their entire point, so we can avoid bloating the MIR with retags. Also this seems to be the best way to handle cases like `*ptr = &[mut] ...`, where doing a retag is somewhat questionable since maybe `*ptr` points to another place now?

For `Rvalue::AddrOf`, Stacked Borrows needs this because even raw ptrs need some retagging, but Tree Borrows doesn't do ant retagging here and I hope we'll end up with a model where raw pointers don't get retagged.
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Mar 21, 2023
not *all* retags might be explicit in Runtime MIR

In rust-lang#105317 I made Miri treat `Rvalue::Ref/AddrOf` as implicit retagging sites. This updates the MIR docs accordingly.

For `Rvalue::Ref` I think this makes a lot more sense: creating a new reference is their entire point, so we can avoid bloating the MIR with retags. Also this seems to be the best way to handle cases like `*ptr = &[mut] ...`, where doing a retag is somewhat questionable since maybe `*ptr` points to another place now?

For `Rvalue::AddrOf`, Stacked Borrows needs this because even raw ptrs need some retagging, but Tree Borrows doesn't do ant retagging here and I hope we'll end up with a model where raw pointers don't get retagged.
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.

4 participants