This is a live mirror of the Perl 5 development currently hosted at https://github.com/perl/perl5
fix *_ = "" for 0 .. 1;
authorDavid Mitchell <davem@iabyn.com>
Fri, 18 Dec 2015 15:09:42 +0000 (15:09 +0000)
committerDavid Mitchell <davem@iabyn.com>
Wed, 3 Feb 2016 09:18:35 +0000 (09:18 +0000)
RT #123994

pp_iter couldn't handle GvSv(gv) being NULL.

In that ticket, Tony Cook suggested two possible fixes. First,
always instantiate the GvSV slot at the start of pp_iter by using
GvSVn rather than GvSV in the CxITERVAR() macro;

Second, test for it being null within the two 'range' branches,
(for(1..9), for('a'..'z')), and if so create it. One advantage of doing
it there is that there's already code for (re)creating the SV if the
reference count is != 1. It also means that the list and array cases
(for(@a), for(1,3,5)) which always put the next iterated SV into the
pad/GvSV slot don't waste time creating and then immediately discarding an
SV if GvSV was NULL.

I went for the second fix.

It also means that's there's no longer any need in pp_enteriter to
initially poulate GvSV is it was null, as this will be detected during
the first pp_iter() anyway.

pp_ctl.c
pp_hot.c
t/op/for.t

index 43dfe3c..9e7888b 100644 (file)
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -2123,12 +2123,8 @@ PP(pp_enteriter)
        SV * const sv = POPs;
        itervarp = (void *)sv;
         if (LIKELY(isGV(sv))) {                /* symbol table variable */
        SV * const sv = POPs;
        itervarp = (void *)sv;
         if (LIKELY(isGV(sv))) {                /* symbol table variable */
-            SV** svp = &GvSV(sv);
-            itersave = *svp;
-            if (LIKELY(itersave))
-                SvREFCNT_inc_simple_void_NN(itersave);
-            else
-                *svp = newSV(0);
+            itersave = GvSV(sv);
+            SvREFCNT_inc_simple_void(itersave);
             cxflags = CXp_FOR_GV;
         }
         else {                          /* LV ref: for \$foo (...) */
             cxflags = CXp_FOR_GV;
         }
         else {                          /* LV ref: for \$foo (...) */
index 9d0b1e2..28360e7 100644 (file)
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -2647,7 +2647,11 @@ PP(pp_iter)
             goto retno;
 
         oldsv = *itersvp;
             goto retno;
 
         oldsv = *itersvp;
-        if (LIKELY(SvREFCNT(oldsv) == 1 && !SvMAGICAL(oldsv))) {
+        /* NB: on the first iteration, oldsv will have a ref count of at
+         * least 2 (one extra from blk_loop.itersave), so the GV or pad
+         * slot will get localised; on subsequent iterations the RC==1
+         * optimisation may kick in and the SV will be reused. */
+         if (oldsv && LIKELY(SvREFCNT(oldsv) == 1 && !SvMAGICAL(oldsv))) {
             /* safe to reuse old SV */
             sv_setsv(oldsv, cur);
         }
             /* safe to reuse old SV */
             sv_setsv(oldsv, cur);
         }
@@ -2657,7 +2661,7 @@ PP(pp_iter)
              * completely new SV for closures/references to work as
              * they used to */
             *itersvp = newSVsv(cur);
              * completely new SV for closures/references to work as
              * they used to */
             *itersvp = newSVsv(cur);
-            SvREFCNT_dec_NN(oldsv);
+            SvREFCNT_dec(oldsv);
         }
         if (strEQ(SvPVX_const(cur), max))
             sv_setiv(cur, 0); /* terminate next time */
         }
         if (strEQ(SvPVX_const(cur), max))
             sv_setiv(cur, 0); /* terminate next time */
@@ -2673,8 +2677,8 @@ PP(pp_iter)
            goto retno;
 
         oldsv = *itersvp;
            goto retno;
 
         oldsv = *itersvp;
-       /* don't risk potential race */
-       if (LIKELY(SvREFCNT(oldsv) == 1 && !SvMAGICAL(oldsv))) {
+       /* see NB comment above */
+       if (oldsv && LIKELY(SvREFCNT(oldsv) == 1 && !SvMAGICAL(oldsv))) {
            /* safe to reuse old SV */
            sv_setiv(oldsv, cur);
        }
            /* safe to reuse old SV */
            sv_setiv(oldsv, cur);
        }
@@ -2684,7 +2688,7 @@ PP(pp_iter)
             * completely new SV for closures/references to work as they
             * used to */
            *itersvp = newSViv(cur);
             * completely new SV for closures/references to work as they
             * used to */
            *itersvp = newSViv(cur);
-           SvREFCNT_dec_NN(oldsv);
+           SvREFCNT_dec(oldsv);
        }
 
        if (UNLIKELY(cur == IV_MAX)) {
        }
 
        if (UNLIKELY(cur == IV_MAX)) {
index ef60f4d..053154c 100644 (file)
@@ -5,7 +5,7 @@ BEGIN {
     require "./test.pl";
 }
 
     require "./test.pl";
 }
 
-plan(112);
+plan(124);
 
 # A lot of tests to check that reversed for works.
 
 
 # A lot of tests to check that reversed for works.
 
@@ -631,3 +631,43 @@ is(fscope(), 1, 'return via loop in sub');
         ok(!defined $foo, "NULL GvSV int iterator");
     }
 }
         ok(!defined $foo, "NULL GvSV int iterator");
     }
 }
+
+# RT #123994 - handle a null GVSV within a loop
+
+{
+    local *foo;
+    local $foo = "outside";
+
+    my $i = 0;
+    for $foo (0..1) {
+        is($foo, $i, "RT #123994 int range $i");
+        *foo = "";
+        $i++;
+    }
+    is($foo, "outside", "RT #123994 int range outside");
+
+    $i = 0;
+    for $foo ('0'..'1') {
+        is($foo, $i, "RT #123994 str range $i");
+        *foo = "";
+        $i++;
+    }
+    is($foo, "outside", "RT #123994 str range outside");
+
+    $i = 0;
+    for $foo (0, 1) {
+        is($foo, $i, "RT #123994 list $i");
+        *foo = "";
+        $i++;
+    }
+    is($foo, "outside", "RT #123994 list outside");
+
+    my @a = (0,1);
+    $i = 0;
+    for $foo (@a) {
+        is($foo, $i, "RT #123994 array $i");
+        *foo = "";
+        $i++;
+    }
+    is($foo, "outside", "RT #123994 array outside");
+}