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

[David Holmes]

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