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

[Tom Rodriguez]

I agree it shouldn't throw if the method was invalidated when the caller 
returns.  That's something the caller can check if they care.  Looks good.

tom

Doug Simon wrote on 6/22/20 9:25 AM:
> 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 
>> <mailto: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 
>>> <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
>>
>