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

[coleen.phillimore at oracle.com]

On 11/15/19 2:13 PM, Chris Plummer wrote:
> 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.

I'm not sure I'm following this.  Yes, if an agent tries to do something 
with the jmethodID, like call it, in the case where it has been 
unloaded, it will crash.

The nmethod was unloaded because some oop in it was no longer valid.  It 
may be that the class is unloaded, in which case the Method* in the 
jmethodID is a bad pointer.  So that was a preexisting problem.

The CompiledMethodLoad event saves the jmethodID from when the nmethod 
was created, so it's good at this point.  If the nmethod is unloaded 
because some *other* oop in it is bad, the Method* would still be 
valid.   My change retrieves the one saved when the load event was 
created.  It's only the case if the nmethod is unloaded because the 
class containing the Method* in _method is unloaded that would get a crash.

The specification should still be clarified to say this though.

>
>>
>> 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.

This is not feasable, since the nmethod may have bad oops in it.

Coleen
>
> 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
>>>
>>
>
>