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

[serguei.spitsyn at oracle.com]

Some additional details on this...
The side effect of a nmethodLocker::lock_nmethod(nm) call
is the nmethod::is_locked_by_vm() returns true.
It is checked in the nmethod::flush(), nmethod::can_convert_to_zombie()
and some other places.
If it is still not safe to look at metadata when the nmethod is locked
then there is some code which does not honor this locking convention.

Thanks,
Serguei


On 11/15/19 1:45 PM, serguei.spitsyn at oracle.com wrote:
> Hi Coleen,
>
> I have some questions.
>
> Both the compiler method load and unload are posted as deferred events.
> Both events keep the nmethod alive until the ServiceThread processes 
> the event.
>
> The implementation is:
>
> JvmtiDeferredEvent 
> JvmtiDeferredEvent::compiled_method_load_event(nmethod* nm) {
>   . . .
>   // Keep the nmethod alive until the ServiceThread can process
>   // this deferred event.
>   nmethodLocker::lock_nmethod(nm);
>   return event;
> }
>
> JvmtiDeferredEvent 
> JvmtiDeferredEvent::compiled_method_unload_event(nmethod* nm, 
> jmethodID id, const void* code) {
>   . . .
>   // Keep the nmethod alive until the ServiceThread can process
>   // this deferred event. This will keep the memory for the
>   // generated code from being reused too early. We pass
>   // zombie_ok == true here so that our nmethod that was just
>   // made into a zombie can be locked.
>   nmethodLocker::lock_nmethod(nm, true /* zombie_ok */);
>   return event;
> }
>
> void JvmtiDeferredEvent::post() {
>   assert(ServiceThread::is_service_thread(Thread::current()),
>          "Service thread must post enqueued events");
>   switch(_type) {
>     case TYPE_COMPILED_METHOD_LOAD: {
>       nmethod* nm = _event_data.compiled_method_load;
>       JvmtiExport::post_compiled_method_load(nm);
>       // done with the deferred event so unlock the nmethod
>       nmethodLocker::unlock_nmethod(nm);
>       break;
>     }
>     case TYPE_COMPILED_METHOD_UNLOAD: {
>       nmethod* nm = _event_data.compiled_method_unload.nm;
>       JvmtiExport::post_compiled_method_unload(
>         _event_data.compiled_method_unload.method_id,
>         _event_data.compiled_method_unload.code_begin);
>       // done with the deferred event so unlock the nmethod
>       nmethodLocker::unlock_nmethod(nm);
>       break;
>     }
>     . . .
>   }
> }
>
> Then I wonder how is it possible for the nmethod to be not alive here?:
> 2168 void JvmtiExport::post_compiled_method_load(nmethod *nm) {
> . . .
> 2173 // It's not safe to look at metadata for unloaded methods.
> 2174 if (!nm->is_alive()) {
> 2175 return;
> 2176 }
> At least, it lokks like something else is broken.
> Do I miss something important here?
>
> Thanks,
> Serguei
>
>
> 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/4be4061e/attachment.html>