Skip to content

Commit

Permalink
fix Perl_rpp_is_lone() for immortals.
Browse files Browse the repository at this point in the history
Perl_rpp_is_lone() checks whether an SV is kept alive only by the args
and/or temps stack, and thus can in various circumstances be used
directly rather than copying - such as a return value from a function, or
assigning a value to an array or hash.

However, this assumption fails on immortal SVs like PL_sv_undef. Their
refcount can reach 1 but they should still be copied rather than stolen.

This showed itself as an extremely intermittent failure in

    cpan/Test-Harness/t/compat/test-harness-compat.t

It only showed up when both these conditions were met:

- on DEBUGGING builds, because that sets the initial ref count of the
immortals to 1000 rather than I32_MAX, making such problems easier to
reproduce; *and*

- under PERL_RC_STACK, because only on that build do you have SVs
on the args stack (but not temps stack) with RC==1 which are stealable.
Also under the RC build, as an optimisation, &PL_sv_undef is often
pushed on the stack without incrementing its RC. Thus it is more liklely
to reach RC==1.

I don't know why the failure on that test was intermittent, even after
setting PERL_HASH_SEED=0. Perhaps it was timing related. Certainly
anything which made it slower, such as strace, valgrind or ASAN, made
the problem go away or fail only after days rather than minutes.

This commit also decreases the value of SvREFCNT_IMMORTAL on
debugging builds from 1000 to 10 under PERL_RC_STACK, to make such
issues/bugs more likely to be triggered.
  • Loading branch information
iabyn committed Nov 16, 2023
1 parent ad706e0 commit 0be1aee
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 4 deletions.
7 changes: 4 additions & 3 deletions inline.h
Expand Up @@ -1040,9 +1040,9 @@ Indicates whether the stacked SV C<sv> (assumed to be not yet popped off
the stack) is only kept alive due to a single reference from the argument
stack and/or and the temps stack.
This can used for example to decide whether the copying of return values in rvalue
context can be skipped, or whether it shouldn't be assigned to in lvalue
context.
This can used for example to decide whether the copying of return values
in rvalue context can be skipped, or whether it shouldn't be assigned to
in lvalue context.
=cut
*/
Expand All @@ -1063,6 +1063,7 @@ Perl_rpp_is_lone(pTHX_ SV *sv)
return SvREFCNT(sv) <= cBOOL(SvTEMP(sv))
#ifdef PERL_RC_STACK
+ 1
&& !SvIMMORTAL(sv) /* PL_sv_undef etc are never stealable */
#endif
;
}
Expand Down
12 changes: 11 additions & 1 deletion sv.h
Expand Up @@ -2415,7 +2415,17 @@ that already have a PV buffer allocated, but no SvTHINKFIRST.

#ifdef DEBUGGING
/* exercise the immortal resurrection code in sv_free2() */
# define SvREFCNT_IMMORTAL 1000
# ifdef PERL_RC_STACK
/* When the stack is ref-counted, the code tends to take a lot of
* short cuts with immortals, such as skipping the bump of the ref
* count of PL_sv_undef when pushing it on the stack. Exercise that
* this doesn't cause problems, especially on code which
* special-cases RC==1 etc.
*/
# define SvREFCNT_IMMORTAL 10
# else
# define SvREFCNT_IMMORTAL 1000
# endif
#else
# define SvREFCNT_IMMORTAL ((~(U32)0)/2)
#endif
Expand Down
21 changes: 21 additions & 0 deletions t/op/hash.t
Expand Up @@ -259,6 +259,27 @@ package Magic {
::is(join( ':', %inner), "x:y", "magic keys");
}

# Make sure PL_sv_undef is copied, and not stored directly, when assigning
# to a hash. This failed on DEBUGGING + PERL_RC_STACK builds because the
# test for a lone SV assumed that an SV on the stack with a ref count of 1
# could be used directly rather than copied. However, PL_sv_undef and
# friends could reach a ref count of 1 but still not be stealable.
#
# A DEBUGGING build sets the initial ref count of the immortals to 1000
# rather than I32_MAX, making such problems easier to reproduce.

{
my $bad = 0;
for (1..1001) {
# Each iteration may leave the RC of PL_sv_undef one lower.
my %h = ('a', undef);
# this could fail with
# "Modification of non-creatable hash value attempted ..."
eval { $h{a} = 1; };
$bad = 1 if $@ ne "";
}
ok(!$bad, "PL_sv_undef RC 1");
}


done_testing();

0 comments on commit 0be1aee

Please sign in to comment.