On 12/4/18 4:54 PM, David Holmes wrote:
Hi everyone,
I'd actually argue that the comment not refer just to JVMCI but more
generally:
+ // when the top frame belongs to the test rather than to
incidental Java code (classloading, JVMCI, etc)
Reasonable.
Also note typo: then -> than
Nice catch!
Thanks,
Serguei
Cheers,
David
On 5/12/2018 5:40 am, serguei.spit...@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/
<http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.02/>
Thanks,
--Daniil
*From: *"serguei.spit...@oracle.com" <serguei.spit...@oracle.com>
*Date: *Monday, December 3, 2018 at 4:14 PM
*To: *Daniil Titov <daniil.x.ti...@oracle.com>, serviceability-dev
<serviceability-dev@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 <jcbey...@google.com>
*Date: *Friday, November 30, 2018 at 7:47 PM
*To: *<daniil.x.ti...@oracle.com>
*Cc: *<serviceability-dev@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/
<http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.01/>
Thanks,
Daniil
