Using a String as a generator resume argument causes a segfault

[shepmaster]

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

[jonas-schievink]

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:

• The first resume leads to bb1, which stores the resume argument from _2 in the generator state as _arg and advances to the next state (discriminant 3).
• On the next call it resumes at bb13, which doesn't store the resume argument _2 in the state (it only moves it to _3). Here, the argument should have ended up in the generator state as some name1 field. It then sets the discriminant to 4 to resume at bb14.
• The next call thinks _3 is 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]

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

[Zoxc]

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: [generator@test.rs: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.

[cynecx]

@Zoxc Hmm:

rust/src/librustc/mir/mod.rs

Line 1478 in 61d9231

Yield { ref value, .. } => write!(fmt, "_1 = suspend({:?})", value),

I assume this isn't correct anymore? (_1 probably should have been replaced with the newly introduced field resume_arg in Yield? 😅)

[Zoxc]

Well that would explain why the MIR looks wrong.

[Zoxc]

I suspect this is because RequiresStorage doesn't consider Yield terminators as writes.

[jonas-schievink]

Thought I'd fix the printing real quick #69200. I'll look into the real bug tomorrow or some time next week, but the RequiresStorage pass does indeed look wrong now.