In perl.git, the branch blead has been updated <http://perl5.git.perl.org/perl.git/commitdiff/f15d05806fb7522031b75cb5a8784727ae03b98a?hp=adec5bbf0b66ee5ebc2ba80eda2389bb68e23d86>
- Log ----------------------------------------------------------------- commit f15d05806fb7522031b75cb5a8784727ae03b98a Author: Aaron Crane <[email protected]> Date: Sun May 15 15:11:12 2016 +0100 [perl #127952] misoptimization for negated constant-ish on lhs of logop Negations were being incorrectly deleted from the op tree for an OP_AND or OP_OR (corresponding to Perl code with any of `&& || and or`, or postfix "if" or "unless") whose left-hand side looks like "!BAREWORD" or "!do { 'const' }" and whose right-hand side is a non-constant-foldable negation. The symptom in the reported case was an assertion failure in ck_refassign for an srefgen op, caused by such an OP_NOT having been nulled. But other cases exist that instead yielded incorrect results. The underlying cause is that two optimisations in S_new_logop() were partially interfering with each other. One of those attempts to optimise code like "!$x && !$y" to the equivalent of "!($x || $y)", saving a negation op; this is valid by De Morgan's laws. If it detects code of this form, it nulls out the negations on each side of the "&&", and makes a note to wrap the op it generates inside a new OP_NOT. The other optimisation looks at the left-hand arm, and if it's a constant at compile time, avoids the entire logop in favour of directly evaluating the lhs or rhs as appropriate, and eliding whichever arm is no longer needed. This optimisation is important for code like this: use constant DEBUG => â¦; print_debug_output() if DEBUG; because it allows the entire statement to be eliminated when DEBUG is false. When both conditions were true simultaneously, the De Morgan optimisation was applied before the constant-based arm elision. But the arm elision involved returning early from S_new_logop(), so the code later in that function that wraps the generated op in a new OP_NOT never had a chance to run. This meant that "!X && !Y" when X is constant was being compiled as if it were in fact "X || Y", which is clearly incorrect. This is, however, a very rare situation: it requires the lhs to be an OP_NOT that dominates an OP_CONST (possibly with some intervening OP_LINESEQ or similar). But OP_NOT is constant-foldable, so that doesn't normally happen. The two ways for it to happen are: - The constant is a bareword (since even though barewords are constants, they don't currently participate in constant folding) - The constant is hidden inside one or more layers of do{} (since that serves as a barrier to constant folding, but the arm-elision optimisation is smart enough to search recursively through the optree for the real constant) The fix is much simpler than the explanation: apply the optimisations in the opposite order, so that when arm elision returns early, the negation ops haven't yet been nulled. M op.c M t/op/lop.t commit 1c00b880613d955236e052aefc52b7eb1fac5dc9 Author: Aaron Crane <[email protected]> Date: Sun May 15 16:38:48 2016 +0100 op.c: add some explanatory comments to S_new_logop() M op.c commit 396ce25fb7d85d57d481752ea9f9e4f33fcbcfde Author: Aaron Crane <[email protected]> Date: Sun May 15 12:21:07 2016 +0100 Delete dead null-pointer check in op.c Code above this point has already used this variable without checking whether it's null, so this can't accomplish anything. M op.c commit bf470003395f7172463f4fa9109e0853ad00272a Author: Aaron Crane <[email protected]> Date: Sat May 14 23:29:45 2016 +0100 Fix misleading indentation in op.c M op.c ----------------------------------------------------------------------- Summary of changes: op.c | 47 +++++++++++++++++++++++++---------------------- t/op/lop.t | 28 +++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 23 deletions(-) diff --git a/op.c b/op.c index 93205fe..9e643e0 100644 --- a/op.c +++ b/op.c @@ -6747,24 +6747,7 @@ S_new_logop(pTHX_ I32 type, I32 flags, OP** firstp, OP** otherp) || type == OP_CUSTOM); scalarboolean(first); - /* optimize AND and OR ops that have NOTs as children */ - if (first->op_type == OP_NOT - && (first->op_flags & OPf_KIDS) - && ((first->op_flags & OPf_SPECIAL) /* unless ($x) { } */ - || (other->op_type == OP_NOT)) /* if (!$x && !$y) { } */ - ) { - if (type == OP_AND || type == OP_OR) { - if (type == OP_AND) - type = OP_OR; - else - type = OP_AND; - op_null(first); - if (other->op_type == OP_NOT) { /* !a AND|OR !b => !(a OR|AND b) */ - op_null(other); - prepend_not = 1; /* prepend a NOT op later */ - } - } - } + /* search for a constant op that could let us fold the test */ if ((cstop = search_const(first))) { if (cstop->op_private & OPpCONST_STRICT) @@ -6774,6 +6757,7 @@ S_new_logop(pTHX_ I32 type, I32 flags, OP** firstp, OP** otherp) if ((type == OP_AND && SvTRUE(((SVOP*)cstop)->op_sv)) || (type == OP_OR && !SvTRUE(((SVOP*)cstop)->op_sv)) || (type == OP_DOR && !SvOK(((SVOP*)cstop)->op_sv))) { + /* Elide the (constant) lhs, since it can't affect the outcome */ *firstp = NULL; if (other->op_type == OP_CONST) other->op_private |= OPpCONST_SHORTCIRCUIT; @@ -6791,6 +6775,9 @@ S_new_logop(pTHX_ I32 type, I32 flags, OP** firstp, OP** otherp) return other; } else { + /* Elide the rhs, since the outcome is entirely determined by + * the (constant) lhs */ + /* check for C<my $x if 0>, or C<my($x,$y) if 0> */ const OP *o2 = other; if ( ! (o2->op_type == OP_LIST @@ -6811,7 +6798,7 @@ S_new_logop(pTHX_ I32 type, I32 flags, OP** firstp, OP** otherp) *otherp = NULL; if (cstop->op_type == OP_CONST) cstop->op_private |= OPpCONST_SHORTCIRCUIT; - op_free(other); + op_free(other); return first; } } @@ -6858,12 +6845,28 @@ S_new_logop(pTHX_ I32 type, I32 flags, OP** firstp, OP** otherp) } } - if (!other) - return first; - if (type == OP_ANDASSIGN || type == OP_ORASSIGN || type == OP_DORASSIGN) other->op_private |= OPpASSIGN_BACKWARDS; /* other is an OP_SASSIGN */ + /* optimize AND and OR ops that have NOTs as children */ + if (first->op_type == OP_NOT + && (first->op_flags & OPf_KIDS) + && ((first->op_flags & OPf_SPECIAL) /* unless ($x) { } */ + || (other->op_type == OP_NOT)) /* if (!$x && !$y) { } */ + ) { + if (type == OP_AND || type == OP_OR) { + if (type == OP_AND) + type = OP_OR; + else + type = OP_AND; + op_null(first); + if (other->op_type == OP_NOT) { /* !a AND|OR !b => !(a OR|AND b) */ + op_null(other); + prepend_not = 1; /* prepend a NOT op later */ + } + } + } + logop = S_alloc_LOGOP(aTHX_ type, first, LINKLIST(other)); logop->op_flags |= (U8)flags; logop->op_private = (U8)(1 | (flags >> 8)); diff --git a/t/op/lop.t b/t/op/lop.t index bc4eb85..fe1c432 100644 --- a/t/op/lop.t +++ b/t/op/lop.t @@ -10,7 +10,7 @@ BEGIN { require './test.pl'; } -plan tests => 17; +plan tests => 23; for my $i (undef, 0 .. 2, "", "0 but true") { my $true = 1; @@ -56,3 +56,29 @@ is( $i, 10, 'negation precedence with &&' ); ++$y; $i = !$x && !$x && !$x && $y; is( $i, 11, 'negation precedence with &&, multiple operands' ); + +# [perl #127952]. This relates to OP_AND and OP_OR with a negated constant +# on the lhs (either a negated bareword, or a negation of a do{} containing +# a constant) and a negated non-foldable expression on the rhs. These cases +# yielded 42 or "Bare" or "str" before the bug was fixed. +{ + $x = 42; + + $i = !Bare || !$x; + is( $i, '', 'neg-bareword on lhs of || with non-foldable neg-true on rhs' ); + + $i = !Bare && !$x; + is( $i, '', 'neg-bareword on lhs of && with non-foldable neg-true on rhs' ); + + $i = do { !$x if !Bare }; + is( $i, '', 'neg-bareword on rhs of modifier-if with non-foldable neg-true on lhs' ); + + $i = do { !$x unless !Bare }; + is( $i, '', 'neg-bareword on rhs of modifier-unless with non-foldable neg-true on lhs' ); + + $i = !do { "str" } || !$x; + is( $i, '', 'neg-do-const on lhs of || with non-foldable neg-true on rhs' ); + + $i = !do { "str" } && !$x; + is( $i, '', 'neg-do-const on lhs of && with non-foldable neg-true on rhs' ); +} -- Perl5 Master Repository
