Skip to content

Commit 73cdf3a

Browse files
committed
Make op_free() non-recursive
Stop using the DEFER mechanism (which could leak if something croaks) and instead tree walk using the new OP_PARENT link to allow walking back up the tree. The freeing is done depth-first: children are freed before their parents.
1 parent 2a56a87 commit 73cdf3a

File tree

1 file changed

+63
-51
lines changed

1 file changed

+63
-51
lines changed

op.c

Lines changed: 63 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -807,8 +807,8 @@ S_op_destroy(pTHX_ OP *o)
807807
/*
808808
=for apidoc op_free
809809

810-
Free an op. Only use this when an op is no longer linked to from any
811-
optree.
810+
Free an op and its children. Only use this when an op is no longer linked
811+
to from any optree.
812812

813813
=cut
814814
*/
@@ -818,13 +818,68 @@ Perl_op_free(pTHX_ OP *o)
818818
{
819819
dVAR;
820820
OPCODE type;
821-
dDEFER_OP;
821+
OP *top_op = o;
822+
OP *next_op = o;
823+
bool went_up = FALSE; /* whether we reached the current node by
824+
following the parent pointer from a child, and
825+
so have already seen this node */
822826

823-
do {
827+
if (!o || o->op_type == OP_FREED)
828+
return;
829+
830+
if (o->op_private & OPpREFCOUNTED) {
831+
/* if base of tree is refcounted, just decrement */
832+
switch (o->op_type) {
833+
case OP_LEAVESUB:
834+
case OP_LEAVESUBLV:
835+
case OP_LEAVEEVAL:
836+
case OP_LEAVE:
837+
case OP_SCOPE:
838+
case OP_LEAVEWRITE:
839+
{
840+
PADOFFSET refcnt;
841+
OP_REFCNT_LOCK;
842+
refcnt = OpREFCNT_dec(o);
843+
OP_REFCNT_UNLOCK;
844+
if (refcnt) {
845+
/* Need to find and remove any pattern match ops from
846+
* the list we maintain for reset(). */
847+
find_and_forget_pmops(o);
848+
return;
849+
}
850+
}
851+
break;
852+
default:
853+
break;
854+
}
855+
}
856+
857+
while (next_op) {
858+
o = next_op;
859+
860+
/* free child ops before ourself, (then free ourself "on the
861+
* way back up") */
862+
863+
if (!went_up && o->op_flags & OPf_KIDS) {
864+
next_op = cUNOPo->op_first;
865+
continue;
866+
}
867+
868+
/* find the next node to visit, *then* free the current node
869+
* (can't rely on o->op_* fields being valid after o has been
870+
* freed) */
871+
872+
/* The next node to visit will be either the sibling, or the
873+
* parent if no siblings left, or NULL if we've worked our way
874+
* back up to the top node in the tree */
875+
next_op = (o == top_op) ? NULL : o->op_sibparent;
876+
went_up = cBOOL(!OpHAS_SIBLING(o)); /* parents are already visited */
877+
878+
/* Now process the current node */
824879

825880
/* Though ops may be freed twice, freeing the op after its slab is a
826881
big no-no. */
827-
assert(!o || !o->op_slabbed || OpSLAB(o)->opslab_refcnt != ~(size_t)0);
882+
assert(!o->op_slabbed || OpSLAB(o)->opslab_refcnt != ~(size_t)0);
828883
/* During the forced freeing of ops after compilation failure, kidops
829884
may be freed before their parents. */
830885
if (!o || o->op_type == OP_FREED)
@@ -843,62 +898,20 @@ Perl_op_free(pTHX_ OP *o)
843898
* we can't spot faults in the main code, only
844899
* evaled/required code */
845900
#ifdef DEBUGGING
846-
if ( o->op_ppaddr == PL_ppaddr[o->op_type]
901+
if ( o->op_ppaddr == PL_ppaddr[type]
847902
&& PL_parser
848903
&& !PL_parser->error_count)
849904
{
850905
assert(!(o->op_private & ~PL_op_private_valid[type]));
851906
}
852907
#endif
853908

854-
if (o->op_private & OPpREFCOUNTED) {
855-
switch (type) {
856-
case OP_LEAVESUB:
857-
case OP_LEAVESUBLV:
858-
case OP_LEAVEEVAL:
859-
case OP_LEAVE:
860-
case OP_SCOPE:
861-
case OP_LEAVEWRITE:
862-
{
863-
PADOFFSET refcnt;
864-
OP_REFCNT_LOCK;
865-
refcnt = OpREFCNT_dec(o);
866-
OP_REFCNT_UNLOCK;
867-
if (refcnt) {
868-
/* Need to find and remove any pattern match ops from the list
869-
we maintain for reset(). */
870-
find_and_forget_pmops(o);
871-
continue;
872-
}
873-
}
874-
break;
875-
default:
876-
break;
877-
}
878-
}
879909

880910
/* Call the op_free hook if it has been set. Do it now so that it's called
881911
* at the right time for refcounted ops, but still before all of the kids
882912
* are freed. */
883913
CALL_OPFREEHOOK(o);
884914

885-
if (o->op_flags & OPf_KIDS) {
886-
OP *kid, *nextkid;
887-
assert(cUNOPo->op_first); /* OPf_KIDS implies op_first non-null */
888-
for (kid = cUNOPo->op_first; kid; kid = nextkid) {
889-
nextkid = OpSIBLING(kid); /* Get before next freeing kid */
890-
if (kid->op_type == OP_FREED)
891-
/* During the forced freeing of ops after
892-
compilation failure, kidops may be freed before
893-
their parents. */
894-
continue;
895-
if (!(kid->op_flags & OPf_KIDS))
896-
/* If it has no kids, just free it now */
897-
op_free(kid);
898-
else
899-
DEFER_OP(kid);
900-
}
901-
}
902915
if (type == OP_NULL)
903916
type = (OPCODE)o->op_targ;
904917

@@ -915,11 +928,10 @@ Perl_op_free(pTHX_ OP *o)
915928
FreeOp(o);
916929
if (PL_op == o)
917930
PL_op = NULL;
918-
} while ( (o = POP_DEFERRED_OP()) );
919-
920-
DEFER_OP_CLEANUP;
931+
}
921932
}
922933

934+
923935
/* S_op_clear_gv(): free a GV attached to an OP */
924936

925937
STATIC

0 commit comments

Comments
 (0)