From 72f28af4674d41a7b1c2a7eaa67360e5e2f32477 Mon Sep 17 00:00:00 2001 From: David Mitchell Date: Wed, 1 Jul 2015 20:19:23 +0100 Subject: [PATCH] pp_entersub: skip resetting @_ Whenever we leave a sub by whatever means (pp_leavesub, pp_return, pp_goto, die etc) it is the responsibility of the code that pops the SUB context to clean up the private @_ of that sub (i.e pad[0]) so that it's empty and not real. There's some code in pp_entersub that duplicates this check. I believe this check is unnecessary and so this commit removes it and replaces it with an assert. It was added by 221373f04 in 1999 to fix the issue described in Subject: [ID 19991015.010] [BUG 5.005_62 Assertation failed: "pp_ctl.c" line 2443] Message-Id: <19991016024500.A32541@athens.aocn.com> There were two fixes applied for this issue; the other was 0253cb4. I think the second commit actually fixed the issue and the first fix was a red herring. If my newly-added assert fails, it implies that something needs fixing in the context-popping code, rather than in pp_entersub. In fact the new assert showed up one issue: the special-case handling of @_ for &CORE::undef in pp_coreargs wasn't emptying the array, so I added a CLEAR_ARGARRAY(). --- pp.c | 8 ++++++-- pp_hot.c | 14 ++++++-------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/pp.c b/pp.c index 30a2dd0..8900706 100644 --- a/pp.c +++ b/pp.c @@ -6440,15 +6440,19 @@ PP(pp_coreargs) if (opnum == OP_UNDEF && SvRV(*svp) == (SV *)PL_defgv && cxstack[cxstack_ix].cx_type & CXp_HASARGS) { /* Undo @_ localisation, so that sub exit does not undo - part of our undeffing. */ + part of our undeffing. + this corresponds to the part of POPSUB that + does @_ cleanup */ PERL_CONTEXT *cx = &cxstack[cxstack_ix]; + AV *av = MUTABLE_AV(PAD_SVl(0)); POP_SAVEARRAY(); cx->cx_type &= ~ CXp_HASARGS; assert(AvARRAY(MUTABLE_AV( PadlistARRAY(CvPADLIST(cx->blk_sub.cv))[ CvDEPTH(cx->blk_sub.cv)])) == PL_curpad); - assert(!AvREAL(MUTABLE_AV(PAD_SVl(0)))); + assert(!AvREAL(av)); + CLEAR_ARGARRAY(av); } } break; diff --git a/pp_hot.c b/pp_hot.c index edd4cf7..b2922b1 100644 --- a/pp_hot.c +++ b/pp_hot.c @@ -3480,18 +3480,16 @@ PP(pp_entersub) SSize_t items; AV **defavp; - if (UNLIKELY(AvREAL(av))) { - /* @_ is normally not REAL--this should only ever - * happen when DB::sub() calls things that modify @_ */ - av_clear(av); - AvREAL_off(av); - AvREIFY_on(av); - } defavp = &GvAV(PL_defgv); cx->blk_sub.savearray = *defavp; *defavp = MUTABLE_AV(SvREFCNT_inc_simple_NN(av)); - items = SP - MARK; + /* it's the responsibility of whoever leaves a sub to ensure + * that a clean, empty AV is left in pad[0]. This is normally + * done by POPSUB() */ + assert(!AvREAL(av) && AvFILLp(av) == -1); + + items = SP - MARK; if (UNLIKELY(items - 1 > AvMAX(av))) { SV **ary = AvALLOC(av); AvMAX(av) = items - 1; -- 1.8.3.1