RFR: 8291736: find_method_handle_intrinsic leaks Method* [v3]

[David Holmes]

On Wed, 31 Aug 2022 13:24:18 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>>> This is a subtle interaction that only you noticed.
>> 
>> You mean in this review?  The original code clearly knew this had to be done outside the lock:
>> 
>> http://hg.openjdk.java.net/jdk7/jdk7/hotspot/rev/e5b0439ef4ae#l33.106
>> 
>> Now perhaps the current code is actually safe (as @iklam suggests) because it can't actually invoke any Java code? But that is something we would need to establish. In any case it seems an anti-pattern to allow any kind of use of CHECK from inside a locked region when we "know" we always have to throw exceptions outside of locked regions.
>
> The problem with the original code was the ranking and multi-purpose use of the SystemDictionary_lock.  Creating the adapters takes out a lock whose ranking is above SystemDictionary_lock.  Also the tables (and lock for the tables) were shared with code that could call Java code for calling Java to create MethodType, so that code had to release the lock.  Leaking the java.lang.invoke.MethodType is ok because it'll be GC'd.
> 
> We've leaked the Method* and adapter for many years now, maybe it's fine to keep leaking it and I should just close this and not try to fix it.
> 
> The case of throwing exceptions inside Mutex locked regions is something I thought we have all over the source code.  I couldn't find any explicit calls, but the pattern of doing metaspace allocation inside of a MutexLocker is in enough places that adding an   assert(!thread->owns_locks(), "must release all locks when possibly throwing exceptions"); during in the TRAPS version of Metaspace::allocate() fails immediately.
> 
> But it may be that the OOM for Metaspace allocation failure doesn't call into Java for constructing the object and this part is fine.

Right, as per our side-bar discussion, while the general rule is "no throwing exceptions while holding a VM mutex/monitor" it seems that it is safe to only throw OutOfMemoryError because `Universe::gen_out_of_memory_error` doesn't execute any Java code. It either uses one of the pre-allocated OOME instances (if available) and fills in the stacktrace, or else (or in case of secondary OOM) it uses the pre-allocated stackless singleton instance.  This should be documented somewhere of course (and perhaps even programmatically verified somehow `NoJavaCodeVerifier`? :) )

Also whilst the lower-level code makes it somewhat clear that the only exception that can arise is the metaspace OOME, it is not at all clear just by looking at a callsite like:

methodHandle m = Method::make_method_handle_intrinsic(iid, signature, CHECK_NULL);

so perhaps we even need additional macros like CHECK_OOM to verify only OOME can be thrown? (Not in this PR.)

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

PR: https://git.openjdk.org/jdk/pull/9983