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

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

JC Beyler

2018-12-01 03:46:53 UTC

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

Post by Daniil Titov
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

d***@oracle.com

2018-12-03 19:53:36 UTC

Post by JC Beyler
Questions because I'm not familiar with JVMCI consequences so not
- Is it normaly that you can suspend when you are in a JVMCI frame?

Yes, because it's just Java code, and we allow all Java code to be
suspended, even Graal and JVMCI code.

Post by JC Beyler
will/is there not a better way that we could detect that we are in a
JVMCI frame?

We could check the threads's _adjusting_comp_level flag for this
particular case, if we decided that we don't want to be able to debug
JVMCI Java code.

Post by JC Beyler
Is it always safe to suspend a JVMCI frame?

That's a good question. If it was grabbing any locks, then suspending
it could cause problems for other threads calling into JVMCI.

Another solution would be to do adjusting_comp_level() in a separate
thread. So I think there are at least 3 possible solutions:

1) Allow JVMCI adjusting_comp_level call to be suspended/debugged
2) Don't allow it to be suspended/debugged,
a) by running in a separate thread, or
b) don't suspend when _adjusting_comp_level flag is set

We could introduce a concept of "system Java" code, which, just like
Unix kernel code that is not debuggable without a kernel debugger, would
not normally be debuggable without setting a special flag.

CCing graal-dev alias.

dl

David Holmes

2018-12-04 04:45:47 UTC

Post by JC Beyler
Questions because I'm not familiar with JVMCI consequences so not
- Is it normaly that you can suspend when you are in a JVMCI frame?

Yes, because it's just Java code, and we allow all Java code to be
suspended, even Graal and JVMCI code.

A choice which can be argued for and against. On the one hand it is nice
to be able to try to debug JVMCI code, and on the other this injects
execution of Java code into places which to date could not execute Java
code and so can "shift" debugging actions from the application/test
code, to the JVMCI code. Arguably the application/test code may need to
have been more specific about its intent (ie verifying that the debuggee
is suspended in the expected frame) and has just "been lucky" but
nevertheless the use of JVMCI may disrupt existing code using these
facilities.

Post by JC Beyler
will/is there not a better way that we could detect that we are in a
JVMCI frame?

We could check the threads's _adjusting_comp_level flag for this
particular case, if we decided that we don't want to be able to debug
JVMCI Java code.

Post by JC Beyler
Is it always safe to suspend a JVMCI frame?

That's a good question. If it was grabbing any locks, then suspending
it could cause problems for other threads calling into JVMCI.
Another solution would be to do adjusting_comp_level() in a separate
1) Allow JVMCI adjusting_comp_level call to be suspended/debugged
2) Don't allow it to be suspended/debugged,
a) by running in a separate thread, or
b) don't suspend when _adjusting_comp_level flag is set
We could introduce a concept of "system Java" code, which, just like
Unix kernel code that is not debuggable without a kernel debugger, would
not normally be debuggable without setting a special flag.

That may be something to consider in the future (albeit something that
should IMHO have been considered well in the past!) but I think it's out
of scope for this particular issue if we want to address this in 12.
There's certainly a need, again IMHO, for a broader discussion as to how
VM services written in Java should interact with other platform services
intended for use with application and library code. I don't know if
JVMCI/Graal explicitly hide themselves from agents and
retransformation/redefinition/ClassFileLoadHook, or even basic things
like event generation (where JVMCI may now generate many more events
than previously encountered).

Post by d***@oracle.com
CCing graal-dev alias.

As a non-subscriber my reply will get held for moderation.

Cheers,
David
-----

s***@oracle.com

2018-12-04 06:32:03 UTC

Hi David and Dean,

One option is to add a command line option (disabled by default)
to enable debugging/profiling of the Graal compiler.
This will help to avoid all these Graal related issues,
simplify the development and stabilize the tests.
Not sure the Graal developer will like this proposal though. :)
Also, it is not very clear what level of complexity we add with this.
For instance, we will have to identify all spots where new checks have
to be added.

Thanks,
Serguei

Post by JC Beyler
Questions because I'm not familiar with JVMCI consequences so not
- Is it normaly that you can suspend when you are in a JVMCI frame?

Yes, because it's just Java code, and we allow all Java code to be
suspended, even Graal and JVMCI code.

A choice which can be argued for and against. On the one hand it is
nice to be able to try to debug JVMCI code, and on the other this
injects execution of Java code into places which to date could not
execute Java code and so can "shift" debugging actions from the
application/test code, to the JVMCI code. Arguably the
application/test code may need to have been more specific about its
intent (ie verifying that the debuggee is suspended in the expected
frame) and has just "been lucky" but nevertheless the use of JVMCI may
disrupt existing code using these facilities.

Post by JC Beyler
will/is there not a better way that we could detect that we are in a
JVMCI frame?

We could check the threads's _adjusting_comp_level flag for this
particular case, if we decided that we don't want to be able to debug
JVMCI Java code.

Post by JC Beyler
Is it always safe to suspend a JVMCI frame?

That's a good question. If it was grabbing any locks, then
suspending it could cause problems for other threads calling into JVMCI.
Another solution would be to do adjusting_comp_level() in a separate
1) Allow JVMCI adjusting_comp_level call to be suspended/debugged
2) Don't allow it to be suspended/debugged,
a) by running in a separate thread, or
b) don't suspend when _adjusting_comp_level flag is set
We could introduce a concept of "system Java" code, which, just like
Unix kernel code that is not debuggable without a kernel debugger,
would not normally be debuggable without setting a special flag.

That may be something to consider in the future (albeit something that
should IMHO have been considered well in the past!) but I think it's
out of scope for this particular issue if we want to address this in
12. There's certainly a need, again IMHO, for a broader discussion as
to how VM services written in Java should interact with other platform
services intended for use with application and library code. I don't
know if JVMCI/Graal explicitly hide themselves from agents and
retransformation/redefinition/ClassFileLoadHook, or even basic things
like event generation (where JVMCI may now generate many more events
than previously encountered).

Post by d***@oracle.com
CCing graal-dev alias.

As a non-subscriber my reply will get held for moderation.
Cheers,
David
-----

David Holmes

2018-12-04 07:32:20 UTC

Post by s***@oracle.com
Hi David and Dean,
One option is to add a command line option (disabled by default)
to enable debugging/profiling of the Graal compiler.
This will help to avoid all these Graal related issues,
simplify the development and stabilize the tests.

It would simply tests, which could just disable it. But things would
still need to work correctly if enabled - we can't have the current
problems of trying to do an early return, or popframe, from the wrong frame.

Post by s***@oracle.com
Not sure the Graal developer will like this proposal though. :)
Also, it is not very clear what level of complexity we add with this.
For instance, we will have to identify all spots where new checks have
to be added.

Yes identifying exactly where you needed to check for this is
non-trivial I think.

And this all needs a broader discussion before choosing this kind of
approach.

Cheers,
David

Post by s***@oracle.com
Thanks,
Serguei

Post by JC Beyler
Questions because I'm not familiar with JVMCI consequences so not
- Is it normaly that you can suspend when you are in a JVMCI frame?

Yes, because it's just Java code, and we allow all Java code to be
suspended, even Graal and JVMCI code.

A choice which can be argued for and against. On the one hand it is
nice to be able to try to debug JVMCI code, and on the other this
injects execution of Java code into places which to date could not
execute Java code and so can "shift" debugging actions from the
application/test code, to the JVMCI code. Arguably the
application/test code may need to have been more specific about its
intent (ie verifying that the debuggee is suspended in the expected
frame) and has just "been lucky" but nevertheless the use of JVMCI may
disrupt existing code using these facilities.

Post by JC Beyler
will/is there not a better way that we could detect that we are in a
JVMCI frame?

We could check the threads's _adjusting_comp_level flag for this
particular case, if we decided that we don't want to be able to debug
JVMCI Java code.

Post by JC Beyler
Is it always safe to suspend a JVMCI frame?

That's a good question. If it was grabbing any locks, then
suspending it could cause problems for other threads calling into JVMCI.
Another solution would be to do adjusting_comp_level() in a separate
1) Allow JVMCI adjusting_comp_level call to be suspended/debugged
2) Don't allow it to be suspended/debugged,
a) by running in a separate thread, or
b) don't suspend when _adjusting_comp_level flag is set
We could introduce a concept of "system Java" code, which, just like
Unix kernel code that is not debuggable without a kernel debugger,
would not normally be debuggable without setting a special flag.

That may be something to consider in the future (albeit something that
should IMHO have been considered well in the past!) but I think it's
out of scope for this particular issue if we want to address this in
12. There's certainly a need, again IMHO, for a broader discussion as
to how VM services written in Java should interact with other platform
services intended for use with application and library code. I don't
know if JVMCI/Graal explicitly hide themselves from agents and
retransformation/redefinition/ClassFileLoadHook, or even basic things
like event generation (where JVMCI may now generate many more events
than previously encountered).

Post by d***@oracle.com
CCing graal-dev alias.

As a non-subscriber my reply will get held for moderation.
Cheers,
David
-----

s***@oracle.com

2018-12-04 07:45:31 UTC

Agreed on both statements below.

Thanks,
Serguei

Yes identifying exactly where you needed to check for this is
non-trivial I think.
And this all needs a broader discussion before choosing this kind of
approach.
Cheers,
David

Post by s***@oracle.com
Thanks,
Serguei

Post by JC Beyler
Questions because I'm not familiar with JVMCI consequences so not
- Is it normaly that you can suspend when you are in a JVMCI frame?

Yes, because it's just Java code, and we allow all Java code to be
suspended, even Graal and JVMCI code.

A choice which can be argued for and against. On the one hand it is
nice to be able to try to debug JVMCI code, and on the other this
injects execution of Java code into places which to date could not
execute Java code and so can "shift" debugging actions from the
application/test code, to the JVMCI code. Arguably the
application/test code may need to have been more specific about its
intent (ie verifying that the debuggee is suspended in the expected
frame) and has just "been lucky" but nevertheless the use of JVMCI
may disrupt existing code using these facilities.

Post by JC Beyler
will/is there not a better way that we could detect that we are in
a JVMCI frame?

We could check the threads's _adjusting_comp_level flag for this
particular case, if we decided that we don't want to be able to
debug JVMCI Java code.

Post by JC Beyler
Is it always safe to suspend a JVMCI frame?

That's a good question. If it was grabbing any locks, then
suspending it could cause problems for other threads calling into JVMCI.
Another solution would be to do adjusting_comp_level() in a
1) Allow JVMCI adjusting_comp_level call to be suspended/debugged
2) Don't allow it to be suspended/debugged,
a) by running in a separate thread, or
b) don't suspend when _adjusting_comp_level flag is set
We could introduce a concept of "system Java" code, which, just
like Unix kernel code that is not debuggable without a kernel
debugger, would not normally be debuggable without setting a
special flag.

That may be something to consider in the future (albeit something
that should IMHO have been considered well in the past!) but I think
it's out of scope for this particular issue if we want to address
this in 12. There's certainly a need, again IMHO, for a broader
discussion as to how VM services written in Java should interact
with other platform services intended for use with application and
library code. I don't know if JVMCI/Graal explicitly hide themselves
from agents and retransformation/redefinition/ClassFileLoadHook, or
even basic things like event generation (where JVMCI may now
generate many more events than previously encountered).

Post by d***@oracle.com
CCing graal-dev alias.

As a non-subscriber my reply will get held for moderation.
Cheers,
David
-----

s***@oracle.com

2018-12-04 00:14:29 UTC

<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Daniil, 
 
It looks good in general. 
 
I have two comments though. 
 
<pre>-vmTestbase/nsk/jvmti/unit/ForceEarlyReturn/earlyretbase/TestDescription.java 8195635 generic-all</pre>
It is not a good idea to remove the test from the ProblemList
before the 8195635 is fixed. 
 
<pre> 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 }</pre>
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 
 
 
 
On 11/30/18 19:08, Daniil Titov wrote: 
</div>
<blockquote type="cite"
cite="mid:4E0EFD3E-F725-4469-BA20-***@oracle.com">
<pre wrap="">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: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8214572">https://bugs.openjdk.java.net/browse/JDK-8214572</a>
Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dtitov/8214572/webrev.01/">http://cr.openjdk.java.net/~dtitov/8214572/webrev.01/</a>

Thanks,
Daniil

</pre>
</blockquote>
 
</body>
</html>

Daniil Titov

2018-12-04 18:24:17 UTC

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: "***@oracle.com" <***@oracle.com>
Date: Monday, December 3, 2018 at 4:14 PM
To: Daniil Titov <***@oracle.com>, serviceability-dev <serviceability-***@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 <***@google.com>
Date: Friday, November 30, 2018 at 7:47 PM
To: <***@oracle.com>
Cc: <serviceability-***@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

s***@oracle.com

2018-12-04 19:40:37 UTC

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

Post by Daniil Titov
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
*Date: *Monday, December 3, 2018 at 4:14 PM
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Â Â Â Â }
Â Â "Failed in the suspThread: was not able to suspend thread with the
activeMethod() on top\n");
Thanks,
Serguei
*Date: *Friday, November 30, 2018 at 7:47 PM
nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should not suspend the
thread when the top frame executes JVMCI code
Hi Daniil,
Â - 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
Â - 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
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

JC Beyler

2018-12-04 21:15:52 UTC

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

Post by JC Beyler
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.
+ // 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.
+ // 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
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
*Date: *Monday, December 3, 2018 at 4:14 PM
*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 }
"Failed in the suspThread: was not able to suspend thread with the
activeMethod() on top\n");
Thanks,
Serguei
*Date: *Friday, November 30, 2018 at 7:47 PM
*Subject: *Re: RFR 8214572: nsk/jvmti/unit/ForceEarlyReturn/earlyretbase
should not suspend the thread when the top frame executes JVMCI code
Hi Daniil,
- 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
- 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
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

Daniil Titov

2018-12-04 21:46:08 UTC

Thank you, JC and Serguei,

I will correct both comments before pushing the change.

Best regards,

Daniil

From: JC Beyler <***@google.com>
Date: Tuesday, December 4, 2018 at 1:16 PM
To: <***@oracle.com>
Cc: <***@oracle.com>, <serviceability-***@openjdk.java.net>, David Holmes <***@oracle.com>, <***@oracle.com>
Subject: Re: RFR 8214572: nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should not suspend the thread when the top frame executes JVMCI code

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 <***@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: "***@oracle.com" <***@oracle.com>
Date: Monday, December 3, 2018 at 4:14 PM
To: Daniil Titov <***@oracle.com>, serviceability-dev <serviceability-***@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 <***@google.com>
Date: Friday, November 30, 2018 at 7:47 PM
To: <***@oracle.com>
Cc: <serviceability-***@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

David Holmes

2018-12-05 00:54:00 UTC

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)

Also note typo: then -> than

Cheers,
David

Post by Daniil Titov
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
*Date: *Monday, December 3, 2018 at 4:14 PM
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 }
 "Failed in the suspThread: was not able to suspend thread with the
activeMethod() on top\n");
Thanks,
Serguei
*Date: *Friday, November 30, 2018 at 7:47 PM
nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should not suspend the
thread when the top frame executes JVMCI code
Hi Daniil,
- 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
- 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
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

s***@oracle.com

2018-12-05 00:55:55 UTC

Post by David Holmes
Hi everyone,
I'd actually argue that the comment not refer just to JVMCI but more
+ // when the top frame belongs to the test rather than to
incidental Java code (classloading, JVMCI, etc)

Reasonable.

Post by David Holmes
Also note typo: then -> than

Nice catch!

Thanks,
Serguei

Post by David Holmes
Cheers,
David

Post by Daniil Titov
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
*Date: *Monday, December 3, 2018 at 4:14 PM
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 }
 "Failed in the suspThread: was not able to suspend thread with
the activeMethod() on top\n");
Thanks,
Serguei
*Date: *Friday, November 30, 2018 at 7:47 PM
nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should not suspend the
thread when the top frame executes JVMCI code
Hi Daniil,
- 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
- 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
 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

Daniil Titov

2018-12-05 01:15:00 UTC

Thank you, David and Serguei,

Please review an updated version of the patch with the corrected comment.

Bug: https://bugs.openjdk.java.net/browse/JDK-8214572
Webrev: http://cr.openjdk.java.net/~dtitov/8214572/webrev.03

Best regards,
Daniil

Reasonable.

Post by David Holmes
Also note typo: then -> than

Nice catch!

Thanks,
Serguei

Post by David Holmes
Cheers,
David

Post by Daniil Titov
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
*Date: *Monday, December 3, 2018 at 4:14 PM
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 }
"Failed in the suspThread: was not able to suspend thread with
the activeMethod() on top\n");
Thanks,
Serguei
*Date: *Friday, November 30, 2018 at 7:47 PM
nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should not suspend the
thread when the top frame executes JVMCI code
Hi Daniil,
- 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
- 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
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

David Holmes

2018-12-05 01:29:42 UTC

Thanks Daniil, that seems fine to me.

I have some reservations about whether looping up to 100ms will
necessarily be enough for compilation to complete, but we can adjust in
the future as needed.

Cheers,
David

Post by Daniil Titov
Thank you, David and Serguei,
Please review an updated version of the patch with the corrected comment.
Bug: https://bugs.openjdk.java.net/browse/JDK-8214572
Webrev: http://cr.openjdk.java.net/~dtitov/8214572/webrev.03
Best regards,
Daniil

Reasonable.

Post by David Holmes
Also note typo: then -> than

Nice catch!
Thanks,
Serguei

Post by David Holmes
Cheers,
David

Post by Daniil Titov
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
*Date: *Monday, December 3, 2018 at 4:14 PM
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 }
"Failed in the suspThread: was not able to suspend thread with
the activeMethod() on top\n");
Thanks,
Serguei
*Date: *Friday, November 30, 2018 at 7:47 PM
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 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
- 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
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

s***@oracle.com

2018-12-05 03:49:16 UTC

<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix"><a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dtitov/8214572/webrev.03/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp.udiff.html">http://cr.openjdk.java.net/~dtitov/8214572/webrev.03/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp.udiff.html</a> 
 
<pre>+ // frame belongs to the test rather then to incidental Java code (classloading,</pre>
 
The same typo but in another comment: then => than 
 
No need in another webrev. 
 
Thanks, 
Serguei 
 
 
On 12/4/18 17:15, Daniil Titov wrote: 
</div>
<blockquote type="cite"
cite="mid:9F03405A-1D91-49F1-9C03-***@oracle.com">
<pre wrap="">Thank you, David and Serguei,

Please review an updated version of the patch with the corrected comment.

Bug: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8214572">https://bugs.openjdk.java.net/browse/JDK-8214572</a>
Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dtitov/8214572/webrev.03">http://cr.openjdk.java.net/~dtitov/8214572/webrev.03</a>

Best regards,
Daniil

On 12/4/18, 4:55 PM, <a class="moz-txt-link-rfc2396E" href="mailto:***@oracle.com">"***@oracle.com"</a> <a class="moz-txt-link-rfc2396E" href="mailto:***@oracle.com"><***@oracle.com></a> wrote:

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, <a class="moz-txt-link-abbreviated" href="mailto:***@oracle.com">***@oracle.com</a> wrote:
>> Hi Daniil,
>>
>> It looks good in general.
>> Thank you for the update!
>>
>> I have some minor comment though.
>>
>> <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h.udiff.html">http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h.udiff.html</a>
>>
>>
>> +/**
>> +* 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: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8214572">https://bugs.openjdk.java.net/browse/JDK-8214572</a>
>>>
>>> Webrev:<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dtitov/8214572/webrev.02/">http://cr.openjdk.java.net/~dtitov/8214572/webrev.02/</a>
>>> <a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.02/"><http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.02/></a>
>>>
>>> Thanks,
>>>
>>> --Daniil
>>>
>>> *From: *<a class="moz-txt-link-rfc2396E" href="mailto:***@oracle.com">"***@oracle.com"</a> <a class="moz-txt-link-rfc2396E" href="mailto:***@oracle.com"><***@oracle.com></a>
>>> *Date: *Monday, December 3, 2018 at 4:14 PM
>>> *To: *Daniil Titov <a class="moz-txt-link-rfc2396E" href="mailto:***@oracle.com"><***@oracle.com></a>, serviceability-dev
>>> <a class="moz-txt-link-rfc2396E" href="mailto:serviceability-***@openjdk.java.net"><serviceability-***@openjdk.java.net></a>
>>> *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 <a class="moz-txt-link-rfc2396E" href="mailto:***@google.com"><***@google.com></a>
>>> *Date: *Friday, November 30, 2018 at 7:47 PM
>>> *To: *<a class="moz-txt-link-rfc2396E" href="mailto:***@oracle.com"><***@oracle.com></a>
>>> *Cc: *<a class="moz-txt-link-rfc2396E" href="mailto:serviceability-***@openjdk.java.net"><serviceability-***@openjdk.java.net></a>
>>> *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:<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8214572">https://bugs.openjdk.java.net/browse/JDK-8214572</a>
>>>
>>> Webrev:<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dtitov/8214572/webrev.01/">http://cr.openjdk.java.net/~dtitov/8214572/webrev.01/</a>
>>> <a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.01/"><http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.01/></a>
>>> Thanks,
>>>
>>> Daniil
>>>
>>>
>>>
>>

</pre>
</blockquote>
 
</body>
</html>

Daniil Titov

2018-12-05 05:31:52 UTC