Re: RFR: 8229118: [TESTBUG] serviceability/sa/ClhsdbFindPC fails on AArch64
Thanks Andrew and Chris. Pushed here:
https://hg.openjdk.java.net/jdk/jdk/rev/902cef494e66
Nick
On 14/08/2019 16:10, Andrew Dinn wrote:
On 14/08/2019 03:28, Chris Plummer wrote:
On 8/13/19 6:26 PM, Nick Gasson wrote:
Hi Chris,
The changes look good, although I think the new file should go in the
serviceability/sa test directory, unless you think this is a generally
useful class that might be used by tests outside of the sa.
The new file is under test/hotspot/jtreg/serviceability/sa/ - the same
directory as ClhsdbFindPC.java - did you mean somewhere else?
Thanks,
Nick
Oh, sorry. For some reason I thought it was in the lib directory with
LingeredApp. Yes, it's good the way it is.
I'm still happy with this patch to go in after these changes.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8229118: [TESTBUG] serviceability/sa/ClhsdbFindPC fails on AArch64
On 14/08/2019 03:28, Chris Plummer wrote:
> On 8/13/19 6:26 PM, Nick Gasson wrote:
>> Hi Chris,
>>
>>> The changes look good, although I think the new file should go in the
>>> serviceability/sa test directory, unless you think this is a generally
>>> useful class that might be used by tests outside of the sa.
>>>
>>
>> The new file is under test/hotspot/jtreg/serviceability/sa/ - the same
>> directory as ClhsdbFindPC.java - did you mean somewhere else?
>>
>> Thanks,
>> Nick
>>
> Oh, sorry. For some reason I thought it was in the lib directory with
> LingeredApp. Yes, it's good the way it is.
I'm still happy with this patch to go in after these changes.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8229118: [TESTBUG] serviceability/sa/ClhsdbFindPC fails on AArch64
On 8/13/19 6:26 PM, Nick Gasson wrote: Hi Chris, The changes look good, although I think the new file should go in the serviceability/sa test directory, unless you think this is a generally useful class that might be used by tests outside of the sa. The new file is under test/hotspot/jtreg/serviceability/sa/ - the same directory as ClhsdbFindPC.java - did you mean somewhere else? Thanks, Nick Oh, sorry. For some reason I thought it was in the lib directory with LingeredApp. Yes, it's good the way it is. thanks, Chris
Re: RFR: 8229118: [TESTBUG] serviceability/sa/ClhsdbFindPC fails on AArch64
Hi Chris, The changes look good, although I think the new file should go in the serviceability/sa test directory, unless you think this is a generally useful class that might be used by tests outside of the sa. The new file is under test/hotspot/jtreg/serviceability/sa/ - the same directory as ClhsdbFindPC.java - did you mean somewhere else? Thanks, Nick
Re: RFR: 8229118: [TESTBUG] serviceability/sa/ClhsdbFindPC fails on AArch64
On 8/13/19 2:38 AM, Nick Gasson wrote: Hi Chris, Adding to Andrew comments, maybe the solution is to have the test extend LingeredApp so it can produce a more reliable stack trace other than the default one you get with LingeredApp. If that's too much trouble, I don't mind the solution you came up with, but seems writing a LingeredApp subclass that is specific for this test would be cleaner. Thanks for the suggestion, this does seem much cleaner. Please check the updated webrev here: http://cr.openjdk.java.net/~ngasson/8229118/webrev.1/ Thanks, Nick The changes look good, although I think the new file should go in the serviceability/sa test directory, unless you think this is a generally useful class that might be used by tests outside of the sa. thanks, Chris
Re: RFR: 8229118: [TESTBUG] serviceability/sa/ClhsdbFindPC fails on AArch64
Hi Chris, Adding to Andrew comments, maybe the solution is to have the test extend LingeredApp so it can produce a more reliable stack trace other than the default one you get with LingeredApp. If that's too much trouble, I don't mind the solution you came up with, but seems writing a LingeredApp subclass that is specific for this test would be cleaner. Thanks for the suggestion, this does seem much cleaner. Please check the updated webrev here: http://cr.openjdk.java.net/~ngasson/8229118/webrev.1/ Thanks, Nick
Re: RFR: 8229118: [TESTBUG] serviceability/sa/ClhsdbFindPC fails on AArch64
Hi Nick,
Adding to Andrew comments, maybe the solution is to have the test extend
LingeredApp so it can produce a more reliable stack trace other than the
default one you get with LingeredApp. If that's too much trouble, I
don't mind the solution you came up with, but seems writing a
LingeredApp subclass that is specific for this test would be cleaner.
thanks,
Chris
On 8/12/19 3:24 AM, Nick Gasson wrote:
Thanks Andrew. Can someone from the serviceability team check this is
OK to push?
Nick
On 08/08/2019 18:16, Andrew Dinn wrote:
Hi Nick,
On 08/08/2019 10:32, Nick Gasson wrote:
Bug: https://bugs.openjdk.java.net/browse/JDK-8229118
Webrev: http://cr.openjdk.java.net/~ngasson/8229118/webrev.0/
This test starts a sub-process with -Xcomp and then uses the SA to
get a
stack trace of it. It expects to see this line:
In code in NMethod for jdk/test/lib/apps/LingeredApp.main
But actually on AArch64 the stack trace looks like this:
- java.lang.Thread.sleep(long) @bci=0, pc=0x74603d08,
Method*=0x031baf98 (Compiled frame; information may be
imprecise)
- jdk.test.lib.apps.LingeredApp.main(java.lang.String[]) @bci=53,
line=502, pc=0x6c9276e0, Method*=0x03611d48
(Interpreted frame)
The main method is interpreted even though we're running with
-Xcomp. That's because it is deoptimized almost immediately, because
main calls some methods on java.nio.file.Paths, but that class hasn't
been loaded when main is compiled.
X86 can patch in the address of the method on-the-fly, but AArch64
can't
do this because of restrictions on which instructions can be legally
rewritten.
This patch lifts the code that uses the java.nio classes out of
LingeredApp::main into a separate static method. LingeredApp.main now
only uses classes that are loaded very early in boot, before main is
compiled. The stack trace now looks like:
"main" #1 prio=5 tid=0xb4022800 nid=0xd610 waiting on
condition [0xbb755000]
java.lang.Thread.State: TIMED_WAITING (sleeping)
JavaThread state: _thread_blocked
- java.lang.Thread.sleep(long) @bci=0, pc=0xace414c8,
Method*=0x3dac8a28 (Compiled frame; information may be
imprecise)
- jdk.test.lib.apps.LingeredApp.pollLockFile(java.lang.String)
@bci=30, line=499, pc=0xa50818e0, Method*=0x3c122cf0
(Interpreted frame)
- jdk.test.lib.apps.LingeredApp.main(java.lang.String[]) @bci=25,
line=529, pc=0xa59afd58, Method*=0x3c122de0
(Compiled frame)
I.e. pollLockFile was deoptimized to an interpreted frame but
LingeredApp.main is still a compiled frame which is what
ClhsdbFindPC is
looking for.
This solution does seem a bit hacky, so if it's not acceptable an
alternative is to just skip the -Xcomp part of the test on AArch64.
Ran a full jtreg test on AArch64/x86 to check for regressions.
Yuck! That's a nice hack to avoid the indeterminate effect of -Xcomp.
However, my gut feeling is still that relying on -Xcomp in tests is just
a /really/ bad idea and I'd prefer to omit it but . . .
I'm not 100% clear what the point of this test is but it looks like it
is meant to exercise the stack backtrace code when there is a compiled
method on the stack. If so then I guess your hack fits the bill while
removing the -Xcomp flag from the command line would not fulfil the
test's remit. If that is the point of the test then I agree,
reluctantly, that your hack is the right solution. On those grounds I'm
happy to accept the patch. However, I'd prefer someone else (Andrew
Haley?) also to review this before it gets pushed.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8229118: [TESTBUG] serviceability/sa/ClhsdbFindPC fails on AArch64
Thanks Andrew. Can someone from the serviceability team check this is OK
to push?
Nick
On 08/08/2019 18:16, Andrew Dinn wrote:
Hi Nick,
On 08/08/2019 10:32, Nick Gasson wrote:
Bug: https://bugs.openjdk.java.net/browse/JDK-8229118
Webrev: http://cr.openjdk.java.net/~ngasson/8229118/webrev.0/
This test starts a sub-process with -Xcomp and then uses the SA to get a
stack trace of it. It expects to see this line:
In code in NMethod for jdk/test/lib/apps/LingeredApp.main
But actually on AArch64 the stack trace looks like this:
- java.lang.Thread.sleep(long) @bci=0, pc=0x74603d08,
Method*=0x031baf98 (Compiled frame; information may be imprecise)
- jdk.test.lib.apps.LingeredApp.main(java.lang.String[]) @bci=53, line=502,
pc=0x6c9276e0, Method*=0x03611d48 (Interpreted frame)
The main method is interpreted even though we're running with
-Xcomp. That's because it is deoptimized almost immediately, because
main calls some methods on java.nio.file.Paths, but that class hasn't
been loaded when main is compiled.
X86 can patch in the address of the method on-the-fly, but AArch64 can't
do this because of restrictions on which instructions can be legally
rewritten.
This patch lifts the code that uses the java.nio classes out of
LingeredApp::main into a separate static method. LingeredApp.main now
only uses classes that are loaded very early in boot, before main is
compiled. The stack trace now looks like:
"main" #1 prio=5 tid=0xb4022800 nid=0xd610 waiting on condition
[0xbb755000]
java.lang.Thread.State: TIMED_WAITING (sleeping)
JavaThread state: _thread_blocked
- java.lang.Thread.sleep(long) @bci=0, pc=0xace414c8,
Method*=0x3dac8a28 (Compiled frame; information may be imprecise)
- jdk.test.lib.apps.LingeredApp.pollLockFile(java.lang.String) @bci=30,
line=499, pc=0xa50818e0, Method*=0x3c122cf0 (Interpreted frame)
- jdk.test.lib.apps.LingeredApp.main(java.lang.String[]) @bci=25, line=529,
pc=0xa59afd58, Method*=0x3c122de0 (Compiled frame)
I.e. pollLockFile was deoptimized to an interpreted frame but
LingeredApp.main is still a compiled frame which is what ClhsdbFindPC is
looking for.
This solution does seem a bit hacky, so if it's not acceptable an
alternative is to just skip the -Xcomp part of the test on AArch64.
Ran a full jtreg test on AArch64/x86 to check for regressions.
Yuck! That's a nice hack to avoid the indeterminate effect of -Xcomp.
However, my gut feeling is still that relying on -Xcomp in tests is just
a /really/ bad idea and I'd prefer to omit it but . . .
I'm not 100% clear what the point of this test is but it looks like it
is meant to exercise the stack backtrace code when there is a compiled
method on the stack. If so then I guess your hack fits the bill while
removing the -Xcomp flag from the command line would not fulfil the
test's remit. If that is the point of the test then I agree,
reluctantly, that your hack is the right solution. On those grounds I'm
happy to accept the patch. However, I'd prefer someone else (Andrew
Haley?) also to review this before it gets pushed.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8229118: [TESTBUG] serviceability/sa/ClhsdbFindPC fails on AArch64
Hi Nick,
On 08/08/2019 10:32, Nick Gasson wrote:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8229118
> Webrev: http://cr.openjdk.java.net/~ngasson/8229118/webrev.0/
>
> This test starts a sub-process with -Xcomp and then uses the SA to get a
> stack trace of it. It expects to see this line:
>
> In code in NMethod for jdk/test/lib/apps/LingeredApp.main
>
> But actually on AArch64 the stack trace looks like this:
>
> - java.lang.Thread.sleep(long) @bci=0, pc=0x74603d08,
> Method*=0x031baf98 (Compiled frame; information may be imprecise)
> - jdk.test.lib.apps.LingeredApp.main(java.lang.String[]) @bci=53, line=502,
> pc=0x6c9276e0, Method*=0x03611d48 (Interpreted frame)
>
> The main method is interpreted even though we're running with
> -Xcomp. That's because it is deoptimized almost immediately, because
> main calls some methods on java.nio.file.Paths, but that class hasn't
> been loaded when main is compiled.
>
> X86 can patch in the address of the method on-the-fly, but AArch64 can't
> do this because of restrictions on which instructions can be legally
> rewritten.
>
> This patch lifts the code that uses the java.nio classes out of
> LingeredApp::main into a separate static method. LingeredApp.main now
> only uses classes that are loaded very early in boot, before main is
> compiled. The stack trace now looks like:
>
> "main" #1 prio=5 tid=0xb4022800 nid=0xd610 waiting on condition
> [0xbb755000]
>java.lang.Thread.State: TIMED_WAITING (sleeping)
>JavaThread state: _thread_blocked
> - java.lang.Thread.sleep(long) @bci=0, pc=0xace414c8,
> Method*=0x3dac8a28 (Compiled frame; information may be imprecise)
> - jdk.test.lib.apps.LingeredApp.pollLockFile(java.lang.String) @bci=30,
> line=499, pc=0xa50818e0, Method*=0x3c122cf0 (Interpreted
> frame)
> - jdk.test.lib.apps.LingeredApp.main(java.lang.String[]) @bci=25, line=529,
> pc=0xa59afd58, Method*=0x3c122de0 (Compiled frame)
>
> I.e. pollLockFile was deoptimized to an interpreted frame but
> LingeredApp.main is still a compiled frame which is what ClhsdbFindPC is
> looking for.
>
> This solution does seem a bit hacky, so if it's not acceptable an
> alternative is to just skip the -Xcomp part of the test on AArch64.
>
> Ran a full jtreg test on AArch64/x86 to check for regressions.
Yuck! That's a nice hack to avoid the indeterminate effect of -Xcomp.
However, my gut feeling is still that relying on -Xcomp in tests is just
a /really/ bad idea and I'd prefer to omit it but . . .
I'm not 100% clear what the point of this test is but it looks like it
is meant to exercise the stack backtrace code when there is a compiled
method on the stack. If so then I guess your hack fits the bill while
removing the -Xcomp flag from the command line would not fulfil the
test's remit. If that is the point of the test then I agree,
reluctantly, that your hack is the right solution. On those grounds I'm
happy to accept the patch. However, I'd prefer someone else (Andrew
Haley?) also to review this before it gets pushed.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
