Re: RFR(S) 8161225: Assert failure in JVMTI GetNamedModule at JPLISAgent.c line: 792
Chris, Looks good for me. But please, wait for Serguei. -Dmitry On 2016-09-21 08:07, Chris Plummer wrote: > Hello, > > Please help review the following: > > https://bugs.openjdk.java.net/browse/JDK-8161225 > http://cr.openjdk.java.net/~cjplummer/8161225/webrev.00/ > > The main fix is in JPLISAgent.c, which is to no longer call > jplis_assert_msg() when there is a PHASE error, and also remove the test > from ProblemList.txt. > > I also fixed a problem with the test. It was not checking if the > subprocess had failed to terminate properly. The result was if the > sub-process crashed, then the test would pass. I noticed this when I > temporarily forced an assert when there was a PHASE error, and suddenly > the test was always passing, yet there was an hs_err.log file. > > Lastly, I made a slight improvement to the trace output when there is a > PHASE error, so now the PHASE number is included in the trace output. So > the trace output now looks like the following when the test triggers the > PHASE error (this is without the fix being made to the test): > > [0.376s][trace][jvmti] [-] GetNamedModule JVMTI_ERROR_WRONG_PHASE(8) > > Tested by running the test 5 times on each supported platform, and also > ran nsk.jvmti and jck/vm/jvmti. > > thanks, > > Chris -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR(S) 8161225: Assert failure in JVMTI GetNamedModule at JPLISAgent.c line: 792
On 9/21/16 9:33 AM, Chris Plummer wrote:
On 9/21/16 7:33 AM, Daniel D. Daugherty wrote:
On 9/20/16 11:07 PM, Chris Plummer wrote:
Hello,
Please help review the following:
https://bugs.openjdk.java.net/browse/JDK-8161225
http://cr.openjdk.java.net/~cjplummer/8161225/webrev.00/
hotspot/src/share/vm/prims/jvmtiEnter.xsl
L490: log_trace(jvmti)("[-] %s %s(%d)", func_name,
nit: extra space after the comma
nit: extra space at the end of the line
Those were both pre-existing, but I'll clean them up.
jdk/src/java.instrument/share/native/libinstrument/JPLISAgent.c
No comments.
jdk/test/ProblemList.txt
No comments.
jdk/test/java/lang/instrument/DaemonThread/TestDaemonThreadLauncher.java
I presume when you change the assertTrue() call to an
OutputAnalyzer.should...() call that the new import can
be deleted.
Correct. I have removed it and it still compiles.
One thing that wasn't clear from your write up below... when you detect
a PHASE error, you now return a NULL (but the code is clear).
Yep.
Actually it always returned NULL when there was a PHASE error because
moduleObject would still be NULL. The only difference now is you don't
get the jplis_assert_msg() call, which is really just a printf.
Chris
Thumbs up. Don't need to see a new webrev for fixing any nits.
Thanks for the review.
Chris
Dan
The main fix is in JPLISAgent.c, which is to no longer call
jplis_assert_msg() when there is a PHASE error, and also remove the
test from ProblemList.txt.
I also fixed a problem with the test. It was not checking if the
subprocess had failed to terminate properly. The result was if the
sub-process crashed, then the test would pass. I noticed this when I
temporarily forced an assert when there was a PHASE error, and
suddenly the test was always passing, yet there was an hs_err.log file.
Lastly, I made a slight improvement to the trace output when there
is a PHASE error, so now the PHASE number is included in the trace
output. So the trace output now looks like the following when the
test triggers the PHASE error (this is without the fix being made to
the test):
[0.376s][trace][jvmti] [-] GetNamedModule
JVMTI_ERROR_WRONG_PHASE(8)
Tested by running the test 5 times on each supported platform, and
also ran nsk.jvmti and jck/vm/jvmti.
thanks,
Chris
Re: RFR(S) 8161225: Assert failure in JVMTI GetNamedModule at JPLISAgent.c line: 792
On 9/21/16 7:33 AM, Daniel D. Daugherty wrote:
On 9/20/16 11:07 PM, Chris Plummer wrote:
Hello,
Please help review the following:
https://bugs.openjdk.java.net/browse/JDK-8161225
http://cr.openjdk.java.net/~cjplummer/8161225/webrev.00/
hotspot/src/share/vm/prims/jvmtiEnter.xsl
L490: log_trace(jvmti)("[-] %s %s(%d)", func_name,
nit: extra space after the comma
nit: extra space at the end of the line
Those were both pre-existing, but I'll clean them up.
jdk/src/java.instrument/share/native/libinstrument/JPLISAgent.c
No comments.
jdk/test/ProblemList.txt
No comments.
jdk/test/java/lang/instrument/DaemonThread/TestDaemonThreadLauncher.java
I presume when you change the assertTrue() call to an
OutputAnalyzer.should...() call that the new import can
be deleted.
Correct. I have removed it and it still compiles.
One thing that wasn't clear from your write up below... when you detect
a PHASE error, you now return a NULL (but the code is clear).
Yep.
Thumbs up. Don't need to see a new webrev for fixing any nits.
Thanks for the review.
Chris
Dan
The main fix is in JPLISAgent.c, which is to no longer call
jplis_assert_msg() when there is a PHASE error, and also remove the
test from ProblemList.txt.
I also fixed a problem with the test. It was not checking if the
subprocess had failed to terminate properly. The result was if the
sub-process crashed, then the test would pass. I noticed this when I
temporarily forced an assert when there was a PHASE error, and
suddenly the test was always passing, yet there was an hs_err.log file.
Lastly, I made a slight improvement to the trace output when there is
a PHASE error, so now the PHASE number is included in the trace
output. So the trace output now looks like the following when the
test triggers the PHASE error (this is without the fix being made to
the test):
[0.376s][trace][jvmti] [-] GetNamedModule JVMTI_ERROR_WRONG_PHASE(8)
Tested by running the test 5 times on each supported platform, and
also ran nsk.jvmti and jck/vm/jvmti.
thanks,
Chris
Re: RFR(S) 8161225: Assert failure in JVMTI GetNamedModule at JPLISAgent.c line: 792
Hi Serguei, Thanks for the review. I'll fix the missing space and run the java/lang/instrument tests. thanks, Chris On 9/21/16 1:25 AM, serguei.spit...@oracle.com wrote: Chris, Forgot to tell about the testing. You also need to run all the JTREG java/lang/instrument tests. Thanks, Serguei On 9/21/16 01:21, serguei.spit...@oracle.com wrote: Hi Chris, It looks good. One minor comment: http://cr.openjdk.java.net/~cjplummer/8161225/webrev.00/webrev.hotspot/src/share/vm/prims/jvmtiEnter.xsl.frames.html 491 JvmtiUtil::error_name(JVMTI_ERROR_WRONG_PHASE),JvmtiEnv::get_phase()); A space is missed after the comma. Thanks, Serguei On 9/20/16 22:07, Chris Plummer wrote: Hello, Please help review the following: https://bugs.openjdk.java.net/browse/JDK-8161225 http://cr.openjdk.java.net/~cjplummer/8161225/webrev.00/ The main fix is in JPLISAgent.c, which is to no longer call jplis_assert_msg() when there is a PHASE error, and also remove the test from ProblemList.txt. I also fixed a problem with the test. It was not checking if the subprocess had failed to terminate properly. The result was if the sub-process crashed, then the test would pass. I noticed this when I temporarily forced an assert when there was a PHASE error, and suddenly the test was always passing, yet there was an hs_err.log file. Lastly, I made a slight improvement to the trace output when there is a PHASE error, so now the PHASE number is included in the trace output. So the trace output now looks like the following when the test triggers the PHASE error (this is without the fix being made to the test): [0.376s][trace][jvmti] [-] GetNamedModule JVMTI_ERROR_WRONG_PHASE(8) Tested by running the test 5 times on each supported platform, and also ran nsk.jvmti and jck/vm/jvmti. thanks, Chris
Re: RFR(S) 8161225: Assert failure in JVMTI GetNamedModule at JPLISAgent.c line: 792
On 9/20/16 11:07 PM, Chris Plummer wrote:
Hello,
Please help review the following:
https://bugs.openjdk.java.net/browse/JDK-8161225
http://cr.openjdk.java.net/~cjplummer/8161225/webrev.00/
hotspot/src/share/vm/prims/jvmtiEnter.xsl
L490: log_trace(jvmti)("[-] %s %s(%d)", func_name,
nit: extra space after the comma
nit: extra space at the end of the line
jdk/src/java.instrument/share/native/libinstrument/JPLISAgent.c
No comments.
jdk/test/ProblemList.txt
No comments.
jdk/test/java/lang/instrument/DaemonThread/TestDaemonThreadLauncher.java
I presume when you change the assertTrue() call to an
OutputAnalyzer.should...() call that the new import can
be deleted.
One thing that wasn't clear from your write up below... when you detect
a PHASE error, you now return a NULL (but the code is clear).
Thumbs up. Don't need to see a new webrev for fixing any nits.
Dan
The main fix is in JPLISAgent.c, which is to no longer call
jplis_assert_msg() when there is a PHASE error, and also remove the
test from ProblemList.txt.
I also fixed a problem with the test. It was not checking if the
subprocess had failed to terminate properly. The result was if the
sub-process crashed, then the test would pass. I noticed this when I
temporarily forced an assert when there was a PHASE error, and
suddenly the test was always passing, yet there was an hs_err.log file.
Lastly, I made a slight improvement to the trace output when there is
a PHASE error, so now the PHASE number is included in the trace
output. So the trace output now looks like the following when the test
triggers the PHASE error (this is without the fix being made to the
test):
[0.376s][trace][jvmti] [-] GetNamedModule JVMTI_ERROR_WRONG_PHASE(8)
Tested by running the test 5 times on each supported platform, and
also ran nsk.jvmti and jck/vm/jvmti.
thanks,
Chris
Re: RFR(S) 8161225: Assert failure in JVMTI GetNamedModule at JPLISAgent.c line: 792
Chris, Forgot to tell about the testing. You also need to run all the JTREG java/lang/instrument tests. Thanks, Serguei On 9/21/16 01:21, serguei.spit...@oracle.com wrote: Hi Chris, It looks good. One minor comment: http://cr.openjdk.java.net/~cjplummer/8161225/webrev.00/webrev.hotspot/src/share/vm/prims/jvmtiEnter.xsl.frames.html 491 JvmtiUtil::error_name(JVMTI_ERROR_WRONG_PHASE),JvmtiEnv::get_phase()); A space is missed after the comma. Thanks, Serguei On 9/20/16 22:07, Chris Plummer wrote: Hello, Please help review the following: https://bugs.openjdk.java.net/browse/JDK-8161225 http://cr.openjdk.java.net/~cjplummer/8161225/webrev.00/ The main fix is in JPLISAgent.c, which is to no longer call jplis_assert_msg() when there is a PHASE error, and also remove the test from ProblemList.txt. I also fixed a problem with the test. It was not checking if the subprocess had failed to terminate properly. The result was if the sub-process crashed, then the test would pass. I noticed this when I temporarily forced an assert when there was a PHASE error, and suddenly the test was always passing, yet there was an hs_err.log file. Lastly, I made a slight improvement to the trace output when there is a PHASE error, so now the PHASE number is included in the trace output. So the trace output now looks like the following when the test triggers the PHASE error (this is without the fix being made to the test): [0.376s][trace][jvmti] [-] GetNamedModule JVMTI_ERROR_WRONG_PHASE(8) Tested by running the test 5 times on each supported platform, and also ran nsk.jvmti and jck/vm/jvmti. thanks, Chris
Re: RFR(S) 8161225: Assert failure in JVMTI GetNamedModule at JPLISAgent.c line: 792
Hi Chris, It looks good. One minor comment: http://cr.openjdk.java.net/~cjplummer/8161225/webrev.00/webrev.hotspot/src/share/vm/prims/jvmtiEnter.xsl.frames.html 491 JvmtiUtil::error_name(JVMTI_ERROR_WRONG_PHASE),JvmtiEnv::get_phase()); A space is missed after the comma. Thanks, Serguei On 9/20/16 22:07, Chris Plummer wrote: Hello, Please help review the following: https://bugs.openjdk.java.net/browse/JDK-8161225 http://cr.openjdk.java.net/~cjplummer/8161225/webrev.00/ The main fix is in JPLISAgent.c, which is to no longer call jplis_assert_msg() when there is a PHASE error, and also remove the test from ProblemList.txt. I also fixed a problem with the test. It was not checking if the subprocess had failed to terminate properly. The result was if the sub-process crashed, then the test would pass. I noticed this when I temporarily forced an assert when there was a PHASE error, and suddenly the test was always passing, yet there was an hs_err.log file. Lastly, I made a slight improvement to the trace output when there is a PHASE error, so now the PHASE number is included in the trace output. So the trace output now looks like the following when the test triggers the PHASE error (this is without the fix being made to the test): [0.376s][trace][jvmti] [-] GetNamedModule JVMTI_ERROR_WRONG_PHASE(8) Tested by running the test 5 times on each supported platform, and also ran nsk.jvmti and jck/vm/jvmti. thanks, Chris
Re: RFR(S) 8161225: Assert failure in JVMTI GetNamedModule at JPLISAgent.c line: 792
On 9/20/16 10:48 PM, David Holmes wrote: Hi Chris, On 21/09/2016 3:07 PM, Chris Plummer wrote: Hello, Please help review the following: https://bugs.openjdk.java.net/browse/JDK-8161225 http://cr.openjdk.java.net/~cjplummer/8161225/webrev.00/ Looks fine. Only comment: test/java/lang/instrument/DaemonThread/TestDaemonThreadLauncher.java Rather than: assertTrue(analyzer.getExitValue() == 0); I would expect to see: analyzer.shouldHaveExitValue(0); That works. I'll change it. Thanks! Chris Unless of course the JDK test library OutputAnalyzer doesn't have that. Thanks, David The main fix is in JPLISAgent.c, which is to no longer call jplis_assert_msg() when there is a PHASE error, and also remove the test from ProblemList.txt. I also fixed a problem with the test. It was not checking if the subprocess had failed to terminate properly. The result was if the sub-process crashed, then the test would pass. I noticed this when I temporarily forced an assert when there was a PHASE error, and suddenly the test was always passing, yet there was an hs_err.log file. Lastly, I made a slight improvement to the trace output when there is a PHASE error, so now the PHASE number is included in the trace output. So the trace output now looks like the following when the test triggers the PHASE error (this is without the fix being made to the test): [0.376s][trace][jvmti] [-] GetNamedModule JVMTI_ERROR_WRONG_PHASE(8) Tested by running the test 5 times on each supported platform, and also ran nsk.jvmti and jck/vm/jvmti. thanks, Chris
Re: RFR(S) 8161225: Assert failure in JVMTI GetNamedModule at JPLISAgent.c line: 792
Hi Chris, On 21/09/2016 3:07 PM, Chris Plummer wrote: Hello, Please help review the following: https://bugs.openjdk.java.net/browse/JDK-8161225 http://cr.openjdk.java.net/~cjplummer/8161225/webrev.00/ Looks fine. Only comment: test/java/lang/instrument/DaemonThread/TestDaemonThreadLauncher.java Rather than: assertTrue(analyzer.getExitValue() == 0); I would expect to see: analyzer.shouldHaveExitValue(0); Unless of course the JDK test library OutputAnalyzer doesn't have that. Thanks, David The main fix is in JPLISAgent.c, which is to no longer call jplis_assert_msg() when there is a PHASE error, and also remove the test from ProblemList.txt. I also fixed a problem with the test. It was not checking if the subprocess had failed to terminate properly. The result was if the sub-process crashed, then the test would pass. I noticed this when I temporarily forced an assert when there was a PHASE error, and suddenly the test was always passing, yet there was an hs_err.log file. Lastly, I made a slight improvement to the trace output when there is a PHASE error, so now the PHASE number is included in the trace output. So the trace output now looks like the following when the test triggers the PHASE error (this is without the fix being made to the test): [0.376s][trace][jvmti] [-] GetNamedModule JVMTI_ERROR_WRONG_PHASE(8) Tested by running the test 5 times on each supported platform, and also ran nsk.jvmti and jck/vm/jvmti. thanks, Chris
