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

interpret: make read-pointer-as-bytes a CTFE-only error with extra information #101101

Merged
merged 5 commits into from
Aug 30, 2022

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Aug 27, 2022

Next step in the reaction to #99923. Also teaches Miri to implicitly strip provenance in more situations when transmuting pointers to integers, which fixes rust-lang/miri#2456.

Pointer-to-int transmutation during CTFE now produces a message like this:

   = help: this code performed an operation that depends on the underlying bytes representing a pointer
   = help: the absolute address of a pointer is not known at compile-time, so such operations are not supported

r? @oli-obk

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

rustbot commented Aug 27, 2022

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

and show some extra information when it happens in CTFE
Comment on lines +445 to +447
// We are *not* reading a pointer.
// If we can just ignore provenance, do exactly that.
if Prov::OFFSET_IS_ADDR {
Copy link
Member

@saethlin saethlin Aug 28, 2022

Choose a reason for hiding this comment

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

The fact that OFFSET_IS_ADDR also indicates whether we support plain reads from bytes with provenance seems like a logical jump. Even the docs for this const only say this:

If `false`, ptr-to-int casts are not supported. The offset *must* be relative in that case.

which is identifying ptr-to-int casts which someone familiar with Miri may be sure is different from a ptr-to-int transmute. So I think this jump from OFFSET_IS_ADDR to "provenance can be discarded" isn't (clearly) documented anywhere that's easy to find, and it would be nice if these branches on the const were more clear.

Can we have something like Prov::can_discard_provenance() or Prov::permits_ptr_int_transmute() then branch on that in these places? Such a function would also be a good place to explain the connection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this is really not about casts any more, it is about transmutation. I have updated the docs.

There is a 1:1 correlation between "we can just ignore provenance and get a meaningful result" and "the offset is a pointer is the absolute address": the reason we can strip provenance is that the pointer offset is the absolute address, so revealing the pointer bytes to the interpreted program is meaningful.

I have updated the comment, I hope it is more clear now.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 29, 2022

@bors r+

took some brain mangling to grok, even with the comments, but I don't think it can be improved, it's just a complex operation

@bors
Copy link
Contributor

bors commented Aug 29, 2022

📌 Commit c46e803 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 Aug 29, 2022
@RalfJung RalfJung changed the title interpret: make read-pointer-as-pointers a CTFE-only error with extra information interpret: make read-pointer-as-bytes a CTFE-only error with extra information Aug 29, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 30, 2022
…oli-obk

interpret: make read-pointer-as-bytes a CTFE-only error with extra information

Next step in the reaction to rust-lang#99923. Also teaches Miri to implicitly strip provenance in more situations when transmuting pointers to integers, which fixes rust-lang/miri#2456.

Pointer-to-int transmutation during CTFE now produces a message like this:
```
   = help: this code performed an operation that depends on the underlying bytes representing a pointer
   = help: the absolute address of a pointer is not known at compile-time, so such operations are not supported
```

r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 30, 2022
Rollup of 9 pull requests

Successful merges:

 - rust-lang#95376 (Add `vec::Drain{,Filter}::keep_rest`)
 - rust-lang#100092 (Fall back when relating two opaques by substs in MIR typeck)
 - rust-lang#101019 (Suggest returning closure as `impl Fn`)
 - rust-lang#101022 (Erase late bound regions before comparing types in `suggest_dereferences`)
 - rust-lang#101101 (interpret: make read-pointer-as-bytes a CTFE-only error with extra information)
 - rust-lang#101123 (Remove `register_attr` feature)
 - rust-lang#101175 (Don't --bless in pre-push hook)
 - rust-lang#101176 (rustdoc: remove unused CSS selectors for `.table-display`)
 - rust-lang#101180 (Add another MaybeUninit array test with const)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 81f3841 into rust-lang:master Aug 30, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 30, 2022
@RalfJung RalfJung deleted the read-pointer-as-bytes branch August 31, 2022 10:45
bors added a commit to rust-lang/miri that referenced this pull request Aug 31, 2022
Adjust for supporting more implicit ptr-to-int transmutation

This is the Miri side of rust-lang/rust#101101.
Fixes #2456.
bors added a commit to rust-lang/miri that referenced this pull request Aug 31, 2022
Adjust for supporting more implicit ptr-to-int transmutation

This is the Miri side of rust-lang/rust#101101.
Fixes #2456.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 9, 2022
…=pnkfelix

beta-backport of provenance-related CTFE changes

This is all part of dealing with rust-lang#99923.

The first commit backports the effects of rust-lang#101101. `@pnkfelix` asked for this and it turned out to be easy, so I think this is uncontroversial.

The second commit effectively repeats rust-lang#99965, which un-does the effects of rust-lang#97684 and therefore means rust-lang#99923 does not apply to the beta branch. I honestly don't think we should do this; the sentiment in rust-lang#99923 was that we should go ahead with the change but improve diagnostics. But `@pnkfelix` seemed to request such a change so I figured I would offer the option.

I'll be on vacation soon, so if you all decide to take the first commit only, then someone please just force-push to this branch and remove the 2nd commit.
celinval added a commit to celinval/kani-dev that referenced this pull request Sep 30, 2022
Fixes model-checking#1615

Relevant changes to rustc:
  - rust-lang/rust#101483: Change to intrinsics.
  - rust-lang/rust#94075: Change to niche opt.
  - rust-lang/rust#101101: Method rename.
celinval added a commit to celinval/kani-dev that referenced this pull request Sep 30, 2022
Fixes model-checking#1615

Relevant changes to rustc:
  - rust-lang/rust#101483: Change to intrinsics.
  - rust-lang/rust#94075: Change to niche opt.
  - rust-lang/rust#101101: Method rename.

Co-authored-by: Adrian Palacios <[email protected]>
celinval added a commit to model-checking/kani that referenced this pull request Oct 3, 2022
Fixes #1615

Relevant changes to rustc:
  - rust-lang/rust#101483: Change to intrinsics.
  - rust-lang/rust#94075: Change to niche opt.
  - rust-lang/rust#101101: Method rename.

Co-authored-by: Adrian Palacios <[email protected]>
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Oct 23, 2022
…oli-obk

interpret: make read-pointer-as-bytes a CTFE-only error with extra information

Next step in the reaction to rust-lang#99923. Also teaches Miri to implicitly strip provenance in more situations when transmuting pointers to integers, which fixes rust-lang/miri#2456.

Pointer-to-int transmutation during CTFE now produces a message like this:
```
   = help: this code performed an operation that depends on the underlying bytes representing a pointer
   = help: the absolute address of a pointer is not known at compile-time, so such operations are not supported
```

r? ``@oli-obk``
antoyo pushed a commit to antoyo/rust that referenced this pull request Jun 19, 2023
…oli-obk

interpret: make read-pointer-as-bytes a CTFE-only error with extra information

Next step in the reaction to rust-lang#99923. Also teaches Miri to implicitly strip provenance in more situations when transmuting pointers to integers, which fixes rust-lang/miri#2456.

Pointer-to-int transmutation during CTFE now produces a message like this:
```
   = help: this code performed an operation that depends on the underlying bytes representing a pointer
   = help: the absolute address of a pointer is not known at compile-time, so such operations are not supported
```

r? ``@oli-obk``
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.

Miri errors when transmuting part of a pointer to an integer
6 participants