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 <dhol...@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
