8054478: C2: Incorrectly compiled char[] array access crashes JVM
Summary: dead backbranch in main loop results in erroneous array access
Reviewed-by: kvn, iveresov
--- a/hotspot/src/share/vm/opto/castnode.cpp Mon Nov 24 07:29:03 2014 -0800
+++ b/hotspot/src/share/vm/opto/castnode.cpp Thu Nov 13 09:19:46 2014 +0100
@@ -83,6 +83,101 @@
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
//=============================================================================
--- a/hotspot/src/share/vm/opto/castnode.hpp Mon Nov 24 07:29:03 2014 -0800
+++ b/hotspot/src/share/vm/opto/castnode.hpp Thu Nov 13 09:19:46 2014 +0100
@@ -48,10 +48,25 @@
//------------------------------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-------------------------------------
--- a/hotspot/src/share/vm/opto/loopTransform.cpp Mon Nov 24 07:29:03 2014 -0800
+++ b/hotspot/src/share/vm/opto/loopTransform.cpp Thu Nov 13 09:19:46 2014 +0100
@@ -27,6 +27,7 @@
#include "memory/allocation.inline.hpp"
#include "opto/addnode.hpp"
#include "opto/callnode.hpp"
+#include "opto/castnode.hpp"
#include "opto/connode.hpp"
#include "opto/convertnode.hpp"
#include "opto/divnode.hpp"
@@ -884,6 +885,20 @@
return n;
}
+bool PhaseIdealLoop::cast_incr_before_loop(Node* incr, Node* ctrl, Node* loop) {
+ Node* castii = new 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
@@ -1080,6 +1095,24 @@
}
}
+ // 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();
--- a/hotspot/src/share/vm/opto/loopnode.hpp Mon Nov 24 07:29:03 2014 -0800
+++ b/hotspot/src/share/vm/opto/loopnode.hpp Thu Nov 13 09:19:46 2014 +0100
@@ -602,6 +602,8 @@
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.");
--- a/hotspot/src/share/vm/opto/phaseX.cpp Mon Nov 24 07:29:03 2014 -0800
+++ b/hotspot/src/share/vm/opto/phaseX.cpp Thu Nov 13 09:19:46 2014 +0100
@@ -1392,15 +1392,27 @@
}
}
- 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) {
@@ -1412,9 +1424,26 @@
}
}
}
+ 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++) {
--- a/hotspot/src/share/vm/opto/subnode.cpp Mon Nov 24 07:29:03 2014 -0800
+++ b/hotspot/src/share/vm/opto/subnode.cpp Thu Nov 13 09:19:46 2014 +0100
@@ -1147,12 +1147,10 @@
//------------------------------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); }
--- a/hotspot/src/share/vm/opto/subnode.hpp Mon Nov 24 07:29:03 2014 -0800
+++ b/hotspot/src/share/vm/opto/subnode.hpp Thu Nov 13 09:19:46 2014 +0100
@@ -275,9 +275,7 @@
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---------------------------------------
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/compiler/loopopts/TestDeadBackbranchArrayAccess.java Thu Nov 13 09:19:46 2014 +0100
@@ -0,0 +1,58 @@
+/*
+ * 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]);
+ }
+ }
+}