RFR 8150689: Thread dump report "waiting to re-lock in wait()" shows incorrectly

[David Holmes]

Sorry this was ambiguous:

Nit: need space after "if", need space after ")"

should be:

Nit: need space after "if", need space before "{"

David

On 21/11/2018 8:34 am, David Holmes wrote:
> Hi Patricio,
> 
> On 21/11/2018 3:51 am, Patricio Chilano wrote:
>> Hi all,
>>
>> Could you review this small fix for thread dump reports?
>> Now the "waiting to re-lock in wait()" message is shown in the "at 
>> java.lang.Object.wait(Native Method)" frame where the re-locking is 
>> actually occurring. In the frame where the lock was first taken we 
>> always show "locked". Please check if there is some scenario where you 
>> think the lock info on the stack is not being printed as you would 
>> expect.
> 
> Can you please update the bug report with examples of how the stack 
> report now looks compared to those used to discuss the bug.
> 
> I applied the patch and ran my examples and they looked good!
> 
>> I also noticed the _thread_state attribute was printed twice so I 
>> removed one instance. I don't know if there was a specific reason to 
>> print it twice so I can add it back.
> 
> ThreadSafepointState::print_on is called from both the stack dump code 
> (where you see _thread_state twice) and safepoint logging. It's useful 
> to also see the thread state during safepoint logging so please undo 
> this change.
> 
> src/hotspot/share/runtime/vframe.cpp
> 
> 182 if(java_lang_Thread::get_thread_status(thread()->threadObj()) == 
> java_lang_Thread::BLOCKED_ON_MONITOR_ENTER){
> 
> Nit: need space after "if", need space after ")"
> Nit please line-break after == and indent appropriately
> 
> This seems such an obvious check I have to really question why we did 
> not do this before, and had the comment that the two states could not be 
> distinguished. :)
> 
> I think what you have done simplifies things nicely, and as I said my 
> two main examples now work fine.
> 
>> Webrev URL: http://cr.openjdk.java.net/~pchilanomate/8150689.01/webrev
>> Bug URL: https://bugs.openjdk.java.net/browse/JDK-8150689
> 
> Please always advise what testing you did. In this case I think we have 
> a problem with a test, but it seems to be a pre-existing problem:
> 
> serviceability/tmtools/jstack/WaitNotifyThreadTest.java
> 
> Seems to expect to find a "waiting to re-lock" line but in fact I can't 
> see anything in the test that actually creates a situation where the 
> thread would be waiting to re-lock! Further the test is supposed to 
> print out:
> 
> System.out.println("Correct monitor info found");
> 
> yet there is no such output. This indicates that the test is not working 
> as expected and is not actually finding any locks. I think the logic for 
> checking the method "name" is broken e.g.
> 
>       if (mi.getName().startsWith("WaitThread.run")) {
> 
> the actual name is WaitNotifyThreadTest$WaitThread.run so it won't match!
> 
> Also:
> 
> if (mi.getName().startsWith(OBJECT_WAIT) && mi.getCompilationUnit() == 
> null /*native method*/) {
> 
> won't match anything either as far as I can see!
> 
> <sigh> I filed a bug for that test: JDK-8214148
> 
> Thanks,
> David
> 
>> Thanks,
>> Patricio