This email summarises some of my recent patching activity
relating to
fixing leaks, especially #29636 - just applied, that should
fix recent
problems caused by the earlier fixes.
Background:
As parsing proceeds, little op trees are pushed onto the
parse stack.
As the parsing progresses, reduce operations pop N items off
the parse
stack and replace therm with a single OP tree, which is
typically a larger
tree that incorporates the just-popped subtrees. If parsing
runs to
completion (eg compiling a sub or file), you end up with a
single large
optree that is then attached to a CV.
Problem:
If parsing runs into some sort of error, parsing is
abandoned, and any
optrees left on the parse stack are leaked. This is the main
cause of the
infamous eval "syntax error" leaks.
There are two main types of error. If the parser itself
detects a syntax
error, it pops items off the parse stack until it reaches a
sane state (eg
a statement boundary) and continues. The second type is if
something
croaks; either the lexer or parser noticing something wrong,
or code
equivalent to BEGIN (eg a C<use> that
generates an error).
Solution:
The first type of leak was fixed by #28319, which simply
added code to
free ops that were being popped during error recovery, and
the second was
(mostly) fixed by #28319, which added a SAVE function that
freed all ops
on the stack on die. However, both these patches had a fatal
flaw in that
in ithreads builds, ops may point to pad slots which need
freeing, but
when compiling nested subs, eg sub { sub { sub { ...,
there's no
guarantee that PL_comppad currently points to the pad
associated with the
op being freed.
This flaw was fixed by recording the current value of
PL_comppad for each
op on the parse stack; this was done in changes #29501 and
#29504.
This all seems to work and be robust. However, there was a
final
unresolved issue related to pooping the stack during a die,
and that was
what happens in the middle of a reduce. During a reduce, the
top N items
on the parse stack are passed to some function which, most
likely,
combines those elements into a singe tree and returns the
result, which
is then pushed onto the parse stack. If that function croaks
for some
reason, then in general, you don't know which of the N ops
have been freed,
or attached to the other ops, or (in the case of
newATTRSUB), attached
to PL_compcv, and in the further case of BEGIN, whether
PL_compcv has
already been freed.
My earlier changes skipped this problem by not attempting to
free the
top N parser stack items, this accepting a certain amount of
leakage.
Change #29543 (with further fix #29547) attempted to solve
this problem
by adding two new OP flags, op_latefree and op_latefreed,
which
effectively converts op_free() into a clear operation father
than a free
operation, and thus allows the same op to be safely
op_free()ed multiple
times.
This fix has a fatal flaw in that, as mentioned above, the
op tree may or
may not have been attached PL_compcv, and PL_compcv may or
may not have
been freed. This has been causing double frees in MM_VMS.t,
and is
possibly responsible for some of the other problems seen
recently. Change
#29636, committed this evening, fixes this by adding a third
new flag,
op_attached. This seems to fix the problem.
However, I seem to approaching the waterbed area of leak
fixing, where each
of my fixes just throws up a new problem. If there are any
more problems
with this fix, I intend to just retract it completely (ie
#29543 and
#29636), and leave 5.10 with the minor leak.
Note also that this fix uses three of the 5 spare OP flags,
which is
rather expensive.
This fix is somewhat fragile. A more robust fix would be to
refcnt OPs:
this only requires a 2-bit ref counter per op (so the 3 bits
I've stolen
would become 2 bits), but would require all OP-manipulating
code (especially
the 8K lines of op.c) to be updated and/or reviewed so that
any time an OP
points to another OP, its refcnt is adjusted. It would also
break any
external code that manipulates optrees. Certainly not
something to be
contemplated before the 5.10 release.
Dave.
--
"I do not resent criticism, even when, for the sake of
emphasis,
it parts for the time with reality".
-- Winston Churchill, House of Commons, 22nd Jan 1941.
|
iabyn.com> wrote:
> This fix is somewhat fragile. A more robust fix would
be to refcnt OPs:
> this only requires a 2-bit ref counter per op (so the 3
bits I've stolen
Why is the refcounter two bits? There can't be more than
three
references to the same opnode?
Josh