8054478: C2: Incorrectly compiled char[] array access crashes JVM

Summary: dead backbranch in main loop results in erroneous array access
Reviewed-by: kvn, iveresov

src/share/vm/opto/connode.cpp

@@ -441,6 +441,101 @@ Node *ConstraintCastNode::Ideal_DU_postCCP( PhaseCCP *ccp ) {
   return this;
 }
 
+uint CastIINode::size_of() const {
+  return sizeof(*this);
+}
+
+uint CastIINode::cmp(const Node &n) const {
+  return TypeNode::cmp(n) && ((CastIINode&)n)._carry_dependency == _carry_dependency;
+}
+
+Node *CastIINode::Identity(PhaseTransform *phase) {
+  if (_carry_dependency) {
+    return this;
+  }
+  return ConstraintCastNode::Identity(phase);
+}
+
+const Type *CastIINode::Value(PhaseTransform *phase) const {
+  const Type *res = ConstraintCastNode::Value(phase);
+
+  // Try to improve the type of the CastII if we recognize a CmpI/If
+  // pattern.
+  if (_carry_dependency) {
+    if (in(0) != NULL && (in(0)->is_IfFalse() || in(0)->is_IfTrue())) {
+      Node* proj = in(0);
+      if (proj->in(0)->in(1)->is_Bool()) {
+        Node* b = proj->in(0)->in(1);
+        if (b->in(1)->Opcode() == Op_CmpI) {
+          Node* cmp = b->in(1);
+          if (cmp->in(1) == in(1) && phase->type(cmp->in(2))->isa_int()) {
+            const TypeInt* in2_t = phase->type(cmp->in(2))->is_int();
+            const Type* t = TypeInt::INT;
+            BoolTest test = b->as_Bool()->_test;
+            if (proj->is_IfFalse()) {
+              test = test.negate();
+            }
+            BoolTest::mask m = test._test;
+            jlong lo_long = min_jint;
+            jlong hi_long = max_jint;
+            if (m == BoolTest::le || m == BoolTest::lt) {
+              hi_long = in2_t->_hi;
+              if (m == BoolTest::lt) {
+                hi_long -= 1;
+              }
+            } else if (m == BoolTest::ge || m == BoolTest::gt) {
+              lo_long = in2_t->_lo;
+              if (m == BoolTest::gt) {
+                lo_long += 1;
+              }
+            } else if (m == BoolTest::eq) {
+              lo_long = in2_t->_lo;
+              hi_long = in2_t->_hi;
+            } else if (m == BoolTest::ne) {
+              // can't do any better
+            } else {
+              stringStream ss;
+              test.dump_on(&ss);
+              fatal(err_msg_res("unexpected comparison %s", ss.as_string()));
+            }
+            int lo_int = (int)lo_long;
+            int hi_int = (int)hi_long;
+
+            if (lo_long != (jlong)lo_int) {
+              lo_int = min_jint;
+            }
+            if (hi_long != (jlong)hi_int) {
+              hi_int = max_jint;
+            }
+
+            t = TypeInt::make(lo_int, hi_int, Type::WidenMax);
+
+            res = res->filter_speculative(t);
+
+            return res;
+          }
+        }
+      }
+    }
+  }
+  return res;
+}
+
+Node *CastIINode::Ideal_DU_postCCP(PhaseCCP *ccp) {
+  if (_carry_dependency) {
+    return NULL;
+  }
+  return ConstraintCastNode::Ideal_DU_postCCP(ccp);
+}
+
+#ifndef PRODUCT
+void CastIINode::dump_spec(outputStream *st) const {
+  TypeNode::dump_spec(st);
+  if (_carry_dependency) {
+    st->print(" carry dependency");
+  }
+}
+#endif
 
 //=============================================================================
 

src/share/vm/opto/connode.hpp

@@ -241,10 +241,25 @@ public:
 //------------------------------CastIINode-------------------------------------
 // cast integer to integer (different range)
 class CastIINode: public ConstraintCastNode {
+  private:
+  // Can this node be removed post CCP or does it carry a required dependency?
+  const bool _carry_dependency;
+
+  protected:
+  virtual uint cmp( const Node &n ) const;
+  virtual uint size_of() const;
+
 public:
-  CastIINode (Node *n, const Type *t ): ConstraintCastNode(n,t) {}
+  CastIINode(Node *n, const Type *t, bool carry_dependency = false)
+    : ConstraintCastNode(n,t), _carry_dependency(carry_dependency) {}
   virtual int Opcode() const;
   virtual uint ideal_reg() const { return Op_RegI; }
+  virtual Node *Identity( PhaseTransform *phase );
+  virtual const Type *Value( PhaseTransform *phase ) const;
+  virtual Node *Ideal_DU_postCCP( PhaseCCP * );
+#ifndef PRODUCT
+  virtual void dump_spec(outputStream *st) const;
+#endif
 };
 
 //------------------------------CastPPNode-------------------------------------

src/share/vm/opto/loopTransform.cpp

@@ -881,6 +881,20 @@ Node *PhaseIdealLoop::clone_up_backedge_goo( Node *back_ctrl, Node *preheader_ct
   return n;
 }
 
+bool PhaseIdealLoop::cast_incr_before_loop(Node* incr, Node* ctrl, Node* loop) {
+  Node* castii = new (C) CastIINode(incr, TypeInt::INT, true);
+  castii->set_req(0, ctrl);
+  register_new_node(castii, ctrl);
+  for (DUIterator_Fast imax, i = incr->fast_outs(imax); i < imax; i++) {
+    Node* n = incr->fast_out(i);
+    if (n->is_Phi() && n->in(0) == loop) {
+      int nrep = n->replace_edge(incr, castii);
+      return true;
+    }
+  }
+  return false;
+}
+
 //------------------------------insert_pre_post_loops--------------------------
 // Insert pre and post loops.  If peel_only is set, the pre-loop can not have
 // more iterations added.  It acts as a 'peel' only, no lower-bound RCE, no
@@ -1079,6 +1093,24 @@ void PhaseIdealLoop::insert_pre_post_loops( IdealLoopTree *loop, Node_List &old_
     }
   }
 
+  // Nodes inside the loop may be control dependent on a predicate
+  // that was moved before the preloop. If the back branch of the main
+  // or post loops becomes dead, those nodes won't be dependent on the
+  // test that guards that loop nest anymore which could lead to an
+  // incorrect array access because it executes independently of the
+  // test that was guarding the loop nest. We add a special CastII on
+  // the if branch that enters the loop, between the input induction
+  // variable value and the induction variable Phi to preserve correct
+  // dependencies.
+
+  // CastII for the post loop:
+  bool inserted = cast_incr_before_loop(zer_opaq->in(1), zer_taken, post_head);
+  assert(inserted, "no castII inserted");
+
+  // CastII for the main loop:
+  inserted = cast_incr_before_loop(pre_incr, min_taken, main_head);
+  assert(inserted, "no castII inserted");
+
   // Step B4: Shorten the pre-loop to run only 1 iteration (for now).
   // RCE and alignment may change this later.
   Node *cmp_end = pre_end->cmp_node();

src/share/vm/opto/loopnode.hpp

@@ -602,6 +602,8 @@ class PhaseIdealLoop : public PhaseTransform {
     return ctrl;
   }
 
+  bool cast_incr_before_loop(Node* incr, Node* ctrl, Node* loop);
+
 public:
   bool has_node( Node* n ) const {
     guarantee(n != NULL, "No Node.");

src/share/vm/opto/phaseX.cpp

@@ -1340,15 +1340,27 @@ void PhaseIterGVN::add_users_to_worklist( Node *n ) {
       }
     }
 
-    if( use->is_Cmp() ) {       // Enable CMP/BOOL optimization
+    uint use_op = use->Opcode();
+    if(use->is_Cmp()) {       // Enable CMP/BOOL optimization
       add_users_to_worklist(use); // Put Bool on worklist
-      // Look for the 'is_x2logic' pattern: "x ? : 0 : 1" and put the
-      // phi merging either 0 or 1 onto the worklist
       if (use->outcnt() > 0) {
         Node* bol = use->raw_out(0);
         if (bol->outcnt() > 0) {
           Node* iff = bol->raw_out(0);
-          if (iff->outcnt() == 2) {
+          if (use_op == Op_CmpI &&
+              iff->is_CountedLoopEnd()) {
+            CountedLoopEndNode* cle = iff->as_CountedLoopEnd();
+            if (cle->limit() == n && cle->phi() != NULL) {
+              // If an opaque node feeds into the limit condition of a
+              // CountedLoop, we need to process the Phi node for the
+              // induction variable when the opaque node is removed:
+              // the range of values taken by the Phi is now known and
+              // so its type is also known.
+              _worklist.push(cle->phi());
+            }
+          } else if (iff->outcnt() == 2) {
+            // Look for the 'is_x2logic' pattern: "x ? : 0 : 1" and put the
+            // phi merging either 0 or 1 onto the worklist
             Node* ifproj0 = iff->raw_out(0);
             Node* ifproj1 = iff->raw_out(1);
             if (ifproj0->outcnt() > 0 && ifproj1->outcnt() > 0) {
@@ -1360,9 +1372,26 @@ void PhaseIterGVN::add_users_to_worklist( Node *n ) {
           }
         }
       }
+      if (use_op == Op_CmpI) {
+        Node* in1 = use->in(1);
+        for (uint i = 0; i < in1->outcnt(); i++) {
+          if (in1->raw_out(i)->Opcode() == Op_CastII) {
+            Node* castii = in1->raw_out(i);
+            if (castii->in(0) != NULL && castii->in(0)->in(0) != NULL && castii->in(0)->in(0)->is_If()) {
+              Node* ifnode = castii->in(0)->in(0);
+              if (ifnode->in(1) != NULL && ifnode->in(1)->in(1) == use) {
+                // Reprocess a CastII node that may depend on an
+                // opaque node value when the opaque node is
+                // removed. In case it carries a dependency we can do
+                // a better job of computing its type.
+                _worklist.push(castii);
+              }
+            }
+          }
+        }
+      }
     }
 
-    uint use_op = use->Opcode();
     // If changed Cast input, check Phi users for simple cycles
     if( use->is_ConstraintCast() || use->is_CheckCastPP() ) {
       for (DUIterator_Fast i2max, i2 = use->fast_outs(i2max); i2 < i2max; i2++) {

src/share/vm/opto/subnode.cpp

@@ -1147,12 +1147,10 @@ const Type *BoolTest::cc2logical( const Type *CC ) const {
 
 //------------------------------dump_spec-------------------------------------
 // Print special per-node info
-#ifndef PRODUCT
 void BoolTest::dump_on(outputStream *st) const {
   const char *msg[] = {"eq","gt","of","lt","ne","le","nof","ge"};
   st->print("%s", msg[_test]);
 }
-#endif
 
 //=============================================================================
 uint BoolNode::hash() const { return (Node::hash() << 3)|(_test._test+1); }

src/share/vm/opto/subnode.hpp

@@ -275,9 +275,7 @@ struct BoolTest VALUE_OBJ_CLASS_SPEC {
   mask commute( ) const { return mask("032147658"[_test]-'0'); }
   mask negate( ) const { return mask(_test^4); }
   bool is_canonical( ) const { return (_test == BoolTest::ne || _test == BoolTest::lt || _test == BoolTest::le || _test == BoolTest::overflow); }
-#ifndef PRODUCT
   void dump_on(outputStream *st) const;
-#endif
 };
 
 //------------------------------BoolNode---------------------------------------

test/compiler/loopopts/TestDeadBackbranchArrayAccess.java

+/*
+ * Copyright (c) 2014, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+/**
+ * @test
+ * @bug 8054478
+ * @summary dead backbranch in main loop results in erroneous array access
+ * @run main/othervm -XX:CompileOnly=TestDeadBackbranchArrayAccess -Xcomp TestDeadBackbranchArrayAccess
+ *
+ */
+
+public class TestDeadBackbranchArrayAccess {
+    static char[] pattern0 = {0};
+    static char[] pattern1 = {1};
+
+    static void test(char[] array) {
+        if (pattern1 == null) return;
+
+        int i = 0;
+        int pos = 0;
+        char c = array[pos];
+
+        while (i >= 0 && (c == pattern0[i] || c == pattern1[i])) {
+            i--;
+            pos--;
+            if (pos != -1) {
+                c = array[pos];
+            }
+        }
+    }
+
+    public static void main(String[] args) {
+        for (int i = 0; i < 1000000; i++) {
+            test(new char[1]);
+        }
+    }
+}