This is a live mirror of the Perl 5 development currently hosted at https://github.com/perl/perl5
eliminate an SAVEFREESV(cv) from PUSHSUB
authorDavid Mitchell <davem@iabyn.com>
Thu, 25 Jun 2015 09:48:58 +0000 (10:48 +0100)
committerDavid Mitchell <davem@iabyn.com>
Wed, 3 Feb 2016 08:59:32 +0000 (08:59 +0000)
When a CXt_SUB context is pushed, the reference count of the CV is
increased (and is effectively owned by that context frame). It is
decremented when the context frame is popped.

There was an issue in that the POPSUB was done before the LEAVE,
which meant that at sub exit, the sub could be freed before stuff that
referred to that sub was done (such as lexicals being cleaned up by
SAVEt_CLEARSV).

This was fixed by perl-5.005_02-2095-gb0d9ce3, which split POPSUB
into POPSUB and LEAVESUB with the latter responsible for the decrement.
So for example code might do POPSUB; LEAVE; LEAVSUB.

However this wasn't sufficient, because a similar thing happened during
die, for example

    sub d {die}
    my $f;
    $f = sub {my $x=1; $f = 0; d};
    eval{ $f->() };

since all *all* the CXt_SUB contexts were popped with POPSUB/LEAVSUB
*before* LEAVE was called.

I fixed this with perl-5.8.0-3206-gb36bdec, by adding another increment to
the CV and doing a SAVEFREESV(cv), both in PUSHSUB. This meant that
the save stack would hold at least one reference to the CV while being
unwound.

However, v5.19.3-139-g2537512 then changed things so that POPSUB also
did a LEAVE. This means that now, die unwinds the context stack and the
save stack in lockstep, so my second fix is now redundant. So this commit
undoes it, saving an rc++/-- and SAVEFREE(cv) per sub call.

cop.h

diff --git a/cop.h b/cop.h
index b15ddf4..3eabd89 100644 (file)
--- a/cop.h
+++ b/cop.h
@@ -571,8 +571,8 @@ struct block_format {
 };
 
 /* base for the next two macros. Don't use directly.
- * Note that the refcnt of the cv is incremented twice;  The CX one is
- * decremented by LEAVESUB, the other by LEAVE. */
+ * The context frame holds a reference to the CV so that it can't be
+ * freed while we're executing it */
 
 #define PUSHSUB_BASE(cx)                                               \
        ENTRY_PROBE(CvNAMED(cv)                                         \
@@ -586,11 +586,8 @@ struct block_format {
        cx->blk_sub.olddepth = CvDEPTH(cv);                             \
        cx->cx_type |= (hasargs) ? CXp_HASARGS : 0;                     \
        cx->blk_sub.retop = NULL;                                       \
-       if (!CvDEPTH(cv)) {                                             \
-           SvREFCNT_inc_simple_void_NN(cv);                            \
-           SvREFCNT_inc_simple_void_NN(cv);                            \
-           SAVEFREESV(cv);                                             \
-       }
+       if (!CvDEPTH(cv))                                               \
+           SvREFCNT_inc_simple_void_NN(cv);
 
 #define PUSHSUB_GET_LVALUE_MASK(func) \
        /* If the context is indeterminate, then only the lvalue */     \