RFR: 8038332: The trace event vm/class/load is not always being sent

[Max Ockner]

Replies inline.
Thanks,
Max
On 6/22/2016 3:13 PM, Coleen Phillimore wrote:
>
> Hi,
>
> Can I suggest actually fixing this code?  Please?
>
Sure!
> Instead of returning THREAD in all the calls in parse_stream(), they 
> should have CHECK_NULL.   Then all the if (!HAS_PENDING_EXCEPTION) 
> conditionals can be removed and the logic fixed.
>
This has been changed for each occurrence of THREAD except for in 
MutexLocker and ResourceMark.
> The only reason to have THREAD as the last parameter is if there's 
> cleanup that needs to be done in the case of an exception, which is 
> rare and I verified not the case in this function.
>
> After this code put a return NULL;
>
>     Exceptions::_throw_msg(THREAD_AND_LOCATION,
>       vmSymbols::java_lang_SecurityException(), message);
> //  add return NULL;
>   }
>
I have followed Ioi's suggestion here to use

     THROW_MSG_NULL(vmSymbols::java_lang_SecurityException(), message);

> Otherwise, the code to add the event looks good.
>
> Thanks,
> Coleen
>
>
> On 6/22/16 1:53 PM, Jiangli Zhou wrote:
>> Hi Max,
>>
>> I see there is already an ‘if (!HAS_PENDING_EXCEPTION)’ check at line 
>> 1170 before the debug_only code. Maybe you cloud  place the 
>> post_class_load_event() in there so it only post the event when there 
>> is no pending exception. That way you don’t need to change the 
>> existing logic and add the additional checks.
>>
>> Thanks,
>> Jiangli
>>
>>> On Jun 20, 2016, at 12:08 PM, Max Ockner <max.ockner at oracle.com> wrote:
>>>
>>> David,
>>>
>>> New webrev: 
>>> http://cr.openjdk.java.net/~mockner/8038332.02/src/share/vm/classfile/systemDictionary.cpp.cdiff.html
>>> I have added the check you suggested before triggering the event:
>>>
>>> if (HAS_PENDING_EXCEPTION || k.is_null()) {
>>>     return NULL;
>>> }
>>>
>>> Thanks,
>>> Max
>>>
>>> On 6/13/2016 6:40 PM, David Holmes wrote:
>>>> Hi Max,
>>>>
>>>> On 14/06/2016 3:42 AM, Max Ockner wrote:
>>>>> Jiangli,
>>>>> Thanks for looking.  I didn't see anything that looked like it might
>>>>> produce duplicate events. However, I did see some additional places
>>>>> where it looks like no event is fire.
>>>>>
>>>>> Can anyone point me to the event specs?
>>>> I'm not sure there is any spec for this. Even JFR doesn't seem to 
>>>> document individual events and when they are triggered.
>>>>
>>>> Your change does not look right however as you are posting the 
>>>> classload event regardless of the exception state. If you look at 
>>>> SystemDictionary::resolve_instance_class_or_null, it only posts 
>>>> after checking the load was successful:
>>>>
>>>> 892   if (HAS_PENDING_EXCEPTION || k.is_null()) {
>>>> 893     return NULL;
>>>> 894   }
>>>> 895
>>>> 896   post_class_load_event(class_load_start_time, k, class_loader);
>>>>
>>>> Similarly, SystemDictionary::parse_stream, has various CHECK macros 
>>>> that will cause a return on exception, prior to getting to the 
>>>> point of posting the load event. So you also need to add a check 
>>>> for a pending exception and that k is not null, I think, before 
>>>> posting the event.
>>>>
>>> I have added this check.
>>>> BTW I would have expected to see trace-events generated at 
>>>> approximately the same locations as the corresponding JVMTI events. 
>>>> That does not seem to be the case which seems very strange to me. 
>>>> The notion of "loading a class" seems to be spread across far too 
>>>> many functions to me.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks,
>>>>> Max
>>>>>
>>>>> On 6/9/2016 4:39 PM, Jiangli Zhou wrote:
>>>>>> Hi Max,
>>>>>>
>>>>>> Looks ok. The only possible issue is more than one event might be 
>>>>>> sent
>>>>>> in some of the call paths. But my quick search didn’t find any of 
>>>>>> such
>>>>>> case.
>>>>>>
>>>>>> Thanks,
>>>>>> Jiangli
>>>>>>
>>>>>>> On Jun 9, 2016, at 11:05 AM, Max Ockner <max.ockner at oracle.com> 
>>>>>>> wrote:
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review this small fix which causes the vm/class/load 
>>>>>>> event to
>>>>>>> be fired from JVM_DefineClass() and JVM_DefineClassWithSource().
>>>>>>>
>>>>>>> Webrev: http://cr.openjdk.java.net/~mockner/8038332/
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8038332
>>>>>>>
>>>>>>> The vm/class/load event  (EventClassLoad) was previously fired in 2
>>>>>>> places:
>>>>>>>
>>>>>>> SystemDictionary::parse_stream
>>>>>>> SystemDictionary::resolve_instance_class_or_null
>>>>>>>
>>>>>>> parse_stream is the standard option for creating a klass from a
>>>>>>> stream, but JVM_DefineClass uses a different function:
>>>>>>>
>>>>>>> SystemDictionary::resolve_from_stream.
>>>>>>>
>>>>>>> This did not fire a vm/class/load event. Now it does fire the 
>>>>>>> event.
>>>>>>>
>>>>>>> Sanity tested with jtreg runtime. Issue was reproduced and tested
>>>>>>> using the reproducer script attached to the bug
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Max
>