8331503: [lworld] Tearing issues in the interpreter when using the Access API

Reviewed-by: dsimms

Author: fparain

src/hotspot/share/oops/accessBackend.cpp

@@ -164,6 +164,10 @@ namespace AccessInternal {
     Copy::conjoint_memory_atomic(src, dst, length);
   }
 
+  void value_copy_internal(void* src, void* dst, size_t length) {
+    Copy::copy_value_content(src, dst, length);
+  }
+
 #ifdef ASSERT
   void check_access_thread_state() {
     if (VMError::is_error_reported() || DebuggingContext::is_enabled()) {

src/hotspot/share/oops/accessBackend.hpp

@@ -160,6 +160,8 @@ namespace AccessInternal {
   void arraycopy_arrayof_conjoint(T* src, T* dst, size_t length);
   template<typename T>
   void arraycopy_conjoint_atomic(T* src, T* dst, size_t length);
+
+  void value_copy_internal(void* src, void* dst, size_t length);
 }
 
 // This mask specifies what decorators are relevant for raw accesses. When passing

src/hotspot/share/oops/accessBackend.inline.hpp

@@ -333,7 +333,7 @@ inline void RawAccessBarrier<decorators>::clone(oop src, oop dst, size_t size) {
 
 template <DecoratorSet decorators>
 inline void RawAccessBarrier<decorators>::value_copy(void* src, void* dst, InlineKlass* md, LayoutKind lk) {
-  assert(is_aligned(src, md->layout_alignment(lk)) && is_aligned(dst, md->layout_alignment(lk)), "Unalign value_copy");
-  AccessInternal::arraycopy_conjoint_atomic(src, dst, static_cast<size_t>(md->layout_size_in_bytes(lk)));
+  assert(is_aligned(src, md->layout_alignment(lk)) && is_aligned(dst, md->layout_alignment(lk)), "Unaligned value_copy");
+  AccessInternal::value_copy_internal(src, dst, static_cast<size_t>(md->layout_size_in_bytes(lk)));
 }
 #endif // SHARE_OOPS_ACCESSBACKEND_INLINE_HPP

src/hotspot/share/utilities/copy.cpp

@@ -54,6 +54,46 @@ void Copy::conjoint_memory_atomic(const void* from, void* to, size_t size) {
   }
 }
 
+#define COPY_ALIGNED_SEGMENT(t) \
+  if (bits % sizeof(t) == 0) { \
+    size_t segment = remain / sizeof(t); \
+    if (segment > 0) { \
+      Copy::conjoint_##t##s_atomic((const t*) cursor_from, (t*) cursor_to, segment); \
+      remain -= segment * sizeof(t); \
+      cursor_from = (void*)(((char*)cursor_from) + segment * sizeof(t)); \
+      cursor_to = (void*)(((char*)cursor_to) + segment * sizeof(t)); \
+    } \
+  } \
+
+void Copy::copy_value_content(const void* from, void* to, size_t size) {
+  // Simple cases first
+  uintptr_t bits = (uintptr_t) from | (uintptr_t) to | (uintptr_t) size;
+  if (bits % sizeof(jlong) == 0) {
+    Copy::conjoint_jlongs_atomic((const jlong*) from, (jlong*) to, size / sizeof(jlong));
+    return;
+  } else if (bits % sizeof(jint) == 0) {
+    Copy::conjoint_jints_atomic((const jint*) from, (jint*) to, size / sizeof(jint));
+    return;
+  } else if (bits % sizeof(jshort) == 0) {
+    Copy::conjoint_jshorts_atomic((const jshort*) from, (jshort*) to, size / sizeof(jshort));
+    return;
+  }
+
+  // Complex cases
+  bits = (uintptr_t) from | (uintptr_t) to;
+  const void* cursor_from = from;
+  void* cursor_to = to;
+  size_t remain = size;
+  COPY_ALIGNED_SEGMENT(jlong)
+  COPY_ALIGNED_SEGMENT(jint)
+  COPY_ALIGNED_SEGMENT(jshort)
+  if (remain > 0) {
+    Copy::conjoint_jbytes((const void*) cursor_from, (void*) cursor_to, remain);
+  }
+}
+
+#undef COPY_ALIGNED_SEGMENT
+
 class CopySwap : AllStatic {
 public:
   /**

src/hotspot/share/utilities/copy.hpp

@@ -161,6 +161,8 @@ class Copy : AllStatic {
   // of two which divides all of from, to, and size, whichever is smaller.
   static void conjoint_memory_atomic(const void* from, void* to, size_t size);
 
+  static void copy_value_content(const void* from, void* to, size_t size);
+
   // bytes,                 conjoint array, atomic on each byte (not that it matters)
   static void arrayof_conjoint_jbytes(const HeapWord* from, HeapWord* to, size_t count) {
     pd_arrayof_conjoint_bytes(from, to, count);

test/hotspot/jtreg/runtime/valhalla/inlinetypes/ValueCopyingTest.java

@@ -0,0 +1,102 @@
+/*
+ * Copyright (c) 2024, 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 ValueCopyingTest
+ * @summary Verify that interpreter doesn't tear up primitive fields when copying a value
+ * @library /test/lib
+ * @modules java.base/jdk.internal.vm.annotation
+ * @enablePreview
+ * @compile ValueCopyingTest.java
+ * @run main/othervm -Xint -XX:+PrintInlineLayout runtime.valhalla.inlinetypes.ValueCopyingTest
+ */
+
+package runtime.valhalla.inlinetypes;
+
+import jdk.internal.vm.annotation.ImplicitlyConstructible;
+import jdk.internal.vm.annotation.LooselyConsistentValue;
+import jdk.internal.vm.annotation.NullRestricted;
+import jdk.test.lib.Asserts;
+
+public class ValueCopyingTest {
+
+  static final int NUM_WORKERS = 16;
+
+  static ValueCopyingTest target = new ValueCopyingTest();
+
+  @ImplicitlyConstructible
+  @LooselyConsistentValue
+  static value class TestValue {
+    int i;
+    byte b;
+    TestValue(int i0) {
+      i = i0;
+      b = 0;
+    }
+  }
+
+  @NullRestricted
+  TestValue tv;
+
+  static class Worker implements Runnable {
+    int i;
+    TestValue v;
+    Worker(byte b) {
+      i = b | (b << 8) | (b << 16) | (b << 24);
+      v = new TestValue(i);
+    }
+
+    static void checkValue(int i) {
+      byte b = (byte)(i & 0xFF);
+      Asserts.assertTrue(((i >> 8) & 0xFF) == b, "Tearing detected");
+      Asserts.assertTrue(((i >> 16) & 0xFF) == b, "Tearing detected");
+      Asserts.assertTrue(((i >> 24) & 0xFF) == b, "Tearing detected");
+    }
+
+    public void run() {
+      for (int n = 0; n < 10000000; n++) {
+        ValueCopyingTest.target.tv = v;
+        int ri = ValueCopyingTest.target.tv.i;
+        checkValue(ri);
+      }
+    }
+  }
+
+  static public void main(String[] args) throws InterruptedException {
+    Thread[] workers = new Thread[NUM_WORKERS];
+    for (int i = 0; i < NUM_WORKERS; i++) {
+      workers[i] = new Thread(new Worker((byte)i));
+    }
+    for (int i = 0; i < NUM_WORKERS; i++) {
+      workers[i].start();
+    }
+    try {
+      for (int i = 0; i < NUM_WORKERS; i++) {
+        workers[i].join();
+      }
+    } catch(InterruptedException e) {
+      e.printStackTrace();
+      throw e;
+    }
+  }
+}