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

[Chris Plummer]

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
>>
>