RFR (S) 8203356: VM Object Allocation Collector can infinite recurse

[Alex Menkov]

+1

--alex

On 08/24/2018 15:00, serguei.spitsyn at oracle.com wrote:
> Look good, thank you for the update!
> 
> Thanks,
> Serguei
> 
> 
> On 8/24/18 14:58, JC Beyler wrote:
>> Thanks for the review Serguei and nice catches! Here is the webrev 
>> with your comments fixed:
>> http://cr.openjdk.java.net/~jcbeyler/8203356/webrev.01/ 
>> <http://cr.openjdk.java.net/%7Ejcbeyler/8203356/webrev.01/>
>>
>> Let me know what you think,
>> Jc
>>
>> On Fri, Aug 24, 2018 at 9:55 AM serguei.spitsyn at oracle.com 
>> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com 
>> <mailto:serguei.spitsyn at oracle.com>> wrote:
>>
>>     One more suggestion for simplification:
>>
>>        56   char *generic = NULL;
>>        57
>>        58   (*jvmti)->GetClassSignature(jvmti, klass, &signature, &generic);
>>        59
>>
>>       The value returned in generic is not used.
>>       You can pass NULL in place of @generic and get rid of line #56.
>>
>>     Thanks,
>>     Serguei
>>
>>
>>     On 8/24/18 09:41, serguei.spitsyn at oracle.com
>>     <mailto:serguei.spitsyn at oracle.com> wrote:
>>>     Hi Jc,
>>>
>>>     It looks good in general, thank you for taking care about this!
>>>     A couple of comments though.
>>>
>>>     http://cr.openjdk.java.net/%7Ejcbeyler/8203356/webrev.00/test/hotspot/jtreg/serviceability/jvmti/VMEvent/libVMEventTest.c.html
>>>
>>>        58   (*jvmti)->GetClassSignature(jvmti, klass, &signature, &generic);
>>>        59
>>>        60   if (JNI_ENV_PTR(jni)->ExceptionOccurred(JNI_ENV_ARG(jni))) {
>>>        61     JNI_ENV_PTR(jni)->FatalError(
>>>        62         JNI_ENV_ARGS2(jni, "Failed during the GetClassSignature call"));
>>>        63   }
>>>
>>>
>>>        The JVMTI error code, returned by GetClassSignature has to be checked, not JNI ExceptionOccurred.
>>>        Also, I'd suggest to check for signature to be non-NULL.
>>>
>>>     Also, the indent in the VMEventRecursionTest.java has to be 4 (as we normally use for java code), not 2.
>>>
>>>     Thanks,
>>>     Serguei
>>>
>>>     On 8/22/18 16:20, JC Beyler wrote:
>>>>     Hi all,
>>>>
>>>>     Would anyone want to look at this change? It helps fix a minor
>>>>     bug if someone provokes a VM allocation during a VM Allocation
>>>>     Event.
>>>>
>>>>     Webrev: http://cr.openjdk.java.net/~jcbeyler/8203356/webrev.00/
>>>>     <http://cr.openjdk.java.net/%7Ejcbeyler/8203356/webrev.00/>
>>>>     Bug: https://bugs.openjdk.java.net/browse/JDK-8203356
>>>>
>>>>     Thanks!
>>>>     Jc
>>>>
>>>>     On Thu, Aug 2, 2018 at 12:46 PM JC Beyler <jcbeyler at google.com
>>>>     <mailto:jcbeyler at google.com>> wrote:
>>>>
>>>>         Hi all,
>>>>
>>>>         (Renaming the thread that did not have the RFR in front of
>>>>         the subject, I apologize)
>>>>
>>>>         Could someone review this change:
>>>>
>>>>         Webrev:
>>>>         http://cr.openjdk.java.net/~jcbeyler/8203356/webrev.00/
>>>>         <http://cr.openjdk.java.net/%7Ejcbeyler/8203356/webrev.00/>
>>>>         Bug: https://bugs.openjdk.java.net/browse/JDK-8203356
>>>>
>>>>         Basically, if during a callback from a VMObjectAlloc event,
>>>>         the user provokes a clone, the code would send a new
>>>>         callback and you can recurse infinitely.
>>>>
>>>>         I added a test that fails without the fix and passes now.
>>>>
>>>>         Thanks,
>>>>         Jc
>>>>
>>>>
>>>>
>>>>     -- 
>>>>
>>>>     Thanks,
>>>>     Jc
>>>
>>
>>
>>
>> -- 
>>
>> Thanks,
>> Jc
>