changeset 58892:b018052fc8b1 lworld

8238780: [lworld] C2: Partially initialized value type buffer might be observed by other thread Reviewed-by: roland
author thartmann
date Fri, 14 Feb 2020 14:10:11 +0100
parents 2f0e31b13e3f
children 46fd5a8c2f5d
files src/hotspot/share/opto/callGenerator.cpp src/hotspot/share/opto/callnode.cpp src/hotspot/share/opto/callnode.hpp src/hotspot/share/opto/library_call.cpp src/hotspot/share/opto/macro.cpp src/hotspot/share/opto/memnode.cpp src/hotspot/share/opto/parseHelper.cpp src/hotspot/share/opto/valuetypenode.cpp test/hotspot/jtreg/compiler/valhalla/valuetypes/TestBufferTearing.java test/hotspot/jtreg/compiler/valhalla/valuetypes/TestDeoptimizationWhenBuffering.java test/hotspot/jtreg/compiler/valhalla/valuetypes/TestMethodHandles.java test/hotspot/jtreg/compiler/valhalla/valuetypes/TestNullableValueTypes.java test/hotspot/jtreg/compiler/valhalla/valuetypes/ValueTypeTest.java
diffstat 13 files changed, 220 insertions(+), 20 deletions(-) [+]
line wrap: on
line diff
--- a/src/hotspot/share/opto/callGenerator.cpp	Fri Feb 14 14:08:43 2020 +0100
+++ b/src/hotspot/share/opto/callGenerator.cpp	Fri Feb 14 14:10:11 2020 +0100
@@ -538,6 +538,11 @@
       assert(buffer_oop != NULL, "should have allocated a buffer");
       ValueTypeNode* vt = result->as_ValueType();
       vt->store(&kit, buffer_oop, buffer_oop, vt->type()->value_klass(), 0);
+      // Do not let stores that initialize this buffer be reordered with a subsequent
+      // store that would make this buffer accessible by other threads.
+      AllocateNode* alloc = AllocateNode::Ideal_allocation(buffer_oop, &kit.gvn());
+      assert(alloc != NULL, "must have an allocation node");
+      kit.insert_mem_bar(Op_MemBarStoreStore, alloc->proj_out_or_null(AllocateNode::RawAddress));
       result = buffer_oop;
     } else if (result->is_ValueTypePtr() && returned_as_fields) {
       result->as_ValueTypePtr()->replace_call_results(&kit, call, C);
--- a/src/hotspot/share/opto/callnode.cpp	Fri Feb 14 14:08:43 2020 +0100
+++ b/src/hotspot/share/opto/callnode.cpp	Fri Feb 14 14:10:11 2020 +0100
@@ -1677,6 +1677,14 @@
       igvn->replace_node(projs->catchall_catchproj, phase->C->top());
     }
     if (projs->resproj[0] != NULL) {
+      // Remove MemBarStoreStore user as well
+      for (DUIterator_Fast imax, i = projs->resproj[0]->fast_outs(imax); i < imax; i++) {
+        MemBarStoreStoreNode* mb = projs->resproj[0]->fast_out(i)->isa_MemBarStoreStore();
+        if (mb != NULL && mb->outcnt() == 2) {
+          mb->remove(igvn);
+          --i; --imax;
+        }
+      }
       igvn->replace_node(projs->resproj[0], phase->C->top());
     }
     igvn->replace_node(this, phase->C->top());
@@ -1696,7 +1704,7 @@
   return CallNode::Ideal(phase, can_reshape);
 }
 
-Node *AllocateNode::make_ideal_mark(PhaseGVN *phase, Node* obj, Node* control, Node* mem) {
+Node* AllocateNode::make_ideal_mark(PhaseGVN* phase, Node* control, Node* mem) {
   Node* mark_node = NULL;
   // For now only enable fast locking for non-array types
   if ((EnableValhalla || UseBiasedLocking) && Opcode() == Op_Allocate) {
--- a/src/hotspot/share/opto/callnode.hpp	Fri Feb 14 14:08:43 2020 +0100
+++ b/src/hotspot/share/opto/callnode.hpp	Fri Feb 14 14:10:11 2020 +0100
@@ -990,7 +990,7 @@
   void compute_MemBar_redundancy(ciMethod* initializer);
   bool is_allocation_MemBar_redundant() { return _is_allocation_MemBar_redundant; }
 
-  Node* make_ideal_mark(PhaseGVN *phase, Node* obj, Node* control, Node* mem);
+  Node* make_ideal_mark(PhaseGVN* phase, Node* control, Node* mem);
 };
 
 //------------------------------AllocateArray---------------------------------
--- a/src/hotspot/share/opto/library_call.cpp	Fri Feb 14 14:08:43 2020 +0100
+++ b/src/hotspot/share/opto/library_call.cpp	Fri Feb 14 14:10:11 2020 +0100
@@ -139,7 +139,7 @@
     if (!stopped() && res != NULL) {
       BasicType bt = res->bottom_type()->basic_type();
       if (C->inlining_incrementally() && res->is_ValueType()) {
-        // The caller expects and oop when incrementally inlining an intrinsic that returns an
+        // The caller expects an oop when incrementally inlining an intrinsic that returns an
         // inline type. Make sure the call is re-executed if the allocation triggers a deoptimization.
         PreserveReexecuteState preexecs(this);
         jvms()->set_should_reexecute(true);
--- a/src/hotspot/share/opto/macro.cpp	Fri Feb 14 14:08:43 2020 +0100
+++ b/src/hotspot/share/opto/macro.cpp	Fri Feb 14 14:10:11 2020 +0100
@@ -1706,7 +1706,7 @@
                                           Node* size_in_bytes) {
   InitializeNode* init = alloc->initialization();
   // Store the klass & mark bits
-  Node* mark_node = alloc->make_ideal_mark(&_igvn, object, control, rawmem);
+  Node* mark_node = alloc->make_ideal_mark(&_igvn, control, rawmem);
   if (!mark_node->is_Con()) {
     transform_later(mark_node);
   }
--- a/src/hotspot/share/opto/memnode.cpp	Fri Feb 14 14:08:43 2020 +0100
+++ b/src/hotspot/share/opto/memnode.cpp	Fri Feb 14 14:10:11 2020 +0100
@@ -1744,7 +1744,7 @@
       alloc->initialization()->proj_out_or_null(0) != NULL) {
     InitializeNode* init = alloc->initialization();
     Node* control = init->proj_out(0);
-    return alloc->make_ideal_mark(phase, address, control, mem);
+    return alloc->make_ideal_mark(phase, control, mem);
   }
 
   return progress ? this : NULL;
--- a/src/hotspot/share/opto/parseHelper.cpp	Fri Feb 14 14:08:43 2020 +0100
+++ b/src/hotspot/share/opto/parseHelper.cpp	Fri Feb 14 14:10:11 2020 +0100
@@ -372,8 +372,8 @@
     if (stopped()) return;
     val = ValueTypeNode::make_from_oop(this, val, gvn().type(val)->value_klass());
   } else if (val->is_ValueType() && !field->is_flattenable()) {
-    // Non-flattenable field should not be scalarized
-    // Re-execute withfield if buffering triggers deoptimization
+    // Non-flattenable field value needs to be allocated because it can be merged
+    // with an oop. Re-execute withfield if buffering triggers deoptimization.
     PreserveReexecuteState preexecs(this);
     jvms()->set_should_reexecute(true);
     inc_sp(2);
--- a/src/hotspot/share/opto/valuetypenode.cpp	Fri Feb 14 14:08:43 2020 +0100
+++ b/src/hotspot/share/opto/valuetypenode.cpp	Fri Feb 14 14:10:11 2020 +0100
@@ -396,6 +396,13 @@
     Node* klass_node = kit->makecon(TypeKlassPtr::make(vk));
     Node* alloc_oop  = kit->new_instance(klass_node, NULL, NULL, /* deoptimize_on_exception */ true, this);
     store(kit, alloc_oop, alloc_oop, vk, 0);
+
+    // Do not let stores that initialize this buffer be reordered with a subsequent
+    // store that would make this buffer accessible by other threads.
+    AllocateNode* alloc = AllocateNode::Ideal_allocation(alloc_oop, &kit->gvn());
+    assert(alloc != NULL, "must have an allocation node");
+    kit->insert_mem_bar(Op_MemBarStoreStore, alloc->proj_out_or_null(AllocateNode::RawAddress));
+
     region->init_req(2, kit->control());
     oop   ->init_req(2, alloc_oop);
     io    ->init_req(2, kit->i_o());
@@ -419,6 +426,7 @@
   if (safe_for_replace) {
     kit->replace_in_map(this, vt);
   }
+  assert(vt->is_allocated(&kit->gvn()), "must be allocated");
   return vt;
 }
 
@@ -608,6 +616,7 @@
   }
   res->set_type(TypeValueType::make(vk, true));
   res = kit->gvn().transform(res)->as_ValueType();
+  assert(!allocate || res->is_allocated(&kit->gvn()), "must be allocated");
   return res;
 }
 
@@ -618,6 +627,12 @@
   mark = kit->gvn().transform(new AndXNode(mark, kit->MakeConX(~markWord::larval_mask_in_place)));
   kit->store_to_memory(kit->control(), mark_addr, mark, TypeX_X->basic_type(), kit->gvn().type(mark_addr)->is_ptr(), MemNode::unordered);
 
+  // Do not let stores that initialize this buffer be reordered with a subsequent
+  // store that would make this buffer accessible by other threads.
+  AllocateNode* alloc = AllocateNode::Ideal_allocation(obj, &kit->gvn());
+  assert(alloc != NULL, "must have an allocation node");
+  kit->insert_mem_bar(Op_MemBarStoreStore, alloc->proj_out_or_null(AllocateNode::RawAddress));
+
   ciValueKlass* vk = value_klass();
   ValueTypeNode* res = clone()->as_ValueType();
   res->set_type(TypeValueType::make(vk, false));
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/hotspot/jtreg/compiler/valhalla/valuetypes/TestBufferTearing.java	Fri Feb 14 14:10:11 2020 +0100
@@ -0,0 +1,151 @@
+/*
+ * Copyright (c) 2020, 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.
+ */
+
+package compiler.valhalla.valuetypes;
+
+import java.lang.invoke.*;
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+
+import jdk.test.lib.Asserts;
+import jdk.internal.misc.Unsafe;
+
+/**
+ * @test TestBufferTearing
+ * @summary Detect tearing on value type buffer writes due to missing barriers.
+ * @library /testlibrary /test/lib /compiler/whitebox /
+ * @modules java.base/jdk.internal.misc
+ * @run main/othervm -XX:ValueFieldMaxFlatSize=0 -XX:ValueArrayElemMaxFlatSize=0
+ *                   -XX:+StressGCM -XX:+StressLCM
+ *                   compiler.valhalla.valuetypes.TestBufferTearing
+ * @run main/othervm -XX:ValueFieldMaxFlatSize=0 -XX:ValueArrayElemMaxFlatSize=0
+ *                   -XX:+StressGCM -XX:+StressLCM -XX:+AlwaysIncrementalInline
+ *                   compiler.valhalla.valuetypes.TestBufferTearing
+ * @run main/othervm -XX:ValueFieldMaxFlatSize=0 -XX:ValueArrayElemMaxFlatSize=0
+ *                   -XX:CompileCommand=dontinline,*::incrementAndCheck*
+ *                   -XX:+StressGCM -XX:+StressLCM
+ *                   compiler.valhalla.valuetypes.TestBufferTearing
+ * @run main/othervm -XX:ValueFieldMaxFlatSize=0 -XX:ValueArrayElemMaxFlatSize=0
+ *                   -XX:CompileCommand=dontinline,*::incrementAndCheck*
+ *                   -XX:+StressGCM -XX:+StressLCM -XX:+AlwaysIncrementalInline
+ *                   compiler.valhalla.valuetypes.TestBufferTearing
+ */
+
+inline class MyValue {
+    int x;
+    int y;
+
+    private static final Unsafe U = Unsafe.getUnsafe();
+    private static final long X_OFFSET;
+    private static final long Y_OFFSET;
+    static {
+        try {
+            Field xField = MyValue.class.getDeclaredField("x");
+            X_OFFSET = U.objectFieldOffset(xField);
+            Field yField = MyValue.class.getDeclaredField("y");
+            Y_OFFSET = U.objectFieldOffset(yField);
+        } catch (Exception e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    MyValue(int x, int y) {
+        this.x = x;
+        this.y = y;
+    }
+
+    MyValue incrementAndCheck() {
+        Asserts.assertEQ(x, y, "Inconsistent field values");
+        return new MyValue(x + 1, y + 1);
+    }
+
+    MyValue incrementAndCheckUnsafe() {
+        Asserts.assertEQ(x, y, "Inconsistent field values");
+        MyValue vt = U.makePrivateBuffer(this);
+        U.putInt(vt, X_OFFSET, x + 1);
+        U.putInt(vt, Y_OFFSET, y + 1);
+        return U.finishPrivateBuffer(vt);
+    }
+}
+
+public class TestBufferTearing {
+
+    static MyValue vtField1;
+    MyValue vtField2;
+    MyValue[] vtField3 = new MyValue[1];
+
+    static final MethodHandle incrementAndCheck_mh;
+
+    static {
+        try {
+            Class<?> clazz = MyValue.class;
+            MethodHandles.Lookup lookup = MethodHandles.lookup();
+
+            MethodType mt = MethodType.methodType(MyValue.class);
+            incrementAndCheck_mh = lookup.findVirtual(clazz, "incrementAndCheck", mt);
+        } catch (NoSuchMethodException | IllegalAccessException e) {
+            e.printStackTrace();
+            throw new RuntimeException("Method handle lookup failed");
+        }
+    }
+
+    static class Runner extends Thread {
+        TestBufferTearing test;
+
+        public Runner(TestBufferTearing test) {
+            this.test = test;
+        }
+
+        public void run() {
+            for (int i = 0; i < 1_000_000; ++i) {
+                test.vtField1 = test.vtField1.incrementAndCheck();
+                test.vtField2 = test.vtField2.incrementAndCheck();
+                test.vtField3[0] = test.vtField3[0].incrementAndCheck();
+
+                test.vtField1 = test.vtField1.incrementAndCheckUnsafe();
+                test.vtField2 = test.vtField2.incrementAndCheckUnsafe();
+                test.vtField3[0] = test.vtField3[0].incrementAndCheckUnsafe();
+
+                try {
+                    test.vtField1 = (MyValue)incrementAndCheck_mh.invokeExact(test.vtField1);
+                    test.vtField2 = (MyValue)incrementAndCheck_mh.invokeExact(test.vtField2);
+                    test.vtField3[0] = (MyValue)incrementAndCheck_mh.invokeExact(test.vtField3[0]);
+                } catch (Throwable t) {
+                    throw new RuntimeException("Invoke failed", t);
+                }
+            }
+        }
+    }
+
+    public static void main(String[] args) throws Exception {
+        // Create threads that concurrently update some value type (array) fields
+        // and check the fields of the value types for consistency to detect tearing.
+        TestBufferTearing test = new TestBufferTearing();
+        Thread runner = null;
+        for (int i = 0; i < 10; ++i) {
+            runner = new Runner(test);
+            runner.start();
+        }
+        runner.join();
+    }
+}
--- a/test/hotspot/jtreg/compiler/valhalla/valuetypes/TestDeoptimizationWhenBuffering.java	Fri Feb 14 14:08:43 2020 +0100
+++ b/test/hotspot/jtreg/compiler/valhalla/valuetypes/TestDeoptimizationWhenBuffering.java	Fri Feb 14 14:10:11 2020 +0100
@@ -94,7 +94,6 @@
     static {
         try {
             Class<?> clazz = TestDeoptimizationWhenBuffering.class;
-            ClassLoader loader = clazz.getClassLoader();
             MethodHandles.Lookup lookup = MethodHandles.lookup();
 
             MethodType mt = MethodType.methodType(MyValue1.class);
--- a/test/hotspot/jtreg/compiler/valhalla/valuetypes/TestMethodHandles.java	Fri Feb 14 14:08:43 2020 +0100
+++ b/test/hotspot/jtreg/compiler/valhalla/valuetypes/TestMethodHandles.java	Fri Feb 14 14:10:11 2020 +0100
@@ -55,7 +55,6 @@
     static {
         try {
             Class<?> clazz = TestMethodHandles.class;
-            ClassLoader loader = clazz.getClassLoader();
             MethodHandles.Lookup lookup = MethodHandles.lookup();
 
             MethodType mt = MethodType.methodType(MyValue3.class);
--- a/test/hotspot/jtreg/compiler/valhalla/valuetypes/TestNullableValueTypes.java	Fri Feb 14 14:08:43 2020 +0100
+++ b/test/hotspot/jtreg/compiler/valhalla/valuetypes/TestNullableValueTypes.java	Fri Feb 14 14:10:11 2020 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2019, 2020, 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
@@ -61,7 +61,6 @@
     static {
         try {
             Class<?> clazz = TestNullableValueTypes.class;
-            ClassLoader loader = clazz.getClassLoader();
             MethodHandles.Lookup lookup = MethodHandles.lookup();
 
             MethodType test18_mt = MethodType.methodType(void.class, MyValue1.class.asNullableType());
@@ -881,4 +880,22 @@
             Asserts.assertEquals(test34Val, testValue1);
         }
     }
+
+    // Same as test17 but with non-allocated value at withfield
+    @Test()
+    public Test17Value test35(boolean b) {
+        Test17Value vt1 = Test17Value.default;
+        if ((Object)vt1.valueField != null) {
+            throw new RuntimeException("Should be null");
+        }
+        MyValue1 vt3 = MyValue1.createWithFieldsInline(rI, rL);
+        Test17Value vt2 = new Test17Value(vt3);
+        return b ? vt1 : vt2;
+    }
+
+    @DontCompile
+    public void test35_verifier(boolean warmup) {
+        test35(true);
+        test35(false);
+    }
 }
--- a/test/hotspot/jtreg/compiler/valhalla/valuetypes/ValueTypeTest.java	Fri Feb 14 14:08:43 2020 +0100
+++ b/test/hotspot/jtreg/compiler/valhalla/valuetypes/ValueTypeTest.java	Fri Feb 14 14:10:11 2020 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2020, 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
@@ -129,25 +129,27 @@
     // User defined settings
     protected static final boolean XCOMP = Platform.isComp();
     private static final boolean PRINT_GRAPH = true;
-    protected static final boolean VERBOSE = Boolean.parseBoolean(System.getProperty("Verbose", "false"));
+    private static final boolean VERBOSE = Boolean.parseBoolean(System.getProperty("Verbose", "false"));
     private static final boolean PRINT_TIMES = Boolean.parseBoolean(System.getProperty("PrintTimes", "false"));
-    private static       boolean VERIFY_IR = Boolean.parseBoolean(System.getProperty("VerifyIR", "true")) && !XCOMP && !TEST_C1;
+    private static final boolean COMPILE_COMMANDS = Boolean.parseBoolean(System.getProperty("CompileCommands", "true"));
+    private static       boolean VERIFY_IR = Boolean.parseBoolean(System.getProperty("VerifyIR", "true")) && !XCOMP && !TEST_C1 && COMPILE_COMMANDS;
     private static final boolean VERIFY_VM = Boolean.parseBoolean(System.getProperty("VerifyVM", "false"));
     private static final String SCENARIOS = System.getProperty("Scenarios", "");
     private static final String TESTLIST = System.getProperty("Testlist", "");
     private static final String EXCLUDELIST = System.getProperty("Exclude", "");
     private static final int WARMUP = Integer.parseInt(System.getProperty("Warmup", "251"));
     private static final boolean DUMP_REPLAY = Boolean.parseBoolean(System.getProperty("DumpReplay", "false"));
-    protected static final boolean FLIP_C1_C2 = Boolean.parseBoolean(System.getProperty("FlipC1C2", "false"));
-    protected static final boolean GCAfter = Boolean.parseBoolean(System.getProperty("GCAfter", "false"));
-    private static final int OSRTestTimeOut = Integer.parseInt(System.getProperty("OSRTestTimeOut", "5000"));
+    private static final boolean FLIP_C1_C2 = Boolean.parseBoolean(System.getProperty("FlipC1C2", "false"));
+    private static final boolean GC_AFTER = Boolean.parseBoolean(System.getProperty("GCAfter", "false"));
+    private static final int OSR_TEST_TIMEOUT = Integer.parseInt(System.getProperty("OSRTestTimeOut", "5000"));
 
     // "jtreg -DXcomp=true" runs all the scenarios with -Xcomp. This is faster than "jtreg -javaoptions:-Xcomp".
     protected static final boolean RUN_SCENARIOS_WITH_XCOMP = Boolean.parseBoolean(System.getProperty("Xcomp", "false"));
 
     // Pre-defined settings
     private static final String[] defaultFlags = {
-        "-XX:-BackgroundCompilation",
+        "-XX:-BackgroundCompilation"};
+    private static final String[] compileCommandFlags = {
         "-XX:CompileCommand=quiet",
         "-XX:CompileCommand=compileonly,java.lang.invoke.*::*",
         "-XX:CompileCommand=compileonly,java.lang.Long::sum",
@@ -433,6 +435,7 @@
         //        getVMParameters()
         //        getExtraVMParameters()
         //     defaultFlags
+        //     compileCommandFlags
         String cmds[] = null;
         if (VERIFY_IR) {
             // Add print flags for IR verification
@@ -445,6 +448,9 @@
         }
         cmds = concat(cmds, vmInputArgs);
         cmds = concat(cmds, defaultFlags);
+        if (COMPILE_COMMANDS) {
+          cmds = concat(cmds, compileCommandFlags);
+        }
 
         // Run tests in own process and verify output
         cmds = concat(cmds, getClass().getName(), "run");
@@ -699,7 +705,7 @@
                         // Don't control compilation if -Xcomp is enabled, or if compiler is disabled
                         break;
                     }
-                    Asserts.assertTrue(OSRTestTimeOut < 0 || elapsed < OSRTestTimeOut, test + " not compiled after " + OSRTestTimeOut + " ms");
+                    Asserts.assertTrue(OSR_TEST_TIMEOUT < 0 || elapsed < OSR_TEST_TIMEOUT, test + " not compiled after " + OSR_TEST_TIMEOUT + " ms");
                 }
                 if (!XCOMP) {
                     Asserts.assertTrue(!USE_COMPILER || WHITE_BOX.isMethodCompiled(test, false), test + " not compiled");
@@ -727,7 +733,7 @@
                     System.out.println("Done " + test.getName() + ": " + duration + " ns = " + (duration / 1000000) + " ms");
                 }
             }
-            if (GCAfter) {
+            if (GC_AFTER) {
                 System.out.println("doing GC");
                 System.gc();
             }