[foreign] RFR 8226250: Calling callback from different native threads causes a VM crash

[Sundararajan Athijegannathan]

Looks good.

-Sundar

On 18/06/19, 1:35 AM, Maurizio Cimadamore wrote:
> Thanks for the catch - I also wonder if there's an already made 
> function to retrieve the 'main_vm' from inside the VM itself (not in 
> JNI).
>
> Maurizio
>
>
> On 17/06/2019 21:02, Henry Jen wrote:
>> Looks reasonable to me, a nit though,
>>
>> JavaVM_ *vm = (JavaVM *)(&main_vm);
>>
>> This line really bothers me, the cast and target type is different, 
>> although it’s correct under cpp.
>> I would suggest make sure we use JavaVM all the way after cast with 
>> something like following for all 4 files.
>>
>> Cheers,
>> Henry
>>
>>
>>
>> diff -r 3b046562fe37 src/hotspot/cpu/x86/universalUpcallHandler_x86.cpp
>> --- a/src/hotspot/cpu/x86/universalUpcallHandler_x86.cpp        Mon 
>> Jun 17 12:44:44 2019 -0700
>> +++ b/src/hotspot/cpu/x86/universalUpcallHandler_x86.cpp        Mon 
>> Jun 17 12:57:23 2019 -0700
>> @@ -189,8 +189,8 @@
>>
>>     JavaThread* thread = JavaThread::current();
>>     if (thread == NULL) {
>> -    JavaVM_ *vm = (JavaVM *)(&main_vm);
>> -    vm -> functions -> AttachCurrentThreadAsDaemon(vm, &p_env, NULL);
>> +    JavaVM *vm = (JavaVM *)(&main_vm);
>> +    vm->AttachCurrentThreadAsDaemon(&p_env, NULL);
>>       thread = JavaThread::current();
>>     }
>>
>>
>>> On Jun 17, 2019, at 10:14 AM, Maurizio Cimadamore 
>>> <maurizio.cimadamore at oracle.com> wrote:
>>>
>>> Hi,
>>> Under JNI, it is user responsibility to attach native threads to the 
>>> VM using attachCurrentThread/detachCurrentThread.
>>>
>>> Panama doesn't do this and, as a result, if native code creates a 
>>> new thread and uses it to call a Java callback, an hard crash occurs.
>>>
>>> This patch adds a call to AttachCurrentThreadAsDaemon before calling 
>>> the Java callback, only if the native thread has no associated Java 
>>> thread.
>>>
>>> The AttachCurrentThreadAsDaemon allows the VM to be shutdown w/o 
>>> waiting for native threads to complete, which is, I think the 
>>> semantics we want.
>>>
>>> We don't call the dual detach method for two reasons:
>>>
>>> * re-attaching the same thread over and over can be expensive
>>>
>>> * calling detach after the callback means that we'd lose some 
>>> register values, so we would additional save/restore
>>>
>>> Overall, it seems like the 'leak' of JavaThread is not worth the 
>>> extra complexity?
>>>
>>> Webrev:
>>>
>>> http://cr.openjdk.java.net/~mcimadamore/panama/8226250_v2/
>>>
>>> Maurizio
>>>