RFR 8214572: nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should not suspend the thread when the top frame executes JVMCI code

Tue Dec 4 21:15:52 UTC 2018

Hi Daniil,

Looks good to me!

Potential Nit: Any reason the comment above the loop in
http://cr.openjdk.java.net/~dtitov/8214572/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp.udiff.html
is of a different indentation?

Thanks,
Jc

On Tue, Dec 4, 2018 at 11:40 AM <serguei.spitsyn at oracle.com> wrote:

> Hi Daniil,
>
> It looks good in general.
> Thank you for the update!
>
> I have some minor comment though.
>
>
> http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h.udiff.html
>
> +/**+* This method suspends the thread while ensuring the top frame executes the test method+* rather then JVMCI code triggered by invocation counter overflow.+*/+int suspendThreadAtMethod(jvmtiEnv *jvmti, jclass cls, jobject thread, jmethodID method);
>
>
> The comment above is not precise as it tells nothing about top frame.
>
> I like this one from implementation:
>
> +        // We need to ensure that the thread is suspended at the right place+        // when the top frame belongs to the test rather then to JVMCI code.
>
>
> So, the can be rephrased to something like:
>
> +        // This method makes the thread to be suspended at the right place+        // when the top frame belongs to the test rather then to JVMCI code.
>
>
>
> No need in another webrev if you fix the comment.
>
> Thanks,
> Serguei
>
>
> On 12/4/18 10:24 AM, Daniil Titov wrote:
>
> Hi Serguei and JC,
>
>
>
> Thank you for reviewing this change. And many thanks to David and Dean for
> answering JVMCI questions.
>
>
>
> Please review a new version of the fix that moves the most of the new code
> in a helper method ( as JC suggested) and corrects error messages. I also
> excluded the changes in test/hotspot/jtreg/ProblemList-graal.txt from this
> webrev.
>
>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8214572
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8214572/webrev.02/
>
>
>
> Thanks,
>
> --Daniil
>
>
>
> *From: *"serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
> <serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>
> *Date: *Monday, December 3, 2018 at 4:14 PM
> *To: *Daniil Titov <daniil.x.titov at oracle.com> <daniil.x.titov at oracle.com>,
> serviceability-dev <serviceability-dev at openjdk.java.net>
> <serviceability-dev at openjdk.java.net>
> *Subject: *Re: RFR 8214572: nsk/jvmti/unit/ForceEarlyReturn/earlyretbase
> should not suspend the thread when the top frame executes JVMCI code
>
>
>
> Hi Daniil,
>
> It looks good in general.
>
> I have two comments though.
>
> -vmTestbase/nsk/jvmti/unit/ForceEarlyReturn/earlyretbase/TestDescription.java       8195635   generic-all
>
>   It is not a good idea to remove the test from the ProblemList before the
> 8195635 is fixed.
>
>  148     if(method == midActiveMethod) {
>
>  149         printf("<<<<<<<< SuspendThread() is successfully done\n");
>
>  150     } else {
>
>  151         printf("Warning: method \"activeMethod\" was missed\n");
>
>  152         errCode = STATUS_FAILED;
>
>  153     }
>
>  I'd suggest to tweak the error message to something like:
>    "Failed in the suspThread: was not able to suspend thread with the
> activeMethod() on top\n");
>
>
> Thanks,
> Serguei
>
> *From: *JC Beyler <jcbeyler at google.com> <jcbeyler at google.com>
> *Date: *Friday, November 30, 2018 at 7:47 PM
> *To: *<daniil.x.titov at oracle.com> <daniil.x.titov at oracle.com>
> *Cc: *<serviceability-dev at openjdk.java.net>
> <serviceability-dev at openjdk.java.net>
> *Subject: *Re: RFR 8214572: nsk/jvmti/unit/ForceEarlyReturn/earlyretbase
> should not suspend the thread when the top frame executes JVMCI code
>
>
>
> Hi Daniil,
>
>
>
> The webrev looks good but I have a few comments and questions (if you
> don't mind :-)):
>
>
>
> Comments:
>
>   - You say that normally the test will be removed from the problem list
> once the two fixes are done but in this webrev, you've already removed it
> (I can't see the other case so I can't see if it is resolved :-))
>
>   - now that we are in C++ for the tests, could we not declare the
> variables at their first use instead of doing the pedantic top of the block
> C style?
>
>   - I feel that this sort of "wait until we are not in JVMCI frames" might
> happen a lot, maybe we could move that code into a helper method (+ it
> cleans the method a bit to just concentrate on the rest) and then if needed
> we can make it public to other tests?
>
>
>
> Questions because I'm not familiar with JVMCI consequences so not really
> comments on the webrev but so that I know:
>
>   - Is it normaly that you can suspend when you are in a JVMCI frame?
> will/is there not a better way that we could detect that we are in a JVMCI
> frame? Is it always safe to suspend a JVMCI frame?
>
>
>
> Thanks!
>
> Jc
>
>
>
>
>
>
> On 11/30/18 19:08, Daniil Titov wrote:
>
> Please review the change for nsk/jvmti/unit/ForceEarlyReturn/earlyretbase test. The problem here is that before doing an early force return the test doesn't check that the test thread is suspended at a right place where the top frame executes the test method rather than JVMCI code triggered by invocation counter overflow. That results in the early return happens for a wrong method and the test fails.
>
>
>
> The fix changes the test to do resume/suspend in the loop until the target method is executed in the top frame or the loop counter exceeds the limit.
>
>
>
> There is another problem with this test that requires changes on VM side and is currently covered by JDK-8195635:" [Graal] nsk/jvmti/unit/ForceEarlyReturn/earlyretbase crashes with assertion "compilation level out of bounds"".  The test will have to be removed from test/hotspot/jtreg/ProblemList-graal.txt only after both these issues are fixed.
>
>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8214572
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8214572/webrev.01/
>
>
>
> Thanks,
>
> Daniil
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>

-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181204/033ea1fc/attachment.html>