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

[coleen.phillimore at oracle.com]

Thanks Dan for looking at this.

On 11/15/19 11:29 AM, Daniel D. Daugherty wrote:
> On 11/15/19 7:48 AM, coleen.phillimore at oracle.com wrote:
>> I meant to add myself to the To list.
>>
>> On 11/15/19 7: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
>
> src/hotspot/share/prims/jvmtiCodeBlobEvents.cpp
>     No comments.
>
> src/hotspot/share/prims/jvmtiExport.cpp
>     L2132:   // It's not safe to look at metadata for unloaded methods.
>     L2133:   if (nm->method() == NULL) {
>     L2134:     return NULL;
>         nit - The comment is about why we're returning NULL early so
>         perhaps the comment should be inside the if-statement (between
>         L2133 and L2134).
>

Sure, I can change that.
>         In the previous version (01) you checked (!nm->is_alive()) in a
>         different place. That function is defined:
>
>           bool  is_alive() const { return _state < unloaded; }
>
>         And states like unloaded are defined like this:
>
>   enum { not_installed = -1, // in construction, only the owner doing 
> the construction is
>                              // allowed to advance state
>          in_use        = 0,  // executable nmethod
>          not_used      = 1,  // not entrant, but revivable
>          not_entrant   = 2,  // marked for deoptimization but 
> activations may still exist,
>                              // will be transformed to zombie when all 
> activations are gone
>          unloaded      = 3,  // there should be no activations, should 
> not be called, will be
>                              // transformed to zombie by the sweeper, 
> when not "locked in vm".
>          zombie        = 4   // no activations exist, nmethod is ready 
> for purge
>   };
>
>         so by switching to (nm->method() == NULL) you are going more
>         directly to whether there is useful data available, but it
>         might be racier. I'm not sure at which state nm->method()
>         starts to return NULL. I'm also not sure if nm->method() can
>         return NULL for any of the earlier states, e.g., not_used.
>
>         I agree with your earlier comment that nm->is_alive() seems
>         safer. Your call on which to use.

I think my earlier comment was wrong.  If the method is unloaded, the 
_method field is zeroed before the unloaded _state is set. With 
concurrent class unloading, this event posting can run at the same time 
as make_unloaded(), if I'm reading this correctly.

I should actually use Atomic::load to get the _method, in both of the 
places where I get the field.

The _method is also zeroed with make_zombie call but this is blocked out 
by the nmethodLocker.

Thanks,
Coleen


>
> Thumbs up. My only non-theory comment is a nit so I don't need to
> see a new webrev.
>
> Dan
>
>
>>>
>>> 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.
>>>
>>> Or we don't send the event like 01.  Either one doesn't crash.
>>>
>>> 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
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20191115/1d3238a5/attachment.html>