RFR: 8303624: The java.lang.Thread.FieldHolder can be null for JNI attaching threads [v3]

[Daniel D. Daugherty]

On Thu, 9 Mar 2023 05:33:48 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> To support virtual threads a number of fields were moved out of `java.lang.Thread` into a separate `FieldHolder` object. The VM was updated to then access certain thread fields via the `FieldHolder`. 
>> 
>> The code for attaching a thread to the VM, specifically `allocate_threadObj` didn't allow for the `Thread` constructor throwing an exception, and so failing to allocate the `FieldHolder` before attempting to access a field through the `FieldHolder`. This resulted in assertion failures in `javaClasses.cpp` (or crashes in a product build). That code is fixed to ensure we cease processing if the constructor throws an exception.
>> 
>> In addition, we need to recognise that whilst a native thread is attaching via JNI, it is partially initialized (to varying degrees) but also visible through JVMTI. Though the window is small JVMTI could get hold of an attaching thread and then invoke methods that would try to access uninitialized state. When this was state of `Thread` instance there was no problem as the object existed in its zero-initialized form (it had been allocated directly but no constructor run). However, anythng accessed via the `FieldHolder` is now a problem as the `FieldHolder` may not exist. So we modify all of the `FieldHolder` get/set methods to account for it being null: setters will do nothing, while getters return the default zero value for the field.
>> 
>> Testing: tiers 1-3 as a sanity test
>> 
>> There's no way to write a regression test for this.
>> 
>> Thanks.
>
> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update comment per @alanb

Thumbs up. My only comments are nits.

I like how much cleaner the code is now.

src/hotspot/share/classfile/javaClasses.cpp line 1603:

> 1601: 
> 1602: oop java_lang_Thread::holder(oop java_thread) {
> 1603:   // Note: may be null if the thread is still attaching

nit perhaps: s/may be null/may return null/

src/hotspot/share/classfile/javaClasses.cpp line 1715:

> 1713:          JavaThread::current()->thread_state() == _thread_in_vm,
> 1714:          "Java Thread is not running in vm");
> 1715:   GET_FIELDHOLDER_FIELD(java_thread, get_thread_status, JavaThreadStatus::NEW;  /* not initialized */);

I didn't expect to see a ';' after the 'NEW'. Should that be deleted?
Should it have caused a compile error?

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

Marked as reviewed by dcubed (Reviewer).

PR: https://git.openjdk.org/jdk/pull/12892