Bug #128164 for Future-AsyncAwait: Scopestack name upsets under -DDEBUGGING

Fri Jan 04 08:25:07 2019 leonerd-cpan [...] leonerd.org.uk - Ticket created

Subject:

Scopestack name upsets under -DDEBUGGING

On -DDEBUGGING perl builds, one test reliably fails: http://www.cpantesters.org/cpan/report/51b44220-0fde-11e9-92ed-2d3e4b661d59 perl: lib/Future/AsyncAwait.xs:1005: MY_future_done_from_stack: Assertion `((char*)PL_scopestack_name[PL_scopestack_ix-1] == (char*)"future_done_from_stack") || strEQ(PL_scopestack_name[PL_scopestack_ix-1], "future_done_from_stack")' failed. t/06await-nested.t ..... No subtests run On comparable platforms that don't have -DDEBUGGING this test runs OK, which suggests it is simply a failure to maintain the scopestack name entry correctly, rather than any underlying stack discipline failure. -- Paul Evans

Fri Jan 04 13:11:38 2019 leonerd-cpan [...] leonerd.org.uk - Correspondence added

Possibly fixed here, though it looks a little fragile - with ENTER only being invoked on -DDEBUGGING builds. That or not doesn't appear to break anything though, so.... kinda confusing. -- Paul Evans

Subject:

rt128164.patch

=== modified file 'lib/Future/AsyncAwait.xs' --- lib/Future/AsyncAwait.xs 2019-01-04 16:26:54 +0000 +++ lib/Future/AsyncAwait.xs 2019-01-04 18:10:19 +0000 @@ -640,14 +640,21 @@ suspend_block(frame, cx); -#ifdef DEBUGGING - frame->scopename = PL_scopestack_name[PL_scopestack_ix-1]; -#endif - /* We'll mutate PL_scopestack_ix but it doesn't matter as dounwind() will - * put it right at the end. Do this unconditionally to avoid divergent - * behaviour between -DDEBUGGING builds and non. - */ - --PL_scopestack_ix; + if(cx->blk_oldscopesp <= PL_scopestack_ix) { +#ifdef DEBUGGING + frame->scopename = PL_scopestack_name[PL_scopestack_ix-1]; +#endif + /* We'll mutate PL_scopestack_ix but it doesn't matter as dounwind() will + * put it right at the end. Do this unconditionally to avoid divergent + * behaviour between -DDEBUGGING builds and non. + */ + --PL_scopestack_ix; + } + else { +#ifdef DEBUGGING + frame->scopename = NULL; +#endif + } /* ref: * https://perl5.git.perl.org/perl.git/blob/HEAD:/cop.h @@ -906,6 +913,17 @@ PERL_CONTEXT *cx; +#if defined(DEBUGGING) && HAVE_PERL_VERSION(5, 24, 0) + /* Apparently, we have to ENTER here on versions of perl before the Grand + * Context Stack Refactoring at 5.24, or else the scopename logic will get + * upset. See + * https://rt.cpan.org/Ticket/Display.html?id=128164 + */ + if(frame->scopename) { + ENTER; + } +#endif + switch(frame->type) { case CXt_BLOCK: cx = cx_pushblock(CXt_BLOCK, frame->gimme, PL_stack_sp, PL_savestack_ix); @@ -964,7 +982,9 @@ resume_block(frame, cx); #ifdef DEBUGGING - PL_scopestack_name[PL_scopestack_ix-1] = frame->scopename; + if(frame->scopename) { + PL_scopestack_name[PL_scopestack_ix-1] = frame->scopename; + } #endif Safefree(frame);

Fri Jan 04 13:11:39 2019 leonerd-cpan [...] leonerd.org.uk - Status changed from 'new' to 'patched'

Sat Jan 05 08:24:33 2019 leonerd-cpan [...] leonerd.org.uk - Correspondence added

I'm going to mark resolved for now, but it may turn out that the oddness of the fix isn't quite resolved yet and we might come back to it. -- Paul Evans

Sat Jan 05 08:24:34 2019 leonerd-cpan [...] leonerd.org.uk - Status changed from 'patched' to 'resolved'

Sat Jan 05 08:24:34 2019 leonerd-cpan [...] leonerd.org.uk - Fixed in 0.17 added

Sat Apr 13 15:39:20 2019 leonerd-cpan [...] leonerd.org.uk - Correspondence added

I knew this didn't feel quite right somehow. Turns out the hax/cx_pushblock I copied from mauke was wrong. It shouldn't ENTER; SAVETMPS. Instead, each callsite should do what is the right thing, but only on pre-5.24 perls. Attached patch. This also means I can delete the frame->scopename hacks as well. -- Paul Evans

Subject:

rt128164-pt2.patch

=== modified file 'hax/cx_pushblock.c.inc' --- hax/cx_pushblock.c.inc 2018-01-15 18:20:52 +0000 +++ hax/cx_pushblock.c.inc 2019-04-13 19:35:37 +0000 @@ -9,9 +9,6 @@ PERL_CONTEXT *cx; assert(saveix == PL_savestack_ix); - ENTER; - SAVETMPS; - PUSHBLOCK(cx, t, sp); return cx; } === modified file 'lib/Future/AsyncAwait.xs' --- lib/Future/AsyncAwait.xs 2019-04-13 19:19:46 +0000 +++ lib/Future/AsyncAwait.xs 2019-04-13 19:35:37 +0000 @@ -1156,11 +1156,20 @@ switch(frame->type) { case CXt_BLOCK: +#if !HAVE_PERL_VERSION(5, 24, 0) + ENTER_with_name("block"); + SAVETMPS; +#endif cx = cx_pushblock(CXt_BLOCK, frame->gimme, PL_stack_sp, PL_savestack_ix); /* nothing else special */ break; case CXt_LOOP_PLAIN: +#if !HAVE_PERL_VERSION(5, 24, 0) + ENTER_with_name("loop1"); + SAVETMPS; + ENTER_with_name("loop2"); +#endif cx = cx_pushblock(frame->type, frame->gimme, PL_stack_sp, PL_savestack_ix); /* don't call cx_pushloop_plain() because it will get this wrong */ cx->blk_loop = frame->el.loop; @@ -1174,6 +1183,11 @@ #endif case CXt_LOOP_LAZYSV: case CXt_LOOP_LAZYIV: +#if !HAVE_PERL_VERSION(5, 24, 0) + ENTER_with_name("loop1"); + SAVETMPS; + ENTER_with_name("loop2"); +#endif cx = cx_pushblock(frame->type, frame->gimme, PL_stack_sp, PL_savestack_ix); /* don't call cx_pushloop_plain() because it will get this wrong */ cx->blk_loop = frame->el.loop; @@ -1199,6 +1213,10 @@ if(CATCH_GET) panic("Too late to docatch()\n"); +#if !HAVE_PERL_VERSION(5, 24, 0) + ENTER_with_name("eval_scope"); + SAVETMPS; +#endif cx = cx_pushblock(CXt_EVAL|CXp_TRYBLOCK, frame->gimme, PL_stack_sp, PL_savestack_ix); cx_pusheval(cx, frame->el.eval.retop, NULL);