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

[Alex Menkov]

+1

--alex

On 08/24/2018 15:00, serguei.spit...@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.spit...@oracle.com <mailto:serguei.spit...@oracle.com> <serguei.spit...@oracle.com <mailto:serguei.spit...@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.spit...@oracle.com
    <mailto:serguei.spit...@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 <jcbey...@google.com
    <mailto:jcbey...@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