Add a feature gate for allowing integers as the address of references in constants #72042
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @RalfJung |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
| if let Err(InterpErrorInfo { kind: err_unsup!(ReadBytesAsPointer), ..}) = ptr { | ||
| if self.ecx.tcx.features().const_int_ref { |
There was a problem hiding this comment.
Wow what a hack. This needs way more comments (more than "no explanation at all", anyway. ;)
But I guess the goal here actually is to allow pointers that CTFE would consider dangling, so no surprise I guess.
There was a problem hiding this comment.
Oh also, feature gates shouldn't affect what we do in Miri, so please make this conditional CTFE-only.
There was a problem hiding this comment.
please make this conditional CTFE-only.
On this note, do we have tests in Miri that make sure we still catch dangling integer references (and fn pointers)?
| if self.ecx.tcx.features().const_int_ref { | ||
| return Ok(()); | ||
| } | ||
| } | ||
| // Direct call to `check_ptr_access_align` checks alignment even on CTFE machines. |
There was a problem hiding this comment.
Please move this comment above the check_ptr_access_align call again.
| @@ -405,7 +411,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' | |||
| err_ub!(PointerOutOfBounds { .. }) => | |||
| { "a dangling {} (going beyond the bounds of its allocation)", kind }, | |||
| err_unsup!(ReadBytesAsPointer) => | |||
| { "a dangling {} (created from integer)", kind }, | |||
| { "a dangling {} (created from integer). Add `#![feature(const_int_ref)]` to the crate attributes to enable", kind }, | |||
There was a problem hiding this comment.
We shouldn't show this feature gate help on stable...
|
|
||
| error: aborting due to previous error | ||
|
|
||
| For more information about this error, try `rustc --explain E0601`. |
There was a problem hiding this comment.
This file shouldn't exist, right? It doesn't belong to any revision.
There was a problem hiding this comment.
hmm... I thought tidy warns us about such files. Yea this is leftover from a previous non-rev version
There was a problem hiding this comment.
I think tidy doesn't understand revisions, it warns only about entirely stale stderr files.
| const GPIO: &i32 = unsafe { Transmute { int: 0x800 }.ptr }; | ||
| //[vanilla]~^ ERROR it is undefined behavior | ||
|
|
||
| fn main() {} |
There was a problem hiding this comment.
Please add a test making sure we still catch NULL and unaligned references.
| @@ -565,6 +565,9 @@ declare_features! ( | |||
| /// Allow conditional compilation depending on rust version | |||
| (active, cfg_version, "1.45.0", Some(64796), None), | |||
|
|
|||
| /// Allows constants of reference type to have integers for the address. | |||
| (active, const_int_ref, "1.45.0", Some(63197), None), | |||
There was a problem hiding this comment.
Shouldn't there be a dedicated tracking issue? Not sure what the usual process is.
There was a problem hiding this comment.
We can change that issue to a tracking issue
There was a problem hiding this comment.
Reading that issue, it contains so much unrelated discussion, I'd prefer a new clean tracking issue.
|
This doesn't sound like there is lang team consensus for this so far. But maybe nightly-only experimentation is okay? |
|
Hmm yea, I read that comment differently (finding it was what prompted me to write this PR). I think the best way forward with either interpretation is to have a nightly feature for it so we can show what's going on. Especially considering the small change necessary, making it trivial to remove the unstable feature again, I think we should definitely create a feature gate and then prompt T-lang again for discussion of the feature. I can create a writeup with examples that run on the playground once this PR is merged. |
Makes sense.
If you do, we'll need to start collecting concerns in the main issue so we have them centralized. Not sure if editing the OP or creating a new issue is better. My main concern is: The "obvious" alternative is to just use raw pointers, so the main question is -- is not having that This is briefly mentioned here, and the response makes me think this is a raw ptr ergonomics issue and maybe relaxing reference checks isn't the best solution. |
| err_unsup!(ReadBytesAsPointer) => | ||
| { "a dangling {} (created from integer). Add `#![feature(const_int_ref)]` to the crate attributes to enable", kind }, | ||
| err_unsup!(ReadBytesAsPointer) => { | ||
| "a dangling {} (created from integer).{}", |
There was a problem hiding this comment.
The . should also be moved to the nightly-only part.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
joelpalmer
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #71718) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing after #63197 (comment) has convinced me that this is the wrong thing to do and we should instead suggest raw pointers once raw borrows (to get pointers to fields from raw pointer pointees) are stable and thus a viable alternative. |
implementation for #63197
cc @rust-lang/wg-const-eval