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

[Coleen Phillimore]

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.

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