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

[Erik Österlund]

Hi Doug,

Looks good.

Thanks,
/Erik

>> On 22 Jun 2020, at 18:25, Doug Simon <doug.simon at oracle.com> wrote:
> Thanks Erik,
> 
> Almost every one of your suggestions makes sense to me - thanks a lot! I’ve incorporated them into https://dougxc.github.io/webrevs/8247992_16.02/
> 
> The only thing I’m not sure about is the check after the nmethod has been executed. It means we can execute the alternative nmethod but still throw an InvalidInstalledCodeException. This is problematic for an alternative nmethod with a side effect (e.g. it changes a static field). As such, I think the better option is to update the javadoc for HotSpotNmethod.executeVargs as follows:
> 
>     /**
>      * {@inheritDoc}
>      *
>      * It's possible for the HotSpot runtime to sweep nmethods at any point in time. As a result,
>      * there is no guarantee that calling this method will execute the wrapped nmethod. Instead, it
>      * may end up executing the bytecode of the associated {@link #getMethod() Java method}. Only if
>      * {@link #isValid()} is {@code true} after returning can the caller be sure that the nmethod
>      * was executed. If {@link #isValid()} is {@code false}, then the only way to determine if the
>      * nmethod was executed is to test for some side-effect specific to the nmethod (e.g., update to
>      * a field) that is not performed by the bytecode of the associated {@link #getMethod() Java
>      * method}.
>      */
> 
> The javadoc update is also part of 8247992_16.02.
> 
> -Doug
> 
>> On 22 Jun 2020, at 15:49, Erik Österlund <erik.osterlund at oracle.com> wrote:
>> 
>> 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.
>>> 
>>> 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