Question about tearing and buffered primitive classes

[Nick Gasson]

Hi,

There's one Valhalla test, TestBufferTearing.java, that fails frequently
on AArch64 when MyValue.incrementAndCheck() is compiled by C1:

java.lang.RuntimeException: Inconsistent field values: expected 0 to equal 675128
        at jdk.test.lib.Asserts.fail(Asserts.java:594)
        at jdk.test.lib.Asserts.assertEquals(Asserts.java:205)
        at jdk.test.lib.Asserts.assertEQ(Asserts.java:178)
        at compiler.valhalla.inlinetypes.MyValue.incrementAndCheck(TestBufferTearing.java:81)
        at compiler.valhalla.inlinetypes.TestBufferTearing$Runner.run(TestBufferTearing.java:124)

This test sets -XX:InlineFieldMaxFlatSize=0 so MyValue is always passed
around as a reference.  C1 inserts a store-store barrier between zeroing
the temporary object withfield allocates and initialising its fields.
But there's no barrier between the field initialisation and the store
that publishes the object (the store to test.vtField1 in
TestBufferTearing::run()) so other threads can observe these stores
out-of-order on a non-TSO system.

An obvious fix is to stick a store-store barrier after every withfield:

diff --git a/src/hotspot/share/c1/c1_GraphBuilder.cpp b/src/hotspot/share/c1/c1_GraphBuilder.cpp
index 86610d15421b..ead767f596d7 100644
--- a/src/hotspot/share/c1/c1_GraphBuilder.cpp
+++ b/src/hotspot/share/c1/c1_GraphBuilder.cpp
@@ -2116,6 +2116,10 @@ void GraphBuilder::withfield(int field_index)
     StoreField* store = new StoreField(new_instance, offset_modify, field_modify, val, false, state_before, needs_patching);
     append(store);
   }
+
+  // Ensure the stores which initialize the fields are visible before
+  // any subsequent store which publishes the object.
+  append(new MemBar(lir_membar_storestore));
 }

 Dependencies* GraphBuilder::dependency_recorder() const {


However based on my reading of the description in JDK-8237741 [1] this
should only be necessary when the object is passed by invisible
reference, not when it's actually flattened.  In Tobias' fix for
JDK-8238780 [2] a store-store barrier was added to C2's
InlineTypeBaseNode::buffer() but I can't see where the equivalent
"buffering" happens in C1.

Furthermore I don't understand why we can't instead rely on the barrier
that gets added before returning from a constructor that writes final
fields?  AIUI a primitive class's fields are always final and `this'
can't escape its constructor so the effect should be the same as putting
a barrier after each intermediate allocation inside the constructor.

GraphBuilder::withfield() calls set_wrote_final() which is used for this
purpose *but* in GraphBuilder::method_return() we don't set need_mem_bar
because method()->is_object_constructor() is false for primitive class
constructors.  So a different patch that also fixes this test case is:

diff --git a/src/hotspot/share/c1/c1_GraphBuilder.cpp b/src/hotspot/share/c1/c1_GraphBuilder.cpp
index ead767f596d7..ac7e4ce5f06d 100644
--- a/src/hotspot/share/c1/c1_GraphBuilder.cpp
+++ b/src/hotspot/share/c1/c1_GraphBuilder.cpp
@@ -1617,7 +1617,7 @@ void GraphBuilder::method_return(Value x, bool ignore_return) {

   // The conditions for a memory barrier are described in Parse::do_exits().
   bool need_mem_bar = false;
-  if (method()->is_object_constructor() &&
+  if ((method()->is_object_constructor() || method()->is_static_init_factory()) &&
        (scope()->wrote_final() ||
          (AlwaysSafeConstructors && scope()->wrote_fields()) ||
          (support_IRIW_for_not_multiple_copy_atomic_cpu && scope()->wrote_volatile()))) {


Any pointers on the best way to solve this?

--
Thanks,
Nick

[1] https://bugs.openjdk.java.net/browse/JDK-8237741
[2] http://hg.openjdk.java.net/valhalla/valhalla/rev/b018052fc8b1