RT#133131: pp_hot.c: deoptimise pp_iter() when non-standard OP_AND op_ppaddr

Commit 7c114860c0fa8ade5e00a4b609d2fbd11d5a494c introduced an optimisation
in pp_iter(). Before the optimisation, pp_iter() pushed either &PL_SV_yes or
&PL_sv_no to the stack, and returned the op_next in the obvious way.

The optimisation takes advantage of the fact that the op_next of an OP_ITER
always points to an OP_AND node, so pp_iter() now directly jumps to either
the op_next or the op_other of the OP_AND as appropriate.

The commit message for the optimisation also says this:

    It's possible that some weird optree-munging XS module may break this
    assumption. For now I've just added asserts that the next op is OP_AND
    with an op_ppaddr of Perl_pp_and; if that assertion fails, it may be
    necessary to convert pp_iter()s' asserts into conditional statements.

However, Devel::Cover does change the op_ppaddr of the ops it can see, so
the assertions on op_ppaddr were being tripped when Devel::Cover was run
under a -DDEBUGGING Perl. But even if the asserts didn't trip, skipping the
OP_AND nodes would prevent Devel::Cover from determining branch coverage in
the way that it wants.

This commit converts the asserts into conditional statements, as outlined in
the commit message above, and undoes the optimisation when the op_ppaddr
doesn't match.

1 files changed, 23 insertions, 7 deletions

diff --git a/pp_hot.c b/pp_hot.c
index ae81e940df..56e3cbe6e1 100644
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -3998,25 +3998,41 @@ PP(pp_iter)
 	DIE(aTHX_ "panic: pp_iter, type=%u", CxTYPE(cx));
     }
 
-    /* Bypass pushing &PL_sv_yes and calling pp_and(); instead
+    /* Try to bypass pushing &PL_sv_yes and calling pp_and(); instead
      * jump straight to the AND op's op_other */
     assert(PL_op->op_next->op_type == OP_AND);
-    assert(PL_op->op_next->op_ppaddr == Perl_pp_and);
-    return cLOGOPx(PL_op->op_next)->op_other;
+    if (PL_op->op_next->op_ppaddr == Perl_pp_and) {
+        return cLOGOPx(PL_op->op_next)->op_other;
+    }
+    else {
+        /* An XS module has replaced the op_ppaddr, so fall back to the slow,
+         * obvious way. */
+        /* pp_enteriter should have pre-extended the stack */
+        EXTEND_SKIP(PL_stack_sp, 1);
+        *++PL_stack_sp = &PL_sv_yes;
+        return PL_op->op_next;
+    }
 
   retno:
-    /* Bypass pushing &PL_sv_no and calling pp_and(); instead
+    /* Try to bypass pushing &PL_sv_no and calling pp_and(); instead
      * jump straight to the AND op's op_next */
     assert(PL_op->op_next->op_type == OP_AND);
-    assert(PL_op->op_next->op_ppaddr == Perl_pp_and);
     /* pp_enteriter should have pre-extended the stack */
     EXTEND_SKIP(PL_stack_sp, 1);
     /* we only need this for the rare case where the OP_AND isn't
      * in void context, e.g. $x = do { for (..) {...} };
-     * but its cheaper to just push it rather than testing first
+     * (or for when an XS module has replaced the op_ppaddr)
+     * but it's cheaper to just push it rather than testing first
      */
     *++PL_stack_sp = &PL_sv_no;
-    return PL_op->op_next->op_next;
+    if (PL_op->op_next->op_ppaddr == Perl_pp_and) {
+        return PL_op->op_next->op_next;
+    }
+    else {
+        /* An XS module has replaced the op_ppaddr, so fall back to the slow,
+         * obvious way. */
+        return PL_op->op_next;
+    }
 }