RFR (M) 8212160: JVMTI agent crashes with "assert(_value != 0LL) failed: resolving NULL _value"

[Daniel D. Daugherty]

Generally speaking, JVM/TI related things should be in JvmtiThreadState
instead of directly in the Thread class. That way the extra space is only
consumed when JVM/TI is in use and only when a Thread does something that
requires a JvmtiThreadState to be created.

Please reconsider moving _jvmti_event_queue.

Dan


On 12/4/19 6:06 PM, coleen.phillimore at oracle.com wrote:
>
> Hi Serguei,
>
> On 12/4/19 5:15 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Collen, (no problem)
>>
>> It looks good in general.
>> Thank you a lot for sorting this out!
>>
>> Just a couple of comments.
>>
>>
>> http://cr.openjdk.java.net/~coleenp/2019/8212160.01/webrev/src/hotspot/share/runtime/thread.hpp.frames.html
>> 1993 protected:
>> 1994 // Jvmti Events that cannot be posted in their current context.
>> 1995 // ServiceThread uses this to collect deferred events from 
>> NonJava threads
>> 1996 // that cannot post events.
>> 1997 JvmtiDeferredEventQueue* _jvmti_event_queue;
>>
>> As David I also have a concern about footprint of having the 
>> _jvmti_event_queue field in the Thread class.
>> I'm thinking if it'd be better to move this field into the 
>> JvmtiThreadState class.
>> Please, see jvmti_thread_state() and 
>> JvmtiThreadState::state_for(JavaThread *thread).
>
> The reason I have it directly in JavaThread is so that the GC oops_do 
> and nmethods_do code can find it easily.  I like your idea of hiding 
> it in jvmti but this doesn't seem good to have this code know about 
> jvmtiThreadState, which seems to be a queue of Jvmti states.  I also 
> don't want to have jvmtiThreadState to have to add an oops_do() or 
> nmethods_do() either.
>
>>
>>
>> http://cr.openjdk.java.net/~coleenp/2019/8212160.01/webrev/src/hotspot/share/prims/jvmtiImpl.cpp.frames.html
>> 973 void JvmtiDeferredEvent::post(JvmtiEnv* env) {
>> 974 assert(_type == TYPE_COMPILED_METHOD_LOAD, "only user of this 
>> method");
>> 975 nmethod* nm = _event_data.compiled_method_load;
>> 976 JvmtiExport::post_compiled_method_load(env, nm);
>> 977 }
>>
>> The JvmtiDeferredEvent::post name looks too generic as it posts 
>> compiled load events only.
>> Do you consider this function extended in the future to support more 
>> event types?
>>
>
> I don't envision an extension for this function but I do for 
> JvmtiDeferredEventQueue::post().  I have a small enhancement that 
> would handoff the entire queue to the ServiceThread and have it call 
> post() to post all the events rather than one at a time.
>
> So I'll rename this one post_compiled_method_load_event() and leave 
> the other post() as is for now.
>
> open webrev at 
> http://cr.openjdk.java.net/~coleenp/2019/8212160.02.incr/webrev
>
> Thanks,
> Coleen
>
>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 11/26/19 06:22, coleen.phillimore at oracle.com wrote:
>>> Summary: Add local deferred event list to thread to post events 
>>> outside CodeCache_lock.
>>>
>>> This patch builds on the patch for JDK-8173361.  With this patch, I 
>>> made the JvmtiDeferredEventQueue an instance class (not AllStatic) 
>>> and have one per thread.  The CodeBlob event that used to drop the 
>>> CodeCache_lock and raced with the sweeper thread, adds the events it 
>>> wants to post to its thread local list, and processes it outside the 
>>> lock.  The list is walked in GC and by the sweeper to keep the 
>>> nmethods from being unloaded and zombied, respectively.
>>>
>>> Also, the jmethod_id field in nmethod was only used as a boolean so 
>>> don't create a jmethod_id until needed for post_compiled_method_unload.
>>>
>>> Ran hs tier1-8 on linux-x64-debug and the stress test that crashed 
>>> in the original bug report.
>>>
>>> open webrev at 
>>> http://cr.openjdk.java.net/~coleenp/2019/8212160.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8212160
>>>
>>> Thanks,
>>> Coleen
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20191204/94daedef/attachment-0001.html>