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

[Maurizio Cimadamore]

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
>>