Check for assignments between non-conflicting generator saved locals #73244
Conversation
rust-highfive
added
the
S-waiting-on-review
ecstatic-morse
changed the title
|
I ran this locally on the |
| let lhs = match self.assigned_local { | ||
| Some(l) => l, | ||
| None => { | ||
| // We should be visiting all places used in the MIR. |
There was a problem hiding this comment.
I think the wording of this comment could be improved. The correctness of the code seems to hinge on the fact that we skip running f in check_assigned_place if the lhs is not saved. In those cases we don't visit all places used in the MIR.
Maybe the more generic "handling" is better?
| // We should be visiting all places used in the MIR. | |
| // We should be handling all place uses. |
This is to prevent the miscompilation in rust-lang#73137 from reappearing. Only runs with `-Zvalidate-mir`.
Co-authored-by: Tyler Mandry <tmandry@gmail.com>
ecstatic-morse
force-pushed
the
validate-generator-mir
branch
from
a6d5b33 to
b2ec645
Compare
| self.check_assigned_place(*resume_arg, |this| this.visit_operand(value, location)); | ||
| } | ||
|
|
||
| // FIXME: Does `asm!` have any aliasing requirements? |
There was a problem hiding this comment.
I don't think so. All inputs are completely moved to registers before running the inline asm and storing the outputs again.
|
@bors r=tmandry Nits have been fixed. I'll handle the ASM FIXMEs separately. |
|
📌 Commit b2ec645 has been approved by |
bors
added
S-waiting-on-bors
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit b2ec645 has been approved by |
…ir, r=tmandry Check for assignments between non-conflicting generator saved locals This is to prevent future changes to the generator transform from reintroducing the problem that caused rust-lang#73137. Namely, a store between two generator saved locals whose storage does not conflict. My ultimate goal is to introduce a modified version of rust-lang#71956 that handles this case properly. r? @tmandry
…ir, r=tmandry Check for assignments between non-conflicting generator saved locals This is to prevent future changes to the generator transform from reintroducing the problem that caused rust-lang#73137. Namely, a store between two generator saved locals whose storage does not conflict. My ultimate goal is to introduce a modified version of rust-lang#71956 that handles this case properly. r? @tmandry
…arth Rollup of 11 pull requests Successful merges: - rust-lang#72780 (Enforce doc alias check) - rust-lang#72876 (Mention that BTreeMap::new() doesn't allocate) - rust-lang#73244 (Check for assignments between non-conflicting generator saved locals) - rust-lang#73488 (code coverage foundation for hash and num_counters) - rust-lang#73523 (Fix -Z unpretty=everybody_loops) - rust-lang#73587 (Move remaining `NodeId` APIs from `Definitions` to `Resolver`) - rust-lang#73601 (Point at the call span when overflow occurs during monomorphization) - rust-lang#73613 (The const propagator cannot trace references.) - rust-lang#73614 (fix `intrinsics::needs_drop` docs) - rust-lang#73630 (Provide context on E0308 involving fn items) - rust-lang#73665 (rustc: Modernize wasm checks for atomics) Failed merges: r? @ghost
| @@ -1315,3 +1341,134 @@ impl<'tcx> MirPass<'tcx> for StateTransform { | |||
| create_generator_resume_function(tcx, transform, source, body, can_return); | |||
| } | |||
| } | |||
|
|
|||
| /// Looks for any assignments between locals (e.g., `_4 = _5`) that will both be converted to fields | |||
There was a problem hiding this comment.
Why do we only need to validate assignments where the RHS is a direct usage of a local? Is there something else that prevents this kind of issue when an indirect usage is involved?
There was a problem hiding this comment.
Good point.
Is MIR such as *p = *q or *p = q valid when p overlaps with q? p = *q is almost certainly invalid, and should be handled by this check. I'll check for all three versions in a subsequent PR.
This is to prevent future changes to the generator transform from reintroducing the problem that caused #73137. Namely, a store between two generator saved locals whose storage does not conflict.
My ultimate goal is to introduce a modified version of #71956 that handles this case properly.
r? @tmandry