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

[Coleen Phillimore]

Hi,

Can I suggest actually fixing this code?  Please?

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.

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;
   }

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