[jdk8u-dev] RFR: 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

[Andrew Dinn]

On Thu, 25 Jul 2024 15:00:49 GMT, Zdenek Zambersky <zzambers at openjdk.org> wrote:

> This backport fixes [failures](https://github.com/zzambers/jdk8u-dev/actions/runs/9858657012/job/27221579883) (segfaults) in following tests which appeared after [macos update](https://github.com/openjdk/jdk8u-dev/pull/544):
> 
> sun/misc/CopyMemory.java 
> compiler/unsafe/OpaqueAccesses.java
> 
> Backport differs from original changeset, because there were significant changes/refactoring in unsafe.
> 
> **Notes:**
> - [original changeset](https://github.com/openjdk/jdk11u-dev/commit/6dc1d8c06d98e127b022886172e16b90bf357c97) changes pointer returned by `addr` (`MemoryAccess` class), to volatile. Otherwise it is basically just refactoring.
> - `MemoryAccess` is used by `Unsafe_{Set,Put}*` and `Unsafe_{Set,Put}*Volatile` functions, defined using `DEFINE_GETSETOOP` and `DEFINE_GETSETOOP_VOLATILE` macros
> - jdk8 does not have `MemoryAccess` class, so equivalent pointers, in functions mentioned higher, are cast to volatile, to achieve same effect
> 
> Testing:
> Tier1: OK (fixes `sun/misc/CopyMemory.java` and `compiler/unsafe/OpaqueAccesses.java` tests on macos, 1 failure on Linux x86 is timeout - seems unrelated, macos failures explained here: https://github.com/openjdk/jdk8u-dev/pull/544#issuecomment-2250636257)

hotspot/src/share/vm/prims/unsafe.cpp line 170:

> 168: #define GET_FIELD(obj, offset, type_name, v) \
> 169:   oop p = JNIHandles::resolve(obj); \
> 170:   type_name v = *(volatile type_name*)index_oop_from_field_offset_long(p, offset)

This patch leaves it rather unclear what is being done here and why. A maintainer might easily be confused as to why `GET_FIELD` and `GET_FIELD_VOLATILE` both treat the field as having a volatile type. Likewise for SET_FIELD and SET_FIELD_VOLATILE.

One solution would be to add a comment here. However, I think it would be clearer to leave `GET_FIELD` and `SET_FIELD` unchanged here and to modify macro `DEFINE_GETSETOOP` so that it appends the volatile prefix to the type parameter passed in calls to `GET_FIELD` and `SET_FIELD` also adding a comment to explain why that is done. i.e.


// Note that the C type passed to GET_FIELD and SET_FIELD is prefixed with a
// volatile qualifier in order to ensure that the C compiler does not reorder
// the object field access wrt to preceding and succeeding volatile accesses
// to the thread flag field (made by UnsafeWrapper) which safeguard the object
// field access. See JDK-8186787.

#define DEFINE_GETSETOOP(jboolean, Boolean) \
 \
UNSAFE_ENTRY(jboolean, Unsafe_Get##Boolean##140(JNIEnv *env, jobject unsafe, jobject obj, jint offset)) \
  UnsafeWrapper("Unsafe_Get"#Boolean); \
  if (obj == NULL)  THROW_0(vmSymbols::java_lang_NullPointerException()); \
  GET_FIELD(obj, offset, volatile jboolean, v); \
  return v; \
UNSAFE_END \
 \
UNSAFE_ENTRY(void, Unsafe_Set##Boolean##140(JNIEnv *env, jobject unsafe, jobject obj, jint offset, jboolean x)) \
  UnsafeWrapper("Unsafe_Set"#Boolean); \
  if (obj == NULL)  THROW(vmSymbols::java_lang_NullPointerException()); \
  SET_FIELD(obj, offset, volatile jboolean, x); \
UNSAFE_END \
 \
UNSAFE_ENTRY(jboolean, Unsafe_Get##Boolean(JNIEnv *env, jobject unsafe, jobject obj, jlong offset)) \
  UnsafeWrapper("Unsafe_Get"#Boolean); \
  GET_FIELD(obj, offset, volatile jboolean, v); \
  return v; \
UNSAFE_END \
 \
UNSAFE_ENTRY(void, Unsafe_Set##Boolean(JNIEnv *env, jobject unsafe, jobject obj, jlong offset, jboolean x)) \
  UnsafeWrapper("Unsafe_Set"#Boolean); \
  SET_FIELD(obj, offset, volatile jboolean, x); \
UNSAFE_END \
 \
// END DEFINE_GETSETOOP.

-------------

PR Review Comment: https://git.openjdk.org/jdk8u-dev/pull/553#discussion_r1971281727