Rust: Using a String as a generator resume argument causes a segfault
I tried this code:
#![feature(generators, generator_trait)]
use std::{
io,
ops::{Generator, GeneratorState},
};
fn my_scenario() -> impl Generator<String, Yield = &'static str, Return = String> {
|_arg: String| {
let my_name = yield "What is your name?";
let my_mood = yield "How are you feeling?";
format!("{} is {}", my_name.trim(), my_mood.trim())
}
}
fn main() {
let mut my_session = Box::pin(my_scenario());
loop {
let mut line = String::new();
match my_session.as_mut().resume(line) {
GeneratorState::Yielded(prompt) => {
println!("{}", prompt);
}
GeneratorState::Complete(v) => {
println!("{}", v);
break;
}
}
line = String::new();
io::stdin().read_line(&mut line).unwrap();
}
}
I expected to see see the output "Person is Mood", instead, I got a segfault:
./target/debug/example
What is your name?
Person
How are you feeling?
Mood
zsh: segmentation fault ./target/debug/example
rustc +nightly-2020-02-10 --version --verbose:
rustc 1.43.0-nightly (71c7e149e 2020-02-09)
binary: rustc
commit-hash: 71c7e149e42cb0fc78a80db70d2525973311d488
commit-date: 2020-02-09
host: x86_64-apple-darwin
release: 1.43.0-nightly
LLVM version: 9.0
Backtrace
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
* frame #0: 0x0000000100023e23 so`core::str::_$LT$impl$u20$str$GT$::trim::hb5debbd36d4b73d0 [inlined] core::str::next_code_point_reverse::hf0fce3726ea99362 at mod.rs:554:21 [opt]
frame #1: 0x0000000100023e13 so`core::str::_$LT$impl$u20$str$GT$::trim::hb5debbd36d4b73d0 [inlined] _$LT$core..str..Chars$u20$as$u20$core..iter..traits..double_ended..DoubleEndedIterator$GT$::next_back::hc11021e0120a3995 at mod.rs:631 [opt]
frame #2: 0x0000000100023e13 so`core::str::_$LT$impl$u20$str$GT$::trim::hb5debbd36d4b73d0 [inlined] _$LT$core..str..CharIndices$u20$as$u20$core..iter..traits..double_ended..DoubleEndedIterator$GT$::next_back::h66ffec0553639954 at mod.rs:722 [opt]
frame #3: 0x0000000100023e13 so`core::str::_$LT$impl$u20$str$GT$::trim::hb5debbd36d4b73d0 [inlined] _$LT$core..str..pattern..MultiCharEqSearcher$LT$C$GT$$u20$as$u20$core..str..pattern..ReverseSearcher$GT$::next_back::hbe5e94cd1a3aa61a at pattern.rs:540 [opt]
frame #4: 0x0000000100023dc6 so`core::str::_$LT$impl$u20$str$GT$::trim::hb5debbd36d4b73d0 [inlined] core::str::pattern::ReverseSearcher::next_reject_back::h865e71e64b0a0a65 at pattern.rs:201 [opt]
frame #5: 0x0000000100023dc6 so`core::str::_$LT$impl$u20$str$GT$::trim::hb5debbd36d4b73d0 [inlined] _$LT$core..str..pattern..CharPredicateSearcher$LT$F$GT$$u20$as$u20$core..str..pattern..ReverseSearcher$GT$::next_reject_back::h30b729c431f6e983 at pattern.rs:615 [opt]
frame #6: 0x0000000100023dc6 so`core::str::_$LT$impl$u20$str$GT$::trim::hb5debbd36d4b73d0 [inlined] core::str::_$LT$impl$u20$str$GT$::trim_matches::h4d77028f159152d3 at mod.rs:3828 [opt]
frame #7: 0x0000000100023c41 so`core::str::_$LT$impl$u20$str$GT$::trim::hb5debbd36d4b73d0 at mod.rs:3631 [opt]
frame #8: 0x0000000100002235 so`so::my_scenario::_$u7b$$u7b$closure$u7d$$u7d$::h5123a67f3a151902((null)=String @ 0x00007ffeefbff148) at main.rs:12:28
frame #9: 0x000000010000328d so`so::main::h64afc6de80b00fef at main.rs:22:14
frame #10: 0x0000000100003682 so`std::rt::lang_start::_$u7b$$u7b$closure$u7d$$u7d$::h2f745c4383613da0 at rt.rs:67:33
frame #11: 0x000000010000c1a8 so`std::panicking::try::do_call::hb3640f3c79a55ca9 [inlined] std::rt::lang_start_internal::_$u7b$$u7b$closure$u7d$$u7d$::h060e10b65682cba1 at rt.rs:52:12 [opt]
frame #12: 0x000000010000c19c so`std::panicking::try::do_call::hb3640f3c79a55ca9 at panicking.rs:303 [opt]
frame #13: 0x000000010000d9cb so`__rust_maybe_catch_panic at lib.rs:86:7 [opt]
frame #14: 0x000000010000ca3c so`std::rt::lang_start_internal::h06d0538a60720103 [inlined] std::panicking::try::h28c1481276f9013c at panicking.rs:281:12 [opt]
frame #15: 0x000000010000ca09 so`std::rt::lang_start_internal::h06d0538a60720103 [inlined] std::panic::catch_unwind::h8d148cd6572d34ca at panic.rs:394 [opt]
frame #16: 0x000000010000ca09 so`std::rt::lang_start_internal::h06d0538a60720103 at rt.rs:51 [opt]
frame #17: 0x0000000100003662 so`std::rt::lang_start::hb165706ba3f75183(main=(so`so::main::h64afc6de80b00fef at main.rs:16), argc=1, argv=0x00007ffeefbff420) at rt.rs:67:4
frame #18: 0x0000000100003612 so`main + 34
frame #19: 0x00007fff6794c7fd libdyld.dylib`start + 1
shepmaster
All 13 comments
Heh, looks like my "fix" 392e59500a96be718383e127d38bf74300f521c0 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:
- The first
resumeleads tobb1, which stores the resume argument from_2in the generator state as_argand advances to the next state (discriminant 3). - On the next call it resumes at
bb13, which doesn't store the resume argument_2in the state (it only moves it to_3). Here, the argument should have ended up in the generator state as somename1field. It then sets the discriminant to 4 to resume atbb14. - The next call thinks
_3is still live and uses it to call the function.
MIR of bb14 (minus storage statements):
bb14: {
_5 = move _2; // bb14[3]: scope 0 at src/main.rs:13:5: 17:6
_9 = &_3; // bb14[8]: scope 2 at src/main.rs:16:11: 16:17
_8 = _9; // bb14[9]: scope 2 at src/main.rs:16:11: 16:17
_11 = &_5; // bb14[12]: scope 2 at src/main.rs:16:19: 16:25
_10 = _11; // bb14[13]: scope 2 at src/main.rs:16:19: 16:25
_7 = const p(move _8, move _10) -> [return: bb3, unwind: bb7]; // bb14[14]: scope 2 at src/main.rs:16:9: 16:26
// ty::Const
// + ty: for<'r, 's> fn(&'r std::string::String, &'s std::string::String) -> std::string::String {p}
// + val: Value(Scalar(<ZST>))
// mir::Constant
// + span: src/main.rs:16:9: 16:10
// + literal: Const { ty: for<'r, 's> fn(&'r std::string::String, &'s std::string::String) -> std::string::String {p}, val: Value(Scalar(<ZST>)) }
}
jonas-schievink
on 10 Feb 2020
Hmm, on second thought, it seems weird that _3 is not put in the generator state at all, I would expect the existing transform infrastructure to handle this. cc @Zoxc
jonas-schievink
on 11 Feb 2020
Looking at the MIR generated for @jonas-schievink's reduced test case we can see that the writes that should have been to name1 is instead done to the generator _1:
fn my_scenario::{{closure}}#0(_1: [[email protected]:12:5: 16:6 {std::string::String, ()}], _2: std::string::String) -> ()
yields ()
{
debug _arg => _2; // in scope 0 at test.rs:12:6: 12:10
let mut _0: (); // return place in scope 0 at test.rs:12:20: 12:20
let _3: std::string::String; // in scope 0 at test.rs:13:13: 13:18
let mut _4: (); // in scope 0 at test.rs:13:21: 13:26
let mut _6: (); // in scope 0 at test.rs:14:21: 14:26
let _7: std::string::String; // in scope 0 at test.rs:15:9: 15:26
let mut _8: &std::string::String; // in scope 0 at test.rs:15:11: 15:17
let _9: &std::string::String; // in scope 0 at test.rs:15:11: 15:17
let mut _10: &std::string::String; // in scope 0 at test.rs:15:19: 15:25
let _11: &std::string::String; // in scope 0 at test.rs:15:19: 15:25
scope 1 {
debug name1 => _3; // in scope 1 at test.rs:13:13: 13:18
let _5: std::string::String; // in scope 1 at test.rs:14:13: 14:18
scope 2 {
debug name2 => _5; // in scope 2 at test.rs:14:13: 14:18
}
}
bb0: {
StorageLive(_3); // bb0[0]: scope 0 at test.rs:13:13: 13:18
StorageLive(_4); // bb0[1]: scope 0 at test.rs:13:21: 13:26
_4 = (); // bb0[2]: scope 0 at test.rs:13:21: 13:26
_1 = suspend(move _4) -> [resume: bb2, drop: bb7]; // bb0[3]: scope 0 at test.rs:13:21: 13:26
}
...
So this seems to be a problem in MIR construction.
Zoxc
on 11 Feb 2020
@Zoxc Hmm:
I assume this isn't correct anymore? (_1 probably should have been replaced with the newly introduced field resume_arg in Yield? 馃槄)
cynecx
on 15 Feb 2020
😄2
Well that would explain why the MIR looks wrong.
Zoxc
on 15 Feb 2020
I suspect this is because RequiresStorage doesn't consider Yield terminators as writes.
Zoxc
on 15 Feb 2020
Thought I'd fix the printing real quick https://github.com/rust-lang/rust/pull/69200. I'll look into the real bug tomorrow or some time next week, but the RequiresStorage pass does indeed look wrong now.
jonas-schievink
on 16 Feb 2020
@jonas-schievink I don't know much about rustc internals but it seems these places seem relevant somehow?
cynecx
on 16 Feb 2020
Yes, that's the issue I was referring to.
jonas-schievink
on 16 Feb 2020
😄1
This patch makes the generator layout look more reasonable (actually containing slots for the Strings), but doesn't fix the crash. In fact, it makes the resume-live-across-yield.rs test miscompile too:
thread 'main' panicked at 'assertion failed: `(left == right)`
left: `""`,
right: `"Number Two"`', /home/jonas/dev/rust/src/test/ui/generator/resume-live-across-yield.rs:36:13
--- 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
Well, afaict the main issue here is that the transformation which replaces the Places with generator struct accesses is run before creating the actual resume function which emits the store of the resume argument to a local which should've been replaced.
This happens:
before this:
I've tested the follwing patch (Hacky, we probably want to use the TransformVisitor somehow):
https://github.com/cynecx/rust/commit/bb14428d61b3acab3018d53804e467a411a13891 (Most of the diff is not relevant to the patch, create_generator_resume_function contains the fix).
And it produces correct MIR code:
fn my_scenario::{{closure}}#0(_1: std::pin::Pin<&mut [[email protected]:13:5: 17:6 {std::string::String, ()}]>, _2: std::string::String) -> std::ops::GeneratorState<(), ()> {
debug _arg => (((*(_1.0: &mut [[email protected]: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 [[email protected]: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 [[email protected]: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 [[email protected]: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 [[email protected]: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 [[email protected]: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 [[email protected]: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 [[email protected]: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 [[email protected]: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 [[email protected]: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 [[email protected]: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've just realized that this doesn't fix the original reproduction code, it doesn't segfault though. The original code contains a logic bug, every loop iteration a new empty String is created , which is probably not intended, instead moving that binding to the outer scope should "fix" it.
EDIT: And resume-live-across-yield.rs passes too.
EDIT: Also I've just stupidly realized that the unwrap for transform.remap should probably be avoided too since a local may not live across a suspension point.
cynecx
on 17 Feb 2020
🎉1
The original code contains a logic bug
I probably would have noticed that if not for that pesky segfault 馃槣
shepmaster
on 17 Feb 2020
😄2
@cynecx That's a great start! I've opened a fix (+ cleanup + more docs) based on the same idea in https://github.com/rust-lang/rust/pull/69302
jonas-schievink
on 19 Feb 2020
👍1
Was this page helpful?
0 / 5 - 0 ratings
Related issues
dnsl48
路
3Comments
defuz
路
3Comments
drewcrawford
路
3Comments
overvenus
路
3Comments
modsec
路
3Comments
Most helpful comment
I probably would have noticed that if not for that pesky segfault 馃槣