Don’t share TARGs between recursive ops

I had to change the definition of IS_PADCONST to account for the
SVf_IsCOW flag.  Previously, anything marked READONLY would be consid-
ered a pad constant, to be shared by pads of different recursion lev-
els.  Some of those READONLY things were not actually read-only, as
they were copy-on-write scalars, which are never read-only.  So I
changed the definition of IS_PADCONST in e3918bb703c to accept COWs
as well as read-only scalars, since I was removing the READONLY flag
from COWs.

With the new copy-on-write scheme, it is easy for a TARG to turn into
a COW.  If that happens and then the same subroutine calls itself
recursively for the first time after that, pad_push will see that this
is a pad ‘constant’ and allow the next recursion level to share it.

If pp_concat calls itself recursively, the recursive call can modify
the scalar the outer call is in the middle of using, causing the
return value to be doubled up (‘tmptmp’) in the test case added here.

Since pad constants are marked PADTMP (I would like to change that
eventually), there is no way to distinguish them from TARGs when the
are COWs, except for the fact that pad constants that are COWs are
always shared hash keys (SvLEN==0).

diff --git a/op.h b/op.h
index 935e126..97228b1 100644 (file)
--- a/op.h
+++ b/op.h
@@ -570,7 +570,8 @@ struct loop {
 #  define      cGVOPx_gv(o)    ((GV*)PAD_SVl(cPADOPx(o)->op_padix))
 #  define      IS_PADGV(v)     (v && SvTYPE(v) == SVt_PVGV && isGV_with_GP(v) \
                                 && GvIN_PAD(v))
-#  define      IS_PADCONST(v)  (v && (SvREADONLY(v) || SvIsCOW(v)))
+#  define      IS_PADCONST(v) \
+       (v && (SvREADONLY(v) || (SvIsCOW(v) && !SvLEN(v))))
 #  define      cSVOPx_sv(v)    (cSVOPx(v)->op_sv \
                                 ? cSVOPx(v)->op_sv : PAD_SVl((v)->op_targ))
 #  define      cSVOPx_svp(v)   (cSVOPx(v)->op_sv \

diff --git a/t/op/concat2.t b/t/op/concat2.t
index 36b62bc..66a7d05 100644 (file)
--- a/t/op/concat2.t
+++ b/t/op/concat2.t
@@ -11,7 +11,7 @@ BEGIN {
     require './test.pl';
 }
 
-plan 2;
+plan 3;
 
 SKIP: {
 skip_if_miniperl("no dynamic loading on miniperl, no Encode", 1);
@@ -31,3 +31,25 @@ $x->[0] = "\xff";
 $x.= chr 257;
 $x.= chr 257;
 is $x, "\xff\x{101}\x{101}", '.= is not confused by changing utf8ness';
+
+# Ops should not share the same TARG between recursion levels.  This may
+# affect other ops, too, but concat seems more susceptible to this than
+# others, since it can call itself recursively.  (Where else would I put
+# this test, anyway?)
+fresh_perl_is <<'end', "tmp\ntmp\n", {},
+ sub canonpath {
+     my ($path) = @_;
+     my $node = '';
+     $path =~ s|/\z||;
+     return "$node$path";
+ }
+ 
+ {
+  package Path::Class::Dir;
+  use overload q[""] => sub { ::canonpath("tmp") };
+ }
+ 
+ print canonpath("tmp"), "\n";
+ print canonpath(bless {},"Path::Class::Dir"), "\n";
+end
+ "recursive concat does not share TARGs";