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

[Max Ockner]

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