Bleadperl v5.19.4-10-g15a4d87 breaks PJCJ/Devel-Cover-1.08.tar.gz #13298
Comments
From @andkgit bisect commit 15a4d87 Optimise if/unless wrt OP_AND/OP_OR/OP_DOR. Also optimise OP_OR/OP_DOR chains. diagnostics Also affected: Mojolicious report already on cpantesters: http://www.cpantesters.org/cpan/report/0182b0e0-231f-11e3-bf45-b0dda0a565e3 Devel::Cover not yet there. perl -V Summary of my perl5 (revision 5 version 19 subversion 5) configuration: Characteristics of this binary (from libperl): -- |
From @wolfsageI see what's wrong with the Mojolicious failure. The line in question is: return sub { $ignore // 1 and shift; $self->_step($id, @_) }; My optimisation assumed OP_DOR behaved like OP_OR and returned truth. Instead OP_DOR evaluates the left operand for definedness, and if Attached is a patch to fix this. The optree now would look like: $ ./perl -Ilib/ -MO=Concise -e 'if ($z // $x || $x // $z || 1) {}' Also, I believe the Devel::Cover failure is because we're now skipping Cheers, -- Matthew Horsfall (alh) |
From @wolfsage0001-OP_DOR-to-a-void-OP_AND-cannot-short-circuit.patchFrom 18b0835101db19d0daf9e6b4d46b5834c7f62486 Mon Sep 17 00:00:00 2001
From: Matthew Horsfall (alh) <wolfsage@gmail.com>
Date: Sun, 22 Sep 2013 11:10:06 -0400
Subject: [PATCH] OP_DOR to a void OP_AND cannot short circuit.
OP_DOR returns the value of the underlying object on success, not
true, so it must be evaluated by the next op and cannot be skipped.
Also, wrap IS_AND_OP with parens to be safer for use in the future.
---
op.c | 14 +++++++++-----
t/op/dor.t | 27 ++++++++++++++++++++++++++-
2 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/op.c b/op.c
index 49de893..b87a55f 100644
--- a/op.c
+++ b/op.c
@@ -10949,7 +10949,8 @@ S_inplace_aassign(pTHX_ OP *o) {
defer_queue[(defer_base + ++defer_ix) % MAX_DEFERRED] = o; \
} STMT_END
-#define IS_AND_OP(o) o->op_type == OP_AND
+#define IS_AND_OP(o) (o->op_type == OP_AND)
+#define IS_OR_OP(o) (o->op_type == OP_OR)
#define IS_ORISH_OP(o) (o->op_type == OP_OR || o->op_type == OP_DOR)
/* A peephole optimizer. We visit the ops in the order they're to execute.
@@ -11425,12 +11426,15 @@ Perl_rpeep(pTHX_ OP *o)
|| o->op_next->op_type == OP_NULL))
o->op_next = o->op_next->op_next;
}
- /* if we're an OR/DOR and our next is a AND in void context, we'll
- follow it's op_other on short circuit, same for reverse */
+ /* if we're an OR and our next is a AND in void context, we'll
+ follow it's op_other on short circuit, same for reverse.
+ We can't do this with OP_DOR since if it's true, its return
+ value is the underlying value which must be evaluated
+ by the next op */
if (o->op_next &&
(
- (IS_AND_OP(o) && IS_ORISH_OP(o->op_next))
- || (IS_ORISH_OP(o) && IS_AND_OP(o->op_next))
+ (IS_AND_OP(o) && IS_OR_OP(o->op_next))
+ || (IS_OR_OP(o) && IS_AND_OP(o->op_next))
)
&& (o->op_next->op_flags & OPf_WANT) == OPf_WANT_VOID
) {
diff --git a/t/op/dor.t b/t/op/dor.t
index e2385f1..d14ad96 100644
--- a/t/op/dor.t
+++ b/t/op/dor.t
@@ -10,7 +10,7 @@ BEGIN {
package main;
require './test.pl';
-plan( tests => 31 );
+plan( tests => 34 );
my($x);
@@ -74,3 +74,28 @@ like( $@, qr/^Search pattern not terminated/,
is(0 // 2, 0, ' // : left-hand operand not optimized away');
is('' // 2, '', ' // : left-hand operand not optimized away');
is(undef // 2, 2, ' // : left-hand operand optimized away');
+
+# Test that OP_DORs other branch isn't run when arg is defined
+# // returns the value if its defined, and we must test its
+# truthness after
+my $x = 0;
+my $y = 0;
+
+$x // 1 and $y = 1;
+is($y, 0, 'y is still 0 after "$x // 1 and $y = 1"');
+
+$y = 0;
+# $x is defined, so its value 0 is returned to the if block
+# and the block is skipped
+if ($x // 1) {
+ $y = 1;
+}
+is($y, 0, 'if ($x // 1) exited out early since $x is defined and 0');
+
+# Test or/dor chain doesn't break dor
+$y = 0;
+
+if ($z // $x || $x // $z || 1) {
+ $y = 1;
+}
+is($y, 0, 'Chained or/dor behaves correctly');
--
1.7.0.4
|
|
The RT System itself - Status changed from 'new' to 'open' |
From @wolfsageOn Sun, Sep 22, 2013 at 11:32 AM, Matthew Horsfall (alh)
I'm not sure of the normal policy here, but with my last patch Should we remove 15a4d87 Thanks, -- Matthew Horsfall (alh) |
From @cpansproutOn Mon Sep 30 07:12:41 2013, alh wrote:
I’m not currently in a position to apply (or revert) anything, so if -- Father Chrysostomos |
From @khwilliamsonOn 09/30/2013 09:38 AM, Father Chrysostomos via RT wrote:
I'll do this; currently the reversion is testing on my machine |
From @khwilliamsonOn 09/30/2013 09:58 AM, Karl Williamson wrote:
Now done with commit 79d51b8 |
From @wolfsageOn Mon, Sep 30, 2013 at 12:08 PM, Karl Williamson
Thanks, I'd love to see these two patches make it in still, should we (If we do I'll squash the two commits) -- Matthew Horsfall (alh) |
From @wolfsageOn Mon Sep 30 10:10:04 2013, alh wrote:
I've done this and fixed another issue. See https://rt-archive.perl.org/perl5/Ticket/Display.html?id=120128 Closing. -- Matthew Horsfall (alh) |
|
@wolfsage - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#119939 (status was 'resolved')
Searchable as RT119939$
The text was updated successfully, but these errors were encountered: