RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v15]

[Jorn Vernee]

Hi David,

On 09/11/2020 14:51, David Holmes wrote:
> Hi Jorn,
>
> On 9/11/2020 9:57 pm, Jorn Vernee wrote:
>> On Mon, 9 Nov 2020 03:29:05 GMT, David Holmes <dholmes at openjdk.org> 
>> wrote:
>>
>>> src/hotspot/cpu/aarch64/universalUpcallHandler_aarch64.cpp line 99:
>>>
>>>> 97:   if (thread == NULL) {
>>>> 98:     JavaVM_ *vm = (JavaVM *)(&main_vm);
>>>> 99:     vm -> functions -> AttachCurrentThreadAsDaemon(vm, &p_env, 
>>>> NULL);
>>>
>>> Style nit: don't put spaces around `->` operator.
>>>
>>> What is the context for this being called? It looks highly 
>>> suspicious to just attach the current thread to the VM this way.
>>
>> The context is a thread that is spawned by native code doing an 
>> upcall. We need to attach the thread to the VM first in that case. 
>> Normally this would be handled by the calling code, but in our case 
>> the calling code doesn't know it's calling into Java.
>
> Apologies that I don't have enough knowledge of this feature to 
> understand the context. Shouldn't you then detach it again afterwards? 
> Otherwise when will it detach? If you don't detach you will get a 
> memory leak.
I went back to look at the original thread that added this code [1]. At 
that point the decision was made to go with the memory leak to avoid 
additional complexity in assembly stub generation and performance loss 
from having to repeatedly attach and detach threads.

At least the complexity argument is no longer valid. As for the 
performance argument. This implementation of upcalls is not expected to 
perform stellar, so I think calling detach again in case we did an 
attach is the right call. (Though we'll have to solve the same problem 
again down the line for the optimized version).

Thanks for catching,
Jorn

[1] : 
https://mail.openjdk.java.net/pipermail/panama-dev/2019-June/005761.html
>>> src/java.base/share/classes/jdk/internal/invoke/NativeEntryPoint.java 
>>> line 63:
>>>
>>>> 61:     }
>>>> 62:
>>>> 63:     public static NativeEntryPoint make(long addr, String name, 
>>>> ABIDescriptorProxy abi, VMStorageProxy[] argMoves, VMStorageProxy[] 
>>>> returnMoves,
>>>
>>> Where is name validation performed, to ensure the named native 
>>> method is in fact legal and not trying to provide backdoor access to 
>>> native code that should be encapsulated and protected?
>>
>> The name is just used as debugging information (in e.g. 
>> CallNativeNode::dump_spec), we are not looking it up. The address 
>> that is passed there is the actual function target.
>
> Okay.
>
> Thanks,
> David
> -----
>
>> -------------
>>
>> PR: https://git.openjdk.java.net/jdk/pull/634
>>