-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Using a String as a generator resume argument causes a segfault #69039
Comments
Heh, looks like my "fix" 392e595 for the miscompilation I found while implementing this wasn't actually sufficient. Reduced to: fn p(n: &String, m: &String) -> String {
println!("PRNT: {} is {}", n, m);
format!("{} is {}", n, m)
}
fn my_scenario() -> impl Generator<String, Yield = (), Return = ()> {
|_arg: String| {
let name1 = yield;
let name2 = yield;
p(&name1, &name2);
}
} Looking at the MIR, you can see:
MIR of
|
Hmm, on second thought, it seems weird that |
Looking at the MIR generated for @jonas-schievink's reduced test case we can see that the writes that should have been to
So this seems to be a problem in MIR construction. |
Well that would explain why the MIR looks wrong. |
I suspect this is because |
Thought I'd fix the printing real quick #69200. I'll look into the real bug tomorrow or some time next week, but the |
…b,Zoxc Fix printing of `Yield` terminator Addresses the bug found in rust-lang#69039 (comment)
@jonas-schievink I don't know much about rustc internals but it seems these places seem relevant somehow? rust/src/librustc_mir/dataflow/impls/storage_liveness.rs Lines 140 to 148 in 5e7af46
rust/src/librustc_mir/dataflow/impls/storage_liveness.rs Lines 150 to 162 in 5e7af46
|
Yes, that's the issue I was referring to. |
This patch makes the generator layout look more reasonable (actually containing slots for the
--- a/src/librustc_mir/dataflow/impls/storage_liveness.rs
+++ b/src/librustc_mir/dataflow/impls/storage_liveness.rs
@@ -146,25 +146,58 @@ impl<'mir, 'tcx> BitDenotation<'tcx> for RequiresStorage<'mir, 'tcx> {
fn before_terminator_effect(&self, sets: &mut GenKillSet<Local>, loc: Location) {
self.check_for_borrow(sets, loc);
- if let TerminatorKind::Call { destination: Some((Place { local, .. }, _)), .. } =
- self.body[loc.block].terminator().kind
- {
- sets.gen(local);
+ match &self.body[loc.block].terminator().kind {
+ TerminatorKind::Call { destination: Some((Place { local, .. }, _)), .. }
+ | TerminatorKind::Yield { resume_arg: Place { local, .. }, .. } => {
+ sets.gen(*local);
+ }
+
+ // Nothing to do for these. Match exhaustively so this fails to compile when new
+ // variants are added.
+ TerminatorKind::Call { destination: None, .. }
+ | TerminatorKind::Abort
+ | TerminatorKind::Assert { .. }
+ | TerminatorKind::Drop { .. }
+ | TerminatorKind::DropAndReplace { .. }
+ | TerminatorKind::FalseEdges { .. }
+ | TerminatorKind::FalseUnwind { .. }
+ | TerminatorKind::GeneratorDrop
+ | TerminatorKind::Goto { .. }
+ | TerminatorKind::Resume
+ | TerminatorKind::Return
+ | TerminatorKind::SwitchInt { .. }
+ | TerminatorKind::Unreachable => {}
}
}
fn terminator_effect(&self, sets: &mut GenKillSet<Local>, loc: Location) {
- // For call terminators the destination requires storage for the call
- // and after the call returns successfully, but not after a panic.
- // Since `propagate_call_unwind` doesn't exist, we have to kill the
- // destination here, and then gen it again in `propagate_call_return`.
- if let TerminatorKind::Call { destination: Some((ref place, _)), .. } =
- self.body[loc.block].terminator().kind
- {
- if let Some(local) = place.as_local() {
- sets.kill(local);
+ match &self.body[loc.block].terminator().kind {
+ // For call terminators the destination requires storage for the call
+ // and after the call returns successfully, but not after a panic.
+ // Since `propagate_call_unwind` doesn't exist, we have to kill the
+ // destination here, and then gen it again in `propagate_call_return`.
+ TerminatorKind::Call { destination: Some((Place { local, .. }, _)), .. } => {
+ sets.kill(*local);
}
+
+ // Nothing to do for these. Match exhaustively so this fails to compile when new
+ // variants are added.
+ TerminatorKind::Call { destination: None, .. }
+ | TerminatorKind::Yield { .. }
+ | TerminatorKind::Abort
+ | TerminatorKind::Assert { .. }
+ | TerminatorKind::Drop { .. }
+ | TerminatorKind::DropAndReplace { .. }
+ | TerminatorKind::FalseEdges { .. }
+ | TerminatorKind::FalseUnwind { .. }
+ | TerminatorKind::GeneratorDrop
+ | TerminatorKind::Goto { .. }
+ | TerminatorKind::Resume
+ | TerminatorKind::Return
+ | TerminatorKind::SwitchInt { .. }
+ | TerminatorKind::Unreachable => {}
}
+
self.check_for_move(sets, loc);
}
--
2.25.0 |
…b,Zoxc Fix printing of `Yield` terminator Addresses the bug found in rust-lang#69039 (comment)
Well, afaict the main issue here is that the transformation which replaces the This happens: rust/src/librustc_mir/transform/generator.rs Lines 279 to 289 in 5e7af46
before this: rust/src/librustc_mir/transform/generator.rs Lines 1122 to 1131 in 5e7af46
I've tested the follwing patch (Hacky, we probably want to use the And it produces correct MIR code: fn my_scenario::{{closure}}#0(_1: std::pin::Pin<&mut [generator@gen.rs:13:5: 17:6 {std::string::String, ()}]>, _2: std::string::String) -> std::ops::GeneratorState<(), ()> {
debug _arg => (((*(_1.0: &mut [generator@gen.rs:13:5: 17:6 {std::string::String, ()}])) as variant#3).1: std::string::String); // in scope 0 at gen.rs:13:6: 13:10
let mut _0: std::ops::GeneratorState<(), ()>; // return place in scope 0 at gen.rs:13:5: 17:6
let mut _3: (); // in scope 0 at gen.rs:14:21: 14:26
let mut _4: (); // in scope 0 at gen.rs:15:21: 15:26
let _5: (); // in scope 0 at gen.rs:16:9: 16:26
let mut _6: &std::string::String; // in scope 0 at gen.rs:16:11: 16:17
let _7: &std::string::String; // in scope 0 at gen.rs:16:11: 16:17
let mut _8: &std::string::String; // in scope 0 at gen.rs:16:19: 16:25
let _9: &std::string::String; // in scope 0 at gen.rs:16:19: 16:25
let mut _10: (); // in scope 0 at gen.rs:13:20: 13:20
let mut _11: isize; // in scope 0 at gen.rs:13:5: 17:6
scope 1 {
debug name1 => (((*(_1.0: &mut [generator@gen.rs:13:5: 17:6 {std::string::String, ()}])) as variant#3).0: std::string::String); // in scope 1 at gen.rs:14:13: 14:18
scope 2 {
debug name2 => (((*(_1.0: &mut [generator@gen.rs:13:5: 17:6 {std::string::String, ()}])) as variant#4).1: std::string::String); // in scope 2 at gen.rs:15:13: 15:18
}
}
bb0: {
_11 = discriminant((*(_1.0: &mut [generator@gen.rs:13:5: 17:6 {std::string::String, ()}]))); // bb0[0]: scope 0 at gen.rs:13:5: 17:6
switchInt(move _11) -> [0u32: bb1, 1u32: bb12, 2u32: bb13, 3u32: bb10, 4u32: bb11, otherwise: bb14]; // bb0[1]: scope 0 at gen.rs:13:5: 17:6
}
bb1: {
(((*(_1.0: &mut [generator@gen.rs:13:5: 17:6 {std::string::String, ()}])) as variant#3).1: std::string::String) = move _2; // bb1[0]: scope 0 at gen.rs:13:5: 17:6
StorageLive(_3); // bb1[1]: scope 0 at gen.rs:14:21: 14:26
((_0 as Yielded).0: ()) = move _3; // bb1[2]: scope 0 at gen.rs:14:21: 14:26
discriminant(_0) = 0; // bb1[3]: scope 0 at gen.rs:14:21: 14:26
discriminant((*(_1.0: &mut [generator@gen.rs:13:5: 17:6 {std::string::String, ()}]))) = 3; // bb1[4]: scope 0 at gen.rs:14:21: 14:26
return; // bb1[5]: scope 0 at gen.rs:14:21: 14:26
}
...
bb10: {
StorageLive(_3); // bb10[0]: scope 0 at gen.rs:13:5: 17:6
(((*(_1.0: &mut [generator@gen.rs:13:5: 17:6 {std::string::String, ()}])) as variant#3).0: std::string::String) = move _2; // bb10[1]: scope 0 at gen.rs:13:5: 17:6
StorageDead(_3); // bb10[2]: scope 0 at gen.rs:14:25: 14:26
StorageLive(_4); // bb10[3]: scope 1 at gen.rs:15:21: 15:26
((_0 as Yielded).0: ()) = move _4; // bb10[4]: scope 1 at gen.rs:15:21: 15:26
discriminant(_0) = 0; // bb10[5]: scope 1 at gen.rs:15:21: 15:26
discriminant((*(_1.0: &mut [generator@gen.rs:13:5: 17:6 {std::string::String, ()}]))) = 4; // bb10[6]: scope 1 at gen.rs:15:21: 15:26
return; // bb10[7]: scope 1 at gen.rs:15:21: 15:26
}
bb11: {
StorageLive(_4); // bb11[0]: scope 0 at gen.rs:13:5: 17:6
(((*(_1.0: &mut [generator@gen.rs:13:5: 17:6 {std::string::String, ()}])) as variant#4).1: std::string::String) = move _2; // bb11[1]: scope 0 at gen.rs:13:5: 17:6
StorageDead(_4); // bb11[2]: scope 1 at gen.rs:15:25: 15:26
StorageLive(_5); // bb11[3]: scope 2 at gen.rs:16:9: 16:26
StorageLive(_6); // bb11[4]: scope 2 at gen.rs:16:11: 16:17
StorageLive(_7); // bb11[5]: scope 2 at gen.rs:16:11: 16:17
_7 = &(((*(_1.0: &mut [generator@gen.rs:13:5: 17:6 {std::string::String, ()}])) as variant#3).0: std::string::String); // bb11[6]: scope 2 at gen.rs:16:11: 16:17
_6 = _7; // bb11[7]: scope 2 at gen.rs:16:11: 16:17
StorageLive(_8); // bb11[8]: scope 2 at gen.rs:16:19: 16:25
StorageLive(_9); // bb11[9]: scope 2 at gen.rs:16:19: 16:25
_9 = &(((*(_1.0: &mut [generator@gen.rs:13:5: 17:6 {std::string::String, ()}])) as variant#4).1: std::string::String); // bb11[10]: scope 2 at gen.rs:16:19: 16:25
_8 = _9; // bb11[11]: scope 2 at gen.rs:16:19: 16:25
_5 = const p(move _6, move _8) -> [return: bb3, unwind: bb6]; // bb11[12]: scope 2 at gen.rs:16:9: 16:26
// ty::Const
// + ty: for<'r, 's> fn(&'r std::string::String, &'s std::string::String) {p}
// + val: Value(Scalar(<ZST>))
// mir::Constant
// + span: gen.rs:16:9: 16:10
// + literal: Const { ty: for<'r, 's> fn(&'r std::string::String, &'s std::string::String) {p}, val: Value(Scalar(<ZST>)) }
}
... EDIT: |
I probably would have noticed that if not for that pesky segfault 😜 |
…b,Zoxc Fix printing of `Yield` terminator Addresses the bug found in rust-lang#69039 (comment)
I tried this code:
I expected to see see the output "Person is Mood", instead, I got a segfault:
rustc +nightly-2020-02-10 --version --verbose
:Backtrace
The text was updated successfully, but these errors were encountered: