8156760: VM crashes if -XX:-ReduceInitialCardMarks is set
authorthartmann
Thu, 02 Jun 2016 08:46:52 +0200
changeset 39254 fb4492288b01
parent 39253 bd5fe208734e
child 39255 c7281e9142ef
child 39256 ac12f57c6d9c
8156760: VM crashes if -XX:-ReduceInitialCardMarks is set Summary: Fixed several compiler crashes with disabled ReduceInitialCardMarks. Reviewed-by: roland, minqi, dlong, tschatzl, kvn
hotspot/src/share/vm/gc/g1/g1CollectedHeap.inline.hpp
hotspot/src/share/vm/gc/shared/collectedHeap.cpp
hotspot/src/share/vm/opto/arraycopynode.cpp
hotspot/src/share/vm/opto/arraycopynode.hpp
hotspot/src/share/vm/opto/graphKit.cpp
hotspot/src/share/vm/opto/macro.cpp
hotspot/src/share/vm/opto/memnode.cpp
hotspot/test/compiler/arraycopy/TestEliminatedArrayCopyDeopt.java
hotspot/test/compiler/arraycopy/TestInstanceCloneAsLoadsStores.java
--- a/hotspot/src/share/vm/gc/g1/g1CollectedHeap.inline.hpp	Wed Jun 01 16:36:44 2016 +0200
+++ b/hotspot/src/share/vm/gc/g1/g1CollectedHeap.inline.hpp	Thu Jun 02 08:46:52 2016 +0200
@@ -109,7 +109,7 @@
   _old_set.remove(hr);
 }
 
-// It dirties the cards that cover the block so that so that the post
+// It dirties the cards that cover the block so that the post
 // write barrier never queues anything when updating objects on this
 // block. It is assumed (and in fact we assert) that the block
 // belongs to a young region.
--- a/hotspot/src/share/vm/gc/shared/collectedHeap.cpp	Wed Jun 01 16:36:44 2016 +0200
+++ b/hotspot/src/share/vm/gc/shared/collectedHeap.cpp	Thu Jun 02 08:46:52 2016 +0200
@@ -386,7 +386,7 @@
 //     initialized by this point, a fact that we assert when doing the
 //     card-mark.)
 // (c) G1CollectedHeap(G1) uses two kinds of write barriers. When a
-//     G1 concurrent marking is in progress an SATB (pre-write-)barrier is
+//     G1 concurrent marking is in progress an SATB (pre-write-)barrier
 //     is used to remember the pre-value of any store. Initializing
 //     stores will not need this barrier, so we need not worry about
 //     compensating for the missing pre-barrier here. Turning now
--- a/hotspot/src/share/vm/opto/arraycopynode.cpp	Wed Jun 01 16:36:44 2016 +0200
+++ b/hotspot/src/share/vm/opto/arraycopynode.cpp	Thu Jun 02 08:46:52 2016 +0200
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 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
@@ -204,7 +204,8 @@
   }
 
   if (!finish_transform(phase, can_reshape, ctl, mem)) {
-    return NULL;
+    // Return NodeSentinel to indicate that the transform failed
+    return NodeSentinel;
   }
 
   return mem;
@@ -222,6 +223,7 @@
   Node* dest = in(ArrayCopyNode::Dest);
   const Type* src_type = phase->type(src);
   const TypeAryPtr* ary_src = src_type->isa_aryptr();
+  assert(ary_src != NULL, "should be an array copy/clone");
 
   if (is_arraycopy() || is_copyofrange() || is_copyof()) {
     const Type* dest_type = phase->type(dest);
@@ -520,7 +522,7 @@
 
   Node* mem = try_clone_instance(phase, can_reshape, count);
   if (mem != NULL) {
-    return mem;
+    return (mem == NodeSentinel) ? NULL : mem;
   }
 
   Node* adr_src = NULL;
@@ -627,31 +629,37 @@
   return CallNode::may_modify_arraycopy_helper(dest_t, t_oop, phase);
 }
 
-bool ArrayCopyNode::may_modify_helper(const TypeOopPtr *t_oop, Node* n, PhaseTransform *phase) {
+bool ArrayCopyNode::may_modify_helper(const TypeOopPtr *t_oop, Node* n, PhaseTransform *phase, ArrayCopyNode*& ac) {
   if (n->is_Proj()) {
     n = n->in(0);
     if (n->is_Call() && n->as_Call()->may_modify(t_oop, phase)) {
+      if (n->isa_ArrayCopy() != NULL) {
+        ac = n->as_ArrayCopy();
+      }
       return true;
     }
   }
   return false;
 }
 
-bool ArrayCopyNode::may_modify(const TypeOopPtr *t_oop, MemBarNode* mb, PhaseTransform *phase) {
+bool ArrayCopyNode::may_modify(const TypeOopPtr *t_oop, MemBarNode* mb, PhaseTransform *phase, ArrayCopyNode*& ac) {
   Node* mem = mb->in(TypeFunc::Memory);
 
   if (mem->is_MergeMem()) {
     Node* n = mem->as_MergeMem()->memory_at(Compile::AliasIdxRaw);
-    if (may_modify_helper(t_oop, n, phase)) {
+    if (may_modify_helper(t_oop, n, phase, ac)) {
       return true;
     } else if (n->is_Phi()) {
       for (uint i = 1; i < n->req(); i++) {
         if (n->in(i) != NULL) {
-          if (may_modify_helper(t_oop, n->in(i), phase)) {
+          if (may_modify_helper(t_oop, n->in(i), phase, ac)) {
             return true;
           }
         }
       }
+    } else if (n->Opcode() == Op_StoreCM) {
+      // Ignore card mark stores
+      return may_modify_helper(t_oop, n->in(MemNode::Memory), phase, ac);
     }
   }
 
--- a/hotspot/src/share/vm/opto/arraycopynode.hpp	Wed Jun 01 16:36:44 2016 +0200
+++ b/hotspot/src/share/vm/opto/arraycopynode.hpp	Thu Jun 02 08:46:52 2016 +0200
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 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
@@ -107,7 +107,7 @@
                             BasicType copy_type, const Type* value_type, int count);
   bool finish_transform(PhaseGVN *phase, bool can_reshape,
                         Node* ctl, Node *mem);
-  static bool may_modify_helper(const TypeOopPtr *t_oop, Node* n, PhaseTransform *phase);
+  static bool may_modify_helper(const TypeOopPtr *t_oop, Node* n, PhaseTransform *phase, ArrayCopyNode*& ac);
 
 public:
 
@@ -162,7 +162,7 @@
 
   bool is_alloc_tightly_coupled() const { return _alloc_tightly_coupled; }
 
-  static bool may_modify(const TypeOopPtr *t_oop, MemBarNode* mb, PhaseTransform *phase);
+  static bool may_modify(const TypeOopPtr *t_oop, MemBarNode* mb, PhaseTransform *phase, ArrayCopyNode*& ac);
   bool modifies(intptr_t offset_lo, intptr_t offset_hi, PhaseTransform* phase, bool must_modify);
 
 #ifndef PRODUCT
--- a/hotspot/src/share/vm/opto/graphKit.cpp	Wed Jun 01 16:36:44 2016 +0200
+++ b/hotspot/src/share/vm/opto/graphKit.cpp	Thu Jun 02 08:46:52 2016 +0200
@@ -4306,8 +4306,15 @@
       } __ end_if();
     } __ end_if();
   } else {
-    // Object.clone() instrinsic uses this path.
-    g1_mark_card(ideal, card_adr, oop_store, alias_idx, index, index_adr, buffer, tf);
+    // The Object.clone() intrinsic uses this path if !ReduceInitialCardMarks.
+    // We don't need a barrier here if the destination is a newly allocated object
+    // in Eden. Otherwise, GC verification breaks because we assume that cards in Eden
+    // are set to 'g1_young_gen' (see G1SATBCardTableModRefBS::verify_g1_young_region()).
+    assert(!use_ReduceInitialCardMarks(), "can only happen with card marking");
+    Node* card_val = __ load(__ ctrl(), card_adr, TypeInt::INT, T_BYTE, Compile::AliasIdxRaw);
+    __ if_then(card_val, BoolTest::ne, young_card); {
+      g1_mark_card(ideal, card_adr, oop_store, alias_idx, index, index_adr, buffer, tf);
+    } __ end_if();
   }
 
   // Final sync IdealKit and GraphKit.
--- a/hotspot/src/share/vm/opto/macro.cpp	Wed Jun 01 16:36:44 2016 +0200
+++ b/hotspot/src/share/vm/opto/macro.cpp	Thu Jun 02 08:46:52 2016 +0200
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2005, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2005, 2016, 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
@@ -32,6 +32,7 @@
 #include "opto/cfgnode.hpp"
 #include "opto/compile.hpp"
 #include "opto/convertnode.hpp"
+#include "opto/graphKit.hpp"
 #include "opto/locknode.hpp"
 #include "opto/loopnode.hpp"
 #include "opto/macro.hpp"
@@ -263,41 +264,58 @@
     // checks if the store done to a different from the value's region.
     // And replace Cmp with #0 (false) to collapse G1 post barrier.
     Node* xorx = p2x->find_out_with(Op_XorX);
-    assert(xorx != NULL, "missing G1 post barrier");
-    Node* shift = xorx->unique_out();
-    Node* cmpx = shift->unique_out();
-    assert(cmpx->is_Cmp() && cmpx->unique_out()->is_Bool() &&
-    cmpx->unique_out()->as_Bool()->_test._test == BoolTest::ne,
-    "missing region check in G1 post barrier");
-    _igvn.replace_node(cmpx, makecon(TypeInt::CC_EQ));
+    if (xorx != NULL) {
+      Node* shift = xorx->unique_out();
+      Node* cmpx = shift->unique_out();
+      assert(cmpx->is_Cmp() && cmpx->unique_out()->is_Bool() &&
+      cmpx->unique_out()->as_Bool()->_test._test == BoolTest::ne,
+      "missing region check in G1 post barrier");
+      _igvn.replace_node(cmpx, makecon(TypeInt::CC_EQ));
 
-    // Remove G1 pre barrier.
+      // Remove G1 pre barrier.
 
-    // Search "if (marking != 0)" check and set it to "false".
-    // There is no G1 pre barrier if previous stored value is NULL
-    // (for example, after initialization).
-    if (this_region->is_Region() && this_region->req() == 3) {
-      int ind = 1;
-      if (!this_region->in(ind)->is_IfFalse()) {
-        ind = 2;
-      }
-      if (this_region->in(ind)->is_IfFalse()) {
-        Node* bol = this_region->in(ind)->in(0)->in(1);
-        assert(bol->is_Bool(), "");
-        cmpx = bol->in(1);
-        if (bol->as_Bool()->_test._test == BoolTest::ne &&
-            cmpx->is_Cmp() && cmpx->in(2) == intcon(0) &&
-            cmpx->in(1)->is_Load()) {
-          Node* adr = cmpx->in(1)->as_Load()->in(MemNode::Address);
-          const int marking_offset = in_bytes(JavaThread::satb_mark_queue_offset() +
-                                              SATBMarkQueue::byte_offset_of_active());
-          if (adr->is_AddP() && adr->in(AddPNode::Base) == top() &&
-              adr->in(AddPNode::Address)->Opcode() == Op_ThreadLocal &&
-              adr->in(AddPNode::Offset) == MakeConX(marking_offset)) {
-            _igvn.replace_node(cmpx, makecon(TypeInt::CC_EQ));
+      // Search "if (marking != 0)" check and set it to "false".
+      // There is no G1 pre barrier if previous stored value is NULL
+      // (for example, after initialization).
+      if (this_region->is_Region() && this_region->req() == 3) {
+        int ind = 1;
+        if (!this_region->in(ind)->is_IfFalse()) {
+          ind = 2;
+        }
+        if (this_region->in(ind)->is_IfFalse()) {
+          Node* bol = this_region->in(ind)->in(0)->in(1);
+          assert(bol->is_Bool(), "");
+          cmpx = bol->in(1);
+          if (bol->as_Bool()->_test._test == BoolTest::ne &&
+              cmpx->is_Cmp() && cmpx->in(2) == intcon(0) &&
+              cmpx->in(1)->is_Load()) {
+            Node* adr = cmpx->in(1)->as_Load()->in(MemNode::Address);
+            const int marking_offset = in_bytes(JavaThread::satb_mark_queue_offset() +
+                                                SATBMarkQueue::byte_offset_of_active());
+            if (adr->is_AddP() && adr->in(AddPNode::Base) == top() &&
+                adr->in(AddPNode::Address)->Opcode() == Op_ThreadLocal &&
+                adr->in(AddPNode::Offset) == MakeConX(marking_offset)) {
+              _igvn.replace_node(cmpx, makecon(TypeInt::CC_EQ));
+            }
           }
         }
       }
+    } else {
+      assert(!GraphKit::use_ReduceInitialCardMarks(), "can only happen with card marking");
+      // This is a G1 post barrier emitted by the Object.clone() intrinsic.
+      // Search for the CastP2X->URShiftX->AddP->LoadB->Cmp path which checks if the card
+      // is marked as young_gen and replace the Cmp with 0 (false) to collapse the barrier.
+      Node* shift = p2x->find_out_with(Op_URShiftX);
+      assert(shift != NULL, "missing G1 post barrier");
+      Node* addp = shift->unique_out();
+      Node* load = addp->find_out_with(Op_LoadB);
+      assert(load != NULL, "missing G1 post barrier");
+      Node* cmpx = load->unique_out();
+      assert(cmpx->is_Cmp() && cmpx->unique_out()->is_Bool() &&
+             cmpx->unique_out()->as_Bool()->_test._test == BoolTest::ne,
+             "missing card value check in G1 post barrier");
+      _igvn.replace_node(cmpx, makecon(TypeInt::CC_EQ));
+      // There is no G1 pre barrier in this case
     }
     // Now CastP2X can be removed since it is used only on dead path
     // which currently still alive until igvn optimize it.
@@ -326,17 +344,15 @@
         CallNode *call = in->as_Call();
         if (call->may_modify(tinst, phase)) {
           assert(call->is_ArrayCopy(), "ArrayCopy is the only call node that doesn't make allocation escape");
-
           if (call->as_ArrayCopy()->modifies(offset, offset, phase, false)) {
             return in;
           }
         }
         mem = in->in(TypeFunc::Memory);
       } else if (in->is_MemBar()) {
-        if (ArrayCopyNode::may_modify(tinst, in->as_MemBar(), phase)) {
-          assert(in->in(0)->is_Proj() && in->in(0)->in(0)->is_ArrayCopy(), "should be arraycopy");
-          ArrayCopyNode* ac = in->in(0)->in(0)->as_ArrayCopy();
-          assert(ac->is_clonebasic(), "Only basic clone is a non escaping clone");
+        ArrayCopyNode* ac = NULL;
+        if (ArrayCopyNode::may_modify(tinst, in->as_MemBar(), phase, ac)) {
+          assert(ac != NULL && ac->is_clonebasic(), "Only basic clone is a non escaping clone");
           return ac;
         }
         mem = in->in(TypeFunc::Memory);
--- a/hotspot/src/share/vm/opto/memnode.cpp	Wed Jun 01 16:36:44 2016 +0200
+++ b/hotspot/src/share/vm/opto/memnode.cpp	Thu Jun 02 08:46:52 2016 +0200
@@ -160,7 +160,8 @@
           }
         }
       } else if (proj_in->is_MemBar()) {
-        if (ArrayCopyNode::may_modify(t_oop, proj_in->as_MemBar(), phase)) {
+        ArrayCopyNode* ac = NULL;
+        if (ArrayCopyNode::may_modify(t_oop, proj_in->as_MemBar(), phase, ac)) {
           break;
         }
         result = proj_in->in(TypeFunc::Memory);
@@ -657,7 +658,8 @@
           continue;         // (a) advance through independent call memory
         }
       } else if (mem->is_Proj() && mem->in(0)->is_MemBar()) {
-        if (ArrayCopyNode::may_modify(addr_t, mem->in(0)->as_MemBar(), phase)) {
+        ArrayCopyNode* ac = NULL;
+        if (ArrayCopyNode::may_modify(addr_t, mem->in(0)->as_MemBar(), phase, ac)) {
           break;
         }
         mem = mem->in(0)->in(TypeFunc::Memory);
--- a/hotspot/test/compiler/arraycopy/TestEliminatedArrayCopyDeopt.java	Wed Jun 01 16:36:44 2016 +0200
+++ b/hotspot/test/compiler/arraycopy/TestEliminatedArrayCopyDeopt.java	Thu Jun 02 08:46:52 2016 +0200
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 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
@@ -23,10 +23,10 @@
 
 /*
  * @test
- * @bug 8130847
+ * @bug 8130847 8156760
  * @summary Eliminated instance/array written to by an array copy variant must be correctly initialized when reallocated at a deopt
  * @run main/othervm -XX:-BackgroundCompilation -XX:-UseOnStackReplacement TestEliminatedArrayCopyDeopt
- *
+ * @run main/othervm -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:+IgnoreUnrecognizedVMOptions -XX:-ReduceInitialCardMarks TestEliminatedArrayCopyDeopt
  */
 
 // Test that if an ArrayCopy node is eliminated because it doesn't
--- a/hotspot/test/compiler/arraycopy/TestInstanceCloneAsLoadsStores.java	Wed Jun 01 16:36:44 2016 +0200
+++ b/hotspot/test/compiler/arraycopy/TestInstanceCloneAsLoadsStores.java	Thu Jun 02 08:46:52 2016 +0200
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 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
@@ -23,12 +23,12 @@
 
 /*
  * @test
- * @bug 6700100
+ * @bug 6700100 8156760
  * @summary small instance clone as loads/stores
  * @compile TestInstanceCloneAsLoadsStores.java TestInstanceCloneUtils.java
  * @run main/othervm -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:CompileCommand=dontinline,TestInstanceCloneAsLoadsStores::m* TestInstanceCloneAsLoadsStores
  * @run main/othervm -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:CompileCommand=dontinline,TestInstanceCloneAsLoadsStores::m* -XX:+IgnoreUnrecognizedVMOptions -XX:+StressArrayCopyMacroNode TestInstanceCloneAsLoadsStores
- *
+ * @run main/othervm -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:CompileCommand=dontinline,TestInstanceCloneAsLoadsStores::m* -XX:+IgnoreUnrecognizedVMOptions -XX:-ReduceInitialCardMarks TestInstanceCloneAsLoadsStores
  */
 
 public class TestInstanceCloneAsLoadsStores extends TestInstanceCloneUtils {