Dynamic checks for all const soundness concerns?

[RalfJung]

With rust-lang/rust#56123, we can bypass const qualification, so we can e.g. write constants that read from interior mutable statics, and probably other things I forgot to think about.

Could we

• implement all these restrictions as dynamic checks (during the miri execution, similar to CTFE: dynamically make sure we do not call non-const-fn rust#56007)
• and collect in this repo what these constraints are?

Ideally, these restrictions would be sufficient, so the soundness criterion for the static checks could be fully described as "statically check for the absence of the following miri errors" -- and such that @oli-obk's feature would actually be sound!

For example, I could imagine tweaking the CTFE instance of miri such that reading from a static is disallowed (and not just disallowing writing to a static like now), implementing a dynamic check that would reject code like what I described above.

For the UnsafeCell and Drop checks, we'd need some value-based analysis, which @oli-obk wanted in the long run anyway.

Checklist:

• Add test for checking that we reject drop calls during initializer evaluation.
• Guard against accessing statics when evaluating const initializer (+ test). (fixed by Ensure that evaluating or validating a constant never reads from a static rust#67337)
• No new mutable state via consts (fixed by Ensure that evaluating or validating a constant never reads from a static rust#67337)
• miri-unleash test for: accessing a thread-local static
• miri-unleash test for: evaluating inline assembly
• Something for Sync? See Constants can contain references that are not Sync rust#49206.

The problem with promotion is tracked separately at #53.

Cc @eddyb

[RalfJung]

Recently @eddyb said something like "turns out Miri catches errors in const qualif, that's good news" -- basically referring to this issue. I take this as confirmation that this is a goal worth pursuing.

With rust-lang/rust#58351, a bunch of "holes" in these checks are fixed. But I think I need help to figure out what is left. Here is an incomplete list, with examples that we could add as -Zunleash-miri tests:

• We have to check that no destructors are called. I think we already do, through the "no non-const-fn-call" check, but we should add a test case for this.

• We have to check that const/promoteds do not access statics.

  How would we implement this? We could tell CTFE whether it is currently evaluating a static or other initializer, and then we could reject reads/writes to/from "global" (already interned) allocations that are marked mutable. That would actually correspond to the more refined analysis that does allow the above code, but does not allow the variant with added interior mutability. Intuitively, if only immutable memory has been read, then clearly the end result must be the same no matter when evaluation happens.

  use std::cell::Cell;
  
  static STATIC: usize = 42;
  static mut STATIC_MUT: usize = 42;
  static mut STATIC_CELL: Cell<usize> = Cell::new(42);
  
  const SCARY_BUT_OKAY: usize = STATIC+1;
  const BAD_MUT: usize = unsafe { STATIC_MUT+1 };
  const BAD_CELL: usize = unsafe { *(&STATIC_CELL as *const _ as *const usize as *mut usize) +1 };

  @eddyb is there any deep reason to forbid SCARY_BUT_OKAY?

• We have to check that the final value of a const/promoted does not add any "new" mutable memory to the global state. I think we do some checks here already with @oli-obk's PR mentioned above, but do we check enough? What about raw pointers / other pointer values in consts?

  const BAD: &Cell<i32> = &Cell::new(42);
  // Inlining `BAD` everywhere clearly is not the same as them all pointing to the same thing.
  
  const BAD_RAW: *mut i32 = &Cell::new(42) as *const _ as *mut _;
  const BAD_USIZE: usize = &Cell::new(42) as *const _ as usize;

  It seems to me like we are actually fine with constants pointing to already existing mutable memory? After all, then evaluating that constant many time would just make it point to the same memory each time. So, conceivably we could accept something like

  static STATIC: Cell<i32> = Cell::new(42);
  const SCARY_BUT_OKAY: *mut i32 = &STATIC as *const _ as *mut _;
  const BAD: i32 = unsafe { *SCARY_BUT_OKAY };

  @eddyb is there anything inherently wrong with SCARY_BUT_OKAY, if we still forbid BAD?

• Can we do any dynamic check for non-Sync references in constants?

• For promoteds on top of the above, the result must not require dropping. Is there something we can check here?

Also, is this list complete? This mostly reflects what we have documented as required constraints in this repo. We already check that only const fn are called, and for promoteds most of the complexity is avoiding dynamic errors, not figuring out what the dynamic error conditions are.

[RalfJung]

Also Cc @ecstatic-morse

[eddyb]

@eddyb is there any deep reason to forbid SCARY_BUT_OKAY?

Not really, a frozen static is just a weird const (that also happens to be a place instead of a value).

@eddyb is there anything inherently wrong with SCARY_BUT_OKAY, if we still forbid BAD?

We used to track whether a value refers to a static, and it was a nightmare (it's also untenable given const fn or generic/associated consts). I don't think we care that much now since miri can just evaluate everything and produce an error if a static was attempted to be mutated.

For promoteds on top of the above, the result must not require dropping. Is there something we can check here?

We could maybe promote any Drop terminators we find (instead of removing them) and then the drop elaboration pass would turn them into noops - if they somehow aren't noops, miri would fail to evaluate the,.

[RalfJung]

We used to track whether a value refers to a static, and it was a nightmare

Yeah, and with more and more CTFE capabilities this will really not scale.
Right now we forbid even mentioning statics in consts, and that's an okay approximation, but the plan in this thread is to tease out what we have to do, so that we know what the design space is for relaxing the static check.

[vertexclique]

Count me in for finding out a solution for Sync. I want to implement that.

[RalfJung]

@vertexclique awesome! This first needs a bunch of research I think, as right now I do not even have a good idea for what the (dynamic) property is that we want to ensure. It's something about "Sync values", but I am not sure I know what that is.

[RalfJung]

I thought a bit more about promoteds here and I don't think it makes much sense to have a dynamic version of "promotion analysis". Or rather, we would have to actually run that code both in Miri and through LLVM codegen and compare results, or so -- that would require a considerable amount of infrastructure, much more than the dynamic checks we otherwise added.

This document otherwise describes the key concerns here pretty well.

[RalfJung]

I just learned about another restriction that const-checking enforces that I had no idea about: accesses to thread-local statics are rejected. This makes a lot of sense (Cc rust-lang/rust#70685), but I am slightly worried that this was never added to the docs here, nor is there (I think) an unleash-miri test for this. Is there anything else we missed? Cc @ecstatic-morse

I noticed the IS_SUPPORTED_IN_MIRI flag in https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/check_consts/ops.rs -- what is the effect of that flag (its doc comment doesn't say)? On first sight, I would say that whenever IS_SUPPORTED_IN_MIRI = false then there must be miri-unleash tests, but maybe that is naive.

[RalfJung]

Oh looks like IS_SUPPORTED_IN_MIRI = false means even with miri-unleashed we still reject this feature... that's odd, why would miri-unleash not unleash thread-locals, or heap allocations?

[oli-obk]

The reason is that @ecstatic-morse ported my if unleash branches 1:1 into the new system, assuming I had reasonably vetted the previous logic. That was not the case, I just added such branches whereever I needed them for tests. We should be able to remove that constant entirely and just let miri handle it (or not)

[ecstatic-morse]

There are some operations like inline assembly that will never be supported by MIRI. I suppose you might want -Zunleash-the-miri-inside-you to try to evaluate these cases anyways. If that's the case then we can indeed remove the flag.

Side note: we don't actually check for inline assembly since it wasn't in the old pass. It currently results in post-monomorphization errors. I fixed this in the PR that removed qualify_min_const_fn, and should separate that fix out.

[RalfJung]

The reason is that @ecstatic-morse ported my if unleash branches 1:1 into the new system, assuming I had reasonably vetted the previous logic. That was not the case, I just added such branches whereever I needed them for tests.

Ah and the new code is much easier to audit so now such issues are much more obvious. That's great. :)

We should be able to remove that constant entirely and just let miri handle it (or not)

Fully agreed.

There are some operations like inline assembly that will never be supported by MIRI. I suppose you might want -Zunleash-the-miri-inside-you to try to evaluate these cases anyways. If that's the case then we can indeed remove the flag.

Indeed I do. I think that everything check_const does should be just a static approximation for checks that Miri already does dynamically, and make sure we get errors pre-monomorphization (plus gating features that we still consider unstable). That gives them a clear specification, and -- if we work on making them more and more precise through better analysis (like dataflow) or using types more or so -- a clear "limit" to reach for.

Side note: we don't actually check for inline assembly since it wasn't in the old pass. It currently results in post-monomorphization errors. I fixed this in the PR that removed qualify_min_const_fn, and should separate that fix out.

Nice catch!

So what is the list of such things that const_prop rejects that we do not yet have miri-unleash tests for? So far, I counted

• accessing thread-locals
• evaluating inline assembly

[RalfJung]

We should be able to remove that constant entirely and just let miri handle it (or not)

Current status: two uses of that constant got removed in rust-lang/rust#71276 and rust-lang/rust#71149. The last remaining use is for TLS, and either rust-lang/rust#71192 or a follow-up PR should remove that.

[RalfJung]

We should be able to remove that constant entirely and just let miri handle it (or not)

With rust-lang/rust#72893, the constant goes away.