RFR (S) 8173361: various crashes in JvmtiExport::post_compiled_method_load

[Chris Plummer]

On 11/15/19 4:39 AM, coleen.phillimore at oracle.com wrote:
>
>
> On 11/14/19 9:07 PM, Chris Plummer wrote:
>> Hi Coleen,
>>
>> Is it ok to end up missing some CompiledMethodLoad events? The spec 
>> says:
>>
>> "Sent when a method is compiled and loaded into memory by the VM. If 
>> it is unloaded, the CompiledMethodUnload event is sent. If it is 
>> moved, the CompiledMethodUnload event is sent, followed by a new 
>> CompiledMethodLoad event. Note that a single method may have multiple 
>> compiled forms, and that this event will be sent for each form. "
>>
>> So a method was still "compiled and loaded into memory", right? We 
>> just didn't get the event out before it was too late. Is the 
>> CompiledMethodUnload still sent?
>
> Yes, the CompiledMethodUnload event would be sent for this.
>
> My first version of my change reported the event without the extra 
> information (inlining and some code blob address location maps). Maybe 
> that would be better.   Here it is and tested successfully with the 
> testcase that crashed.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/2019/8173361.02/webrev
>
> The more I look at this code, the more problems I see. 
> get_and_cache_jmethod_id() will send back a jmethodID from when the 
> method was live.  But like the class unloading event, a user cannot 
> trust that the Method* in the jmethodID points to anything valid.
>
> So the spec for CompiledMethodLoad event should say for the method, 
> like the CompiledMethodUnload event:
> Compiled method being unloaded. For identification of the compiled 
> method only -- the class may be unloaded and therefore the method 
> should not be used as an argument to further JNI or JVMTI functions.
Yes, I agree that with this approach a spec clarification is needed. I 
just wonder about compatibility for agents that assume it is a valid 
jmethodID. I suppose if an agent treated it as valid and crashed as a 
result, this is just moving the crash from the jvmti impl to the agent. 
We also have to consider that agents might currently treat it as valid 
for functional purposes, and likely never run into this problem, but 
with the spec update technically that would mean that they would no 
longer be able to. However, what's likely is that any existing agent 
would just continue to be ignorant of this spec change, and continue to 
run with no issue.

>
> Or we don't send the event like 01.  Either one doesn't crash.
Yeah, I guess I don't have a good answer here. Seems like both 
approaches have issues. Maybe the correct fix is to keep the nm live 
until the deferred event can be sent.

thanks,

Chris
>
> Thanks,
> Coleen
>
>>
>> thanks,
>>
>> Chris
>>
>> On 11/14/19 5:15 PM, coleen.phillimore at oracle.com wrote:
>>> Summary: Don't post information which uses metadata from unloaded 
>>> nmethods
>>>
>>> Tested tier1-3 and 100 times with test that failed (reproduced 
>>> failure without the fix).
>>>
>>> open webrev at 
>>> http://cr.openjdk.java.net/~coleenp/2019/8173361.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8173361
>>>
>>> Thanks,
>>> Coleen
>>
>