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

[Patricio Chilano]

Hi David,

On 11/20/18 5:34 PM, 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.

Ok, I updated the bug showing the stack report for the example discussed 
there and I added a new one showing when "waiting to re-lock in wait()" 
will now be displayed.

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

Ok, but wouldn't removing "print_thread_state_on(st);" line before 
"_safepoint_state->print_on(st);" on method JavaThread::print_on be okay 
then? I actually moved up the print of thread state in file 
safepoint.cpp to kept printing the states together as we do now.

> 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

Done!

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

Thanks! Yes, I spent some time trying to figure out that comment.

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

Yes, sorry forgot to do that. I actually run mach5 tiers1-2 and saw no 
errors. I see this test executes as part of tier2 so it should have 
failed. I'll look into that. Thanks for filing the bug!


Waiting for the duplicate removal confirmation to make the new webrev 
(Dan also made some comments on that).


Thanks!
Patricio
>
> Thanks,
> David
>
>> Thanks,
>> Patricio