Try to find cyclic data dependencies during const-checking #71526
Conversation
This was needed by an early version of dataflow-based const qualification where `QualifCursor` needed to return a full `BitSet` with the current state.
rust-highfive
added
the
S-waiting-on-review
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
|
This causes a new error for the case in #62189. I guess we should ignore statics in the initial version of this. Obviously, this would allow cycles like the following: static A: i32 = {
let p = &A;
*p
}; |
I don't think that's accurate. That PR solves the problem by making zero-sized accesses actually access the pointed-to data. It has nothing to do with optimizations, and IMO that is exactly the right fix for the problem: it makes it so that the dependency of a static on another is recorded in the query graph even if it is a ZST dependency. |
|
That's true. For context, this is an attempt to address #71330 (comment). I guess it wasn't "optimizing away" in the MIR transform sense that caused #71330, but an optimization in the interpreter. |
ecstatic-morse
force-pushed
the
const-check-cycle
branch
from
258b8ce
to
3608535
Compare
|
|
||
| // If a cyclic data dependency exists within a const initializer, try to find | ||
| // it during const-checking. This is important because MIR optimizations could | ||
| // eliminate a cycle before const-eval runs. See #71078 for an example of this. |
There was a problem hiding this comment.
#71078 isn't actually an example of this, as Ralf notes.
There was a problem hiding this comment.
Huh? It isn't? Or are you talking about a comment by Ralf from elsewhere but this PR?
|
☔ The latest upstream changes (presumably #71539) made this pull request unmergeable. Please resolve the merge conflicts. |
| // | ||
| // FIXME: This means we don't look for cycles involving associated constants, but we | ||
| // should handle fully monomorphized ones here at least. | ||
| if self.tcx.trait_of_item(def_id).is_none() { |
There was a problem hiding this comment.
You can invoke Instance::resolve here, which will either bail out or give you the correct assoc constant if it can be resolved with the information at hand.
Indeed, and thanks for the background link. Which optimizations do we run on MIR before const-evalauting it? Maybe it would be better to just not optimize that MIR. Also, what would be an example for an optimization turning a const-unsound item it a const-sound one? I can imagine those exist, but this discussion would be easier with an example. My hypothesis is that those are not actually soundness issues -- if the optimization can "mask" the failure, then I think it is actually perfectly fine to use that const (e.g., the user could have written the const in the optimized way to begin with, and that must be sound, too). |
|
The first example coming to my mind is a const reading from a mutable static, and discarding the result. Optimizations could eliminate the read. There is no bug here though, this is fine. If this fixes a hole in the dynamic checks, there should be a testcase -- and ideally a miri.unleashed test case to make sure that indeed there is no hole any more. |
All of them. We can't do this any other way except by duplicating all MIR, even in metadata. The query to obtain earlier MIR without optimizations has a |
Okay, makes sense. My other point remains though: the MIR optimizations I can think of, since they preserve behavior, do not actually make our dynamic checks unsound in any way. The bug with recursive statics was not a MIR optimizations, it was an oversight in the MIR engine where it failed to properly inform the query system about some of the dependencies (namely dependencies via ZST reads -- which are irrelevant in terms of just the underlying value, but can be relevant in terms of safety invariants). |
I agree |
|
Consider the following: const A: i32 = {
let _ = A;
4
};This currently fails to compile on stable but compiles successfully on nightly with #71330 applied. This is because MIR optimizations remove the useless load of
|
Okay so we are not talking about fixing possible correctness problems any more, but about future-compat concerns. That's a totally different game. And I agree, this is a future-compat hazard. I am not sure what to do about that though, it seems pretty inherent to the way our dynamic checks work. We could possibly pretend that mir-opt-level is 0 for all static and const bodies, to at least lessen the impact of this?
Note that this can only happen for cases where the static checks are missing things that the dynamic checks are properly catching. That helps to find places of possible concern here. Not sure how many such "gaps" we have, besides recursive-static-checking. We should keep this in mind in the future whenever we consider relaxing the static checks and relying on the dynamic checks instead. (I thought maybe panics or other dynamic post-monomorphization CTFE failures could be an issue, but it would be incorrect for an optimization to remove those.)
Relatedly, |
|
From my POV, the problem is that, while we want to assume that variable accesses are side-effect free during MIR optimization, this assumption is violated while executing initializers. At this stage, simple reads can result in a cycle error and ultimately compilation failure.
Initializers can call const A: usize = b();
const fn b() -> usize {
let _: usize = A;
42
} |
That sounds like a fair characterization.
Indeed, my comment was meant to point out that that scheme would not be water-proof. |
|
Could you give a high-level description of how this PR solves the problem? The description gives a lot of background but does not explain that part (or I missed it). |
|
It doesn't solve the problem. It does reduce its surface area by making things that used to cause cycle errors during const-checking prior to #71330 still cause cycle errors. These cycle errors were triggered incidentally while running the dataflow analysis that determines the set of qualifs in each local. We would call Cyclic data dependencies in initializers that involved statics, associated constants or went through a function call did not trigger a compile error because we use type-based qualification in all of those cases. Otherwise, #71078 would have caused a cycle error during const-checking when we tried to look up the qualifications in |
|
I see. I think this means we should have a miri-unleash test for cyclic ZST statics, to make sure that even if something like this lands, we still test #71140. |
I remember having talked about this with people during the move to using miri for CTFE. There were two options:
The assumption was that any optimization that would cause previously not const evaluable MIR to become const evaluable, would trigger cycles or cause other errors itself. Specifically for the cycle case shown here, shouldn't this already be happening, even for associated consts, since we now have the list of unevaluated constants in the MIR and evaluate them during codegen? We may have forgotten to evaluate them during ctfe though (cc @spastorino). If a MIR is never codegenned but is used in const eval, we should also evaluate the list of unevaluated constants. |
Ah, that's a good point! So maybe |
|
miri should get it automatically when we do it in |
|
Yes that's what I meant. We should probably have a shared method for this that is used by Miri and all codegen backends (Cc @bjorn3). Ideally we'd make it so that codegen backends cannot forget to call that method; I am not sure if that is possible. |
You could make a wrapper around |
|
Do we wanna just close this PR? It can't address the general problem that I described above, although it does reduce the surface area a bit. I think this probably deserves a tracking issue. I'm not sure how evaluating unused constants before CTFE as opposed to codegen fixes the problem. Is the point that the unoptimized MIR is still available then? |
joelpalmer
added
S-waiting-on-review
joelpalmer
added
S-waiting-on-author
crlf0710
added
S-waiting-on-author
crlf0710
added
S-waiting-on-author
JohnCSimon
added
S-waiting-on-author
#71078 contained an example of a cyclic data dependency in a
staticinitializer being optimized away before reaching const-eval. #71140 solved this by trying to trigger the cycle while performing the optimization that may remove it.I think we should try to catch as many cycles as we can during const-checking, which will always run before optimizations are applied. Unlike const-eval, however, const-checking is done pre-monomorphization, so I don't think it's possible to detect cyclic dependencies with perfect precision if they involve associated consts. I could be wrong though.
This PR is based on #71330, which only runs const qualification when type-based qualification fails, meaning that without this PR, const-checking will find even less cycles than before. Only the last two commits are new.
It causes an error on #71078 even without #71140 applied. Like #71140, this is a breaking change, and will need a crater run. Unlike #71140, it may result in false positives.
r? @oli-obk