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

[Daniel D. Daugherty]

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

         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.

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/222a983a/attachment-0001.html>