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

[Zdenek Zambersky]

On Wed, 26 Feb 2025 10:07:24 GMT, Andrew Dinn <adinn at openjdk.org> wrote:

>> 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.
>
> @zzambers @jerboaa
> 
> Suggestion:
> 
> I'd really prefer it if we could rename the parameters `jboolean` and `Boolean` in macro `DEFINE_GETSETOOP` to something like `ctypename` and `javatypename` (or maybe just `ctype and javatype`). That makes what is going on much clearer than with the current version -- where, somewhat bizarrely, the parameters are named using the arguments passed at the first call.
> 
> However, that's not a necessary change and the policy is not to change things that ain't broke. So, I'm not going to push this point. Whatever you feel is appropriate.

@adinn I think passing `volatile` as part of `type_name` would be problematic, because it is also used in `SET_FIELD` macro to derive name of another macro `truncate_##type_name(x)`. Comment sound like a good idea.

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

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