[foreign-memaccess+abi] RFR: 8270851: Logic for attaching/detaching native threads could be improved [v2]

[Maurizio Cimadamore]

On Tue, 29 Mar 2022 01:51:40 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
>> 
>>  - Fix whitespaces
>>  - Merge branch 'foreign-memaccess+abi' into upcall_lazy_detach
>>  - Merge branch 'foreign-memaccess+abi' into upcall_lazy_detach
>>  - Remove whitespaces, fix indent
>>  - Lazily deattach native threads using thread local storage destructors
>
> src/hotspot/share/prims/universalUpcallHandler.cpp line 67:
> 
>> 65:     JNIEnv* p_env = nullptr; // unused
>> 66:     jint result = vm->functions->AttachCurrentThread(vm, (void**) &p_env, nullptr);
>> 67:     guarantee(result == JNI_OK, "Could not attach thread for upcall. JNI error code: %d", result);
> 
> Do you really want to abort if something goes wrong i.e. out-of-memory**? (Also the JNI error code won't be very illuminating as only JNI_ERR is ever returned from AttachCurrentThread.)
> 
> ** Any unexpected exceptions from the java.lang.Thread construction will be silently cleared but also cause failure.

For errors that occurs inside the upcall, we generally try to print a stack trace - but in this case we're a bit of a limbo between native and Java - and we still don't have a Java thread - so I'm assuming that if something goes wrong here there's not much we can do to dress it up nicely and/or recover? I agree that printing out JNI error code seems over the top (and not very useful).

I note that this code has not changed, even in the foreign-preview branch, which contains a bunch of JVM cleanups. Maybe we can file a followup (but I'd like to understand better what you are proposing - and what options we have on the table).

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

PR: https://git.openjdk.java.net/panama-foreign/pull/570