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

[Coleen Phillimore]

This looks so much better.
Thanks!
Coleen

On 6/23/16 2:28 PM, Max Ockner wrote:
> Hello again!
>
> New webrev: http://cr.openjdk.java.net/~mockner/8038332.03/
>
> Thanks,
> Max
>
>
> On 6/22/2016 10:53 PM, Coleen Phillimore wrote:
>>
>>
>> On 6/22/16 5:02 PM, Ioi Lam wrote:
>>> Instead of using Exceptions::_throw_msg directly, I think it's 
>>> better to use the macro
>>>
>>>     THROW_MSG_NULL(vmSymbols::java_lang_SecurityException(), message);
>>>
>>> It will do the "return NULL" for you, so there's no danger of 
>>> forgetting doing it.
>>
> Done.
>
>> Agree.
>> Coleen
>>>
>>> Thanks
>>> - Ioi
>>>
>>> On 6/22/16 12:13 PM, Coleen Phillimore wrote:
>>>>
>>>> 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
>>>>
>>>
>>
>