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

[David Holmes]

Agreed! Good to use consistent code style in these related functions.

Thanks,
David

On 24/06/2016 5:32 AM, Coleen Phillimore wrote:
>
> 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
>>>>>
>>>>
>>>
>>
>