Discussion:
RFR 8214572: nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should not suspend the thread when the top frame executes JVMCI code
Daniil Titov
2018-12-01 03:08:53 UTC
Permalink
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
Permalink
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
Permalink
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
Permalink
Post by d***@oracle.com
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 d***@oracle.com
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
-----
Post by d***@oracle.com
dl
s***@oracle.com
2018-12-04 06:32:03 UTC
Permalink
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 David Holmes
Post by d***@oracle.com
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 d***@oracle.com
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
-----
Post by d***@oracle.com
dl
David Holmes
2018-12-04 07:32:20 UTC
Permalink
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 David Holmes
Post by d***@oracle.com
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 d***@oracle.com
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
-----
Post by d***@oracle.com
dl
s***@oracle.com
2018-12-04 07:45:31 UTC
Permalink
Agreed on both statements below.

Thanks,
Serguei
Post by David Holmes
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 David Holmes
Post by d***@oracle.com
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.