Allow constants to refer statics in const eval #71332
Conversation
rust-highfive
added
the
S-waiting-on-review
This relaxes the dynamic and validity checks so that constants can construct pointers to all statics and read from immutable statics.
rpjohnst
force-pushed
the
consts-can-refer-to-statics
branch
from
5e3b3c0
to
8f86833
Compare
|
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 |
| @@ -366,20 +366,6 @@ impl<'a, 'b, 'tcx> Visitor<'tcx> for TypeVerifier<'a, 'b, 'tcx> { | |||
| ); | |||
| } | |||
| } | |||
| } else if let Some(static_def_id) = constant.check_static_ptr(tcx) { | |||
There was a problem hiding this comment.
What is this diff doing here? This is changing the borrow checker, which should be unnecessary when all you want to do is change the dynamic const-to-static check.
There was a problem hiding this comment.
I guess I should have been clearer in the commit message. This check assumes that the only constants that can point to statics are the ones created and immediately dereferenced during MIR building, so it fails with anything else. (For example, a constant pointing to a static's field, which has a different type than the static itself.)
So the first commit of this PR replaces this check with a more precise one suggested (AFAIU) by @eddyb when it was added. I guess this could also make sense as a separate PR? Or at least some borrowck-oriented review?
There was a problem hiding this comment.
So without this, the rest of the PR fails, or so?
I certainly cannot review anything in borrw_check or mir_build, I have never touched or read any of that code.^^
There was a problem hiding this comment.
Yes, without this many scenarios using consts that refer to statics will fail at that span_mirbug!.
| @@ -94,10 +94,13 @@ pub(super) fn mk_eval_cx<'mir, 'tcx>( | |||
| ) | |||
| } | |||
|
|
|||
| pub(super) fn op_to_const<'tcx>( | |||
| #[derive(Debug)] | |||
| pub(super) struct RefersToStatic; | |||
There was a problem hiding this comment.
I don't think we need a new error type here. There are tons of errors that can happen in const-eval, why is this one so special that it has to shop up here? There is no precedent for anything like this.
There was a problem hiding this comment.
This is not actually an error. This is a signal that we should turn this into a ByRef constant instead of trying to use Scalar or Slice. At the very least it must be documented to explain this properly, but imo you an also just use Option as the return type of try_op_to_const
| @@ -368,16 +366,14 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter { | |||
| if memory_extra.can_access_statics { | |||
There was a problem hiding this comment.
This should be rename to can_access_mutable_globals or so.
| @@ -368,16 +366,14 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter { | |||
| if memory_extra.can_access_statics { | |||
| // Machine configuration allows us read from anything (e.g., `static` initializer). | |||
| Ok(()) | |||
| } else if static_def_id.is_some() { | |||
| // Machine configuration does not allow us to read statics | |||
| } else if allocation.mutability != Mutability::Not { | |||
There was a problem hiding this comment.
| } else if allocation.mutability != Mutability::Not { | |
| } else if allocation.mutability == Mutability::Mut { |
| @@ -411,11 +411,11 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M | |||
| if !did.is_local() || self.ecx.tcx.is_foreign_item(did) { | |||
| return Ok(()); | |||
| } | |||
| // FIXME: Statics referenced from consts are skipped. | |||
| // This avoids spurious "const accesses static" errors | |||
| // unrelated to validity, but is similarly unsound. | |||
There was a problem hiding this comment.
I'd merge this with the if above, to
if !did.is_local() || self.ecx.tcx.is_static(did) {The comment should also be updated:
// We don't re-validate statics because
// - the const-machine we are in might forbid reading from them, and
// - they have already been validated.
// FIXME: validation might have happened at a different type, but for now
// we consider this an okay trade-off.
There was a problem hiding this comment.
But your check is stricter in the sense that it validates more, so maybe @oli-obk would prefer that one.
I just think it's generally not worth recursing into other globals -- this can only possibly catch a bug if people did some crazy unsafe stuff, and those people asked for it when they get post-monomorphization errors. So I'd actually be fine to just always bail for any Some(GlobalAlloc::Static(_)) here.
There was a problem hiding this comment.
I went with the stricter check because there are tests for this, for statics-pointing-to-statics:
- https://github.com/rust-lang/rust/blob/master/src/test/ui/consts/const-eval/double_check.rs
- https://github.com/rust-lang/rust/blob/master/src/test/ui/consts/const-eval/double_check2.rs
Happy to collapse it into the first check if we're okay ditching those tests.
There was a problem hiding this comment.
Yeah I think it is okay to not catch the failure in double_check2. We already do not catch it cross-crate. But let's wait what @oli-obk thinks -- and in fact let's Cc @rust-lang/wg-const-eval and see what everyone else thinks :D
There was a problem hiding this comment.
wait, how do we not catch this cross crate? Don't we revisit statics from other crates if we encounter them at a different type of non-zero offset?
There was a problem hiding this comment.
No we don't, as far as I know.
| @@ -1,10 +1,10 @@ | |||
| // check-pass | |||
| // compile-flags: -Zunleash-the-miri-inside-of-you | |||
There was a problem hiding this comment.
This test should be moved to the miri-unleashed folder.
There was a problem hiding this comment.
Also it should then be adjusted to check that reading the static actually works. Like,
const TEST_VAL: u8 = *TEST;
fn main() { assert_eq!(TEST_VAL, 4); }
| // compile-flags: -Zunleash-the-miri-inside-of-you | ||
|
|
||
| #![allow(dead_code)] | ||
|
|
||
| const TEST: u8 = MY_STATIC; //~ ERROR any use of this value will cause an error |
There was a problem hiding this comment.
This, too, should get an assertion in main checking the value that was read.
Also Cc @ecstatic-morse as this is a const-prop test; I am not entirely sure what it is supposed to test and why it unleashes Miri so it is unclear how to adjust it.
There was a problem hiding this comment.
Didn't you do const-prop stuff? Or am I mixing that up with someone else?
There was a problem hiding this comment.
Oh I think I meant @wesleywiser, sorry for that!
There was a problem hiding this comment.
I think this was added just to document the current state when -Zunleash-the-miri-inside-of-you is enabled. This change seems fine to me. Asserting in main that the value was read like @RalfJung suggested also sounds like a good idea to me.
| LL | const DEREF_INTERIOR_MUT: usize = *REF_INTERIOR_MUT; | ||
| | ----------------------------------^^^^^^^^^^^^^^^^^- | ||
| | | | ||
| | constant accesses static |
There was a problem hiding this comment.
The error message should be reworded now, something like "constant reading from mutable static".
| LL | const READ_EXTERN: usize = unsafe { EXTERN }; | ||
| | ------------------------------------^^^^^^--- | ||
| | | | ||
| | cannot read from foreign (extern) static DefId(0:11 ~ const_refers_to_static[317d]::[0]::EXTERN[0]) |
There was a problem hiding this comment.
That error message doesn't look great.
| fn main() { | ||
| assert!(parse_slice(&DATA)); | ||
| assert!(parse_id(DATA.as_ptr())); | ||
| } |
There was a problem hiding this comment.
This doesn't actually read from the static in a const, does it?
Also I feel we are getting a lot of duplicate tests here, some of the ones above seem to already test the same thing?
There was a problem hiding this comment.
Hm, I added this test because it caught some ICEs that the others didn't- one related to the borrowck change and another related to the fallback-to-ByRef changes. Perhaps that code would also be exercised by some of your suggestions above? I'll look into it.
There was a problem hiding this comment.
If this caused extra ICEs, that seems fair then. Please add a comment in the test explaining that.
| let alloc_map = ecx.tcx.alloc_map.lock(); | ||
| let try_unwrap_memory = |alloc_id: AllocId| match alloc_map.get(alloc_id) { | ||
| Some(GlobalAlloc::Memory(mem)) => Ok(mem), | ||
| Some(GlobalAlloc::Static(_)) => Err(RefersToStatic), |
There was a problem hiding this comment.
This breaks our assumption, that any constant of scalar type or &str/&[u8] type is ConstValue::Scalar or ConstValue::Slice respectively. If such a constant is used in pattern matching, things will break in bad ways (e.g. two identical patterns not comparing equal). I guess such constants must be forbidden from occurring in patterns and const generics.
There was a problem hiding this comment.
Oh... pattern matching assumes that references in consts can only point to immutable memory? That's... a rather big assumption! It was true so far but only due to extreme conservatism on the side of our static const checks.
However, I feel like the fact that &str is an immutable shared reference should count for something. Maybe interning should ensure that shared references only point to read-only allocations, or so? (recursively)
There was a problem hiding this comment.
If the &str came from a &mut str, we can't turn the &mut str allocation into an immutable one. And if we allow constants to refer to statics, that can happen, and we must keep the pointer to the static, because otherwise we violate the fact that a pointer to the same static must have the same address.
There was a problem hiding this comment.
How'd that look like though? When a const takes an &mut to something, that reference has lifetime 'static, so nothing else ever in the entire program is allowed to access the memory behind that unique reference.
Though when we have &i32, that could indeed be from an &mut i32 that only covers part of an allocation and the rest of it could still permit mutation.
I think allowing references in consts in patterns was a big mistake... do we permit this for all types or just &str/&[u8]?
There was a problem hiding this comment.
I think allowing references in consts in patterns was a big mistake... do we permit this for all types or just &str/&[u8]?
Hold up :D we need special rules for pattern constants and const generics anyway. @eddyb talked to me about this already. E.g. we can't non-normalized padding or non-interned references in constants used in const generics. We do need a separate interning & normalization for such constants anyway. Not sure how we can detect references into statics though, maybe we need a new flag in Alloation that marks the allocation as part of a static, and thus having a unique address.
There was a problem hiding this comment.
I opened rust-lang/const-eval#42 for the soundness concerns.
We probably have to get that answered before allowing consts pointing to mutable statics in our dynamic checks. Sorry @rpjohnst, I was entirely unaware of this particular minefield.
For now, could you remove those changes from this PR? I.e., dynamically permit consts pointing to immutable statics, but make no effort to permit consts pointing to mutable statics?
There was a problem hiding this comment.
As long as we do it behind a feature gate, that's perfectly fine in my book.
There was a problem hiding this comment.
@rpjohnst's use-case doesn't need consts pointing to mutable statics though. I think this change is part of this PR because I recommended them to do that, because I thought we could weaken our dynamic checks all the way to "just prevent reads from mutable statics, everything else is fine".
I was wrong as I didn't know about pattern-matching-soundness. In light of this new information, I'd prefer it we didn't weaken our dynamic checks quite so far.
There was a problem hiding this comment.
Sure, will do. To clarify- do we want these dynamic checks behind a feature gate as well? Or can we rely on the static check and just feature gate any changes there?
There was a problem hiding this comment.
If you avoid touching validity (beyond what is necessary to enable consts to point to immutable statics), I don't think any feature gates are needed for weakening the dynamic checks.
|
☔ The latest upstream changes (presumably #71496) made this pull request unmergeable. Please resolve the merge conflicts. |
crlf0710
added
S-waiting-on-author
|
@rpjohnst Ping from triage, need a rebase here. |
|
This is blocked on deciding what to actually do, I can close it for now. |
This is the dynamic portion of what we discussed in rust-lang/const-eval#11.
Changes to the static check are left to a separate PR.
r? @oli-obk