Bug #129319 for Future-AsyncAwait: panic: ARGH CXt_LOOP_LIST frame tried to steal stack values or marks

Fri Apr 26 08:09:01 2019 leonerd-cpan [...] leonerd.org.uk - Ticket created

Subject:

panic: ARGH CXt_LOOP_LIST frame tried to steal stack values or marks

(from https://rt.cpan.org/Ticket/Display.html?id=129316) Happens via Device::BusPirate but only on 5.24/5.26, not 5.28: $ perl5.24.4th Build test t/00use.t ..... ok t/01serial.t .. ok t/10bbio.t .... ok t/20spi.t ..... ok t/30i2c.t ..... 1/? Future::AsyncAwait panic: ARGH CXt_LOOP_LIST frame tried to steal stack values or marks t/30i2c.t ..... All 5 subtests passed t/99pod.t ..... skipped: Test::Pod 1.00 required for testing POD Test Summary Report ------------------- t/30i2c.t (Wstat: 6 Tests: 5 Failed: 0) Non-zero wait status: 6 Parse errors: No plan found in TAP output Files=6, Tests=36, 0 wallclock secs ( 0.04 usr 0.00 sys + 0.36 cusr 0.03 csys = 0.43 CPU) Result: FAIL Failed 1/6 test programs. 0/36 subtests failed. $ perl5.26.2-dbg Build test t/00use.t ..... ok t/01serial.t .. ok t/10bbio.t .... ok t/20spi.t ..... ok t/30i2c.t ..... 1/? Future::AsyncAwait panic: ARGH CXt_LOOP_LIST frame tried to steal stack values or marks t/30i2c.t ..... All 5 subtests passed t/99pod.t ..... skipped: Test::Pod 1.00 required for testing POD Test Summary Report ------------------- t/30i2c.t (Wstat: 6 Tests: 5 Failed: 0) Non-zero wait status: 6 Parse errors: No plan found in TAP output Files=6, Tests=36, 1 wallclock secs ( 0.04 usr 0.00 sys + 0.64 cusr 0.03 csys = 0.71 CPU) Result: FAIL Failed 1/6 test programs. 0/36 subtests failed. $ perl5.28.1 Build test t/00use.t ..... ok t/01serial.t .. ok t/10bbio.t .... ok t/20spi.t ..... ok t/30i2c.t ..... ok t/99pod.t ..... ok All tests successful. Files=6, Tests=51, 0 wallclock secs ( 0.04 usr 0.01 sys + 0.59 cusr 0.08 csys = 0.72 CPU) Result: PASS -- Paul Evans

Fri Apr 26 10:01:24 2019 leonerd-cpan [...] leonerd.org.uk - Correspondence added

I believe this is a return of RT129215. If I apply the previous workaround (using a temporary array rather than stack temporaries) then the problem goes away. Seems like 129215 wasn't as fixed as we thought. -- Paul Evans

Fri Apr 26 16:52:12 2019 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Fri Apr 26 10:01:24 2019, PEVANS wrote: Show quoted text

> I believe this is a return of RT129215. If I apply the previous > workaround (using a temporary array rather than stack temporaries) > then the problem goes away. > > Seems like 129215 wasn't as fixed as we thought.

Further investigation shows this is caused by there being extra SVs on the stack after the foreach-LIST items. Such are put there by temporaries in the await expression; a unit test to find it: { my @F = map { Future->new } 0, 1, 2; my $fret = (async sub { my $ret = ""; foreach my $idx ( 0, 1, 2 ) { $ret .= await $F[$idx]; } return $ret; })->(); $F[0]->done( "A" ); $F[1]->done( "B" ); $F[2]->done( "C" ); is( scalar $fret->get, "ABC", '$fret now ready after await with stack items before LIST' ); } This fails on everything before 5.28. I don't yet know why it *doesn't* fail on 5.28 - perhaps the concat-assign op looks a little different? Anyhow, I have a test to demonstrate it so hopefully fixing it shouldn't be hard now. -- Paul Evans

Fri Apr 26 17:03:45 2019 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Fri Apr 26 16:52:12 2019, PEVANS wrote: Show quoted text

> This fails on everything before 5.28. I don't yet know why it > *doesn't* fail on 5.28 - perhaps the concat-assign op looks a little > different?

wagnerc on #p5p suggested multiconcat, and indeed that's it. on 5.28 the read() sub contains an optree fragment: 1b <+> multiconcat("",-1,-1)[$ret:1943,1949] vK/APPEND,TARGMY ->1c - <0> ex-padsv sRM ->11 1a <1> await sK/1 ->1b so the multiconcat stores $ret directly and the padsv op becomes delinked; whereas on 5.26 or earlier it shows: 1d <2> concat[t14] vKS/2 ->1e 12 <0> padsv[$ret:1814,1820] sRM ->13 1c <1> await sK/1 ->1d a regular concat op and a real padsv op. wagnerc++ -- Paul Evans

Fri Apr 26 17:20:59 2019 leonerd-cpan [...] leonerd.org.uk - Correspondence added

Turns out the fix is pretty easy. Passes now on all of 5.16 - 5.28. -- Paul Evans

Subject:

rt129319.patch

=== modified file 'lib/Future/AsyncAwait.xs' --- lib/Future/AsyncAwait.xs 2019-04-26 18:17:47 +0000 +++ lib/Future/AsyncAwait.xs 2019-04-26 21:17:29 +0000 @@ -761,8 +761,6 @@ * have to store them relative to something fixed. */ if(!cx->blk_loop.state_u.ary.ary) { - if(frame->stacklen) - panic("ARGH CXt_LOOP_FOR/stacklist frame tried to steal stack values\n"); if(frame->marklen) panic("ARGH CXt_LOOP_FOR/stacklist frame tried to steal marks\n"); I32 height = PL_stack_sp - PL_stack_base; @@ -781,8 +779,6 @@ * other frame types, but that meaning shouldn't matter provided we * didn't attempt to steal any stack values or marks */ - if(frame->stacklen) - panic("ARGH CXt_LOOP_LIST frame tried to steal stack values\n"); if(frame->marklen) panic("ARGH CXt_LOOP_LIST frame tried to steal marks\n"); @@ -1287,7 +1283,7 @@ #if !HAVE_PERL_VERSION(5, 24, 0) case CXt_LOOP_FOR: if(!cx->blk_loop.state_u.ary.ary) { - I32 height = PL_stack_sp - PL_stack_base; + I32 height = PL_stack_sp - PL_stack_base - frame->stacklen; cx->blk_loop.state_u.ary.ix = height - cx->blk_loop.state_u.ary.ix; } break; @@ -1295,7 +1291,7 @@ #if HAVE_PERL_VERSION(5, 24, 0) case CXt_LOOP_LIST: { - I32 height = PL_stack_sp - PL_stack_base; + I32 height = PL_stack_sp - PL_stack_base - frame->stacklen; cx->blk_loop.state_u.stack.basesp = height - cx->blk_loop.state_u.stack.basesp; cx->blk_loop.state_u.stack.ix = height - cx->blk_loop.state_u.stack.ix; === modified file 't/22context-foreach.t' --- t/22context-foreach.t 2019-04-18 16:22:52 +0000 +++ t/22context-foreach.t 2019-04-26 21:17:29 +0000 @@ -137,6 +137,25 @@ is( scalar $fret->get, "OK", '$fret now ready after differing stack resumes' ); } +# RT#129319 +{ + my @F = map { Future->new } 0, 1, 2; + my $fret = (async sub { + my $ret = ""; + foreach my $idx ( 0, 1, 2 ) { + # $ret will appear on the stack after the foreach-LIST items + $ret .= await $F[$idx]; + } + return $ret; + })->(); + + $F[0]->done( "A" ); + $F[1]->done( "B" ); + $F[2]->done( "C" ); + + is( scalar $fret->get, "ABC", '$fret now ready after await with stack items before LIST' ); +} + SKIP: { skip "IO::Async::Loop not available", 1 unless eval { require IO::Async::Loop; }; my $loop = IO::Async::Loop->new;

Fri Apr 26 17:21:00 2019 leonerd-cpan [...] leonerd.org.uk - Status changed from 'new' to 'patched'

Mon Apr 29 04:43:45 2019 leonerd-cpan [...] leonerd.org.uk - Correspondence added

Fix released in 0.26 -- Paul Evans

Mon Apr 29 04:43:46 2019 leonerd-cpan [...] leonerd.org.uk - Status changed from 'patched' to 'resolved'

Mon Apr 29 04:43:46 2019 leonerd-cpan [...] leonerd.org.uk - Fixed in 0.26 added