RFR(S) 8161225: Assert failure in JVMTI GetNamedModule at JPLISAgent.c line: 792

[Daniel D. Daugherty]

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