RFR: 8247992: [JVMCI] HotSpotNmethod.executeVarargs can try execute a zombie nmethod

[Erik Österlund]

Hi Doug,

It would be nice if we could get rid of some cluttering of nested

#if JVMCI
   if (blah) {
#endif
...
#if JVMCI
   }
#endif

It makes it hard to read for (to me) no obvious reason.

Here is a patch on top of yours to remove some of that:
http://cr.openjdk.java.net/~eosterlund/8247992/webrev.doug..00/

Full webrev:
http://cr.openjdk.java.net/~eosterlund/8247992/webrev.00/

The reasoning in my changes is that since the SharedRuntime wrong method 
handler has no idea about these alternative call targets, checking if 
the nmethod is still entrant right before the call, will not guarantee 
that we actually end up calling the target nmethod anyway. It could get 
made not entrant just after the call, but before executing the first 
instruction, which when becoming not entrant jumps to the wrong method 
handler, which has no clue about any of this and just re-routes the call 
to the currently best method code of the method used in the Java call 
wrapper, without throwing any exception. So either we can live with 
that, or we can not. If we can live with that, then all exception 
handling can (and should) be removed from the Java call wrapper, because 
it is not needed. Instead, we check if the nmethod is still entrant in 
the JVMCI caller function, and accept that it could subsequently become 
not entrant at any point in time. A few more instructions for a slippery 
race to happen, but not worth cluttering the call wrapper with JVMCI 
code over the difference.

Now I don't know how this API is used, but would it make sense to 
instead check *after* the nmethod has been executed, if it is still 
in_use()? Then we know that the nmethod has been executed to completion 
without getting killed off, if we do not get an exception. And if we get 
an exception then it got deoptimized either right before calling, or 
during its execution. Like this:

http://cr.openjdk.java.net/~eosterlund/8247992/webrev.00..01/

That would completely close the window for that race, but assumes the 
caller is okay with getting an exception if the nmethod gets deoptimized 
while running. You know this API better than I do, so that's your call 
if it is a good idea or not.

Thanks,
/Erik

On 2020-06-22 13:12, Doug Simon wrote:
> Hi, can I please get some reviews for this patch that fixes a race 
> described in JDK-8247992 
> <https://bugs.openjdk.java.net/browse/JDK-8247992>.
>
> The fix in the patch is to pass the HotSpotNmethod mirror object (in a 
> handle) through the problematic VM-to-Java transition and only after 
> the transition will the verified entry point be extracted from the 
> mirror. This will prevent a zombie nmethod from being executed.
>
> It’s still possible for the call to not reach the “alternative target” 
> due to sweeping heuristics. This is an acceptable limitation of having 
> an alternative call target in JavaCallWrapper. It could be fixed by 
> altering CPU specific i2c trampoline code but the added complexity is 
> not worth it for this test-only mechanism. Tests using this API can 
> detect whether the alternative target was actually called if they 
> really care about it.
>
> Thanks to Erik Osterlund for the in-depth analysis and suggested 
> solutions.
>
> https://dougxc.github.io/webrevs/8247992_16/index.html
> https://bugs.openjdk.java.net/browse/JDK-8247992
>
> Testing: hs-tier1,hs-tier2,hs-tier3-graal
>
> -Doug