On Fri, Dec 03, 2004 at 10:32:14AM +0100, Rafael Garcia-Suarez wrote:
> Steve Peters wrote:
> >
> > Change 23418 by [EMAIL PROTECTED] on 2004/10/23 21:50:19
> >
> > [perl #32039] Chained goto &sub drops data too early.
> >
> > Change 22373 to stop a memory leak in goto &foo intead caused
> > the elements of @_ to be freed too early. This revised fix
> > just transfers the reifiedness of the old @_ to the new @_
>
> Reverting this one makes Spiffy's regression tests pass (with blead).
>
> Dave ! I don't understand @_ reification !
I think the behaviour of 5.8.6 is actually the correct one (for some
twisted interpretation of 'correct'). Or to put it another way, the bug
resides in caller()'s setting of DB::args rather than in goto.
First a bit of background on reification (aka mangling of the English
language).
Most arrays are AvREAL, which means that their elements are refcounted;
so for example, when an array is freed, all its elements also have their
refcount decremented, possibly freeing them up too.
Some special arrays, notably the stack and @_, have the AvREAL flag turned
off, meaning that their elements aren't refcounted. This is for speed and
efficiency (and as we all know, is an endlesss source of bugs).
This however is problematic with @_, as it can be modifed; eg C<push @_, 1>;
in this case we might end up with the last element of @_ refcounted, but
none of the others. To avoid this problem, @_ has the AVf_REIFY flag set.
This tells all the functions in av.c that if they modify such an array (ie
one that has (!AVf_REAL && AVf_REIFY), then they should first bump up the
refcount of each element by one, then turn on the AVf_REAL flag; this is
"reification" of the array. From now on @_ behaves like a normal array,
and needs special handling at the end of a function call or in an &goto,
to clean up its children.
Now we come onto an interesting quirk of caller(). When it attempts to
copy the caller's @_ into DB::args, it uses AvALLOC rather than AvARRAY;
ie it attempts, as far as possible, to grab the original state of the
caller's args, rather than the current state. For example, in the
following code
f(1..3);
sub f {
shift;
{ package DB; my @dummy = caller(0); }
use Data::Dumper; print Dumper(@DB::args);
}
we get in both 5.8.5 and 5.8.6:
$VAR1 = 1;
$VAR2 = 2;
$VAR3 = 3;
note that the '1' has been magically resurrected. It happens to still
exist because @_ is not real, so doing a shift doesn't decrement the 1's
refcount. Now if we modify the code slightly by assigning to @_ first,
f(1..3);
sub f {
@_ = (4..6);
shift;
{ package DB; my @dummy = caller(0); }
use Data::Dumper; print Dumper(@DB::args);
}
we get in both 5.8.5 and 5.8.6:
$VAR1 = undef;
$VAR2 = 5;
$VAR3 = 6;
because now the shift causes a refcount decrement of the '4' SV, so in
fact a freed SV pointer is being stored in DB::args[0]. (Hint: this is
a bug.)
In the following, which is essentially the OP's code:
f(1..3);
sub f { @_ = (4..6); goto &g }
sub g {
shift;
{ package DB; my @dummy = caller(0); }
use Data::Dumper; print Dumper(@DB::args);
}
$VAR1 is undef in 5.8.6, but is 4 in 5.8.5. This is because my patch in
5.8.6 maintains the reifiedness state of @_ as it is passed between two
subs in goto &foo. Doing this avoids problems with leaking or premature
freeing of @_ elements in nested gotos. I think this behaviour should
remain.
Meanwhile, there are several things that need to be, or may need to be
fixed in caller().
The first is that when caller() populates @DB::args from a reified @_
whose AvARRAY != AvALLOC, then the first elements shouldn't point to
freed SVs.
Second, it could be argued that the DB::args behaviour of displaying the
original @_ of the caller, rather than its current state after shifts etc,
is wrong. In that case, it should either keep the original indices but
replace shifted elements with undefs even in the non-reified case, or
just show the current @_, ie don't preseve indices.
The goal with one of these last two suggestions is to remove the
difference in behaviour seeen in DB::args depending on whether @_ has been
reified or not. ie
@_ = (1,2,3); shift @_; caller(0);
should *always* set @DB::args to either (undef,2,3) or (2,3), but never
to (1,2,3), regardless fo the reifiedness of @_.
Dave.
--
Justice is when you get what you deserve.
Law is when you get what you pay for.
