RFR 8193879: Java debugger hangs on method invocation

[Alex Menkov]

On 10/04/2018 10:28, Chris Plummer wrote:
> On 10/4/18 5:12 AM, Gary Adams wrote:
>> In TTY.java do you need to force a simple prompt for the
>> breakpoint event output? What prevents currentThread from
>> being set at the time you are printing the prompt?
>>
>>
>>  103         // Print the prompt if suspend policy is 
>> SUSPEND_EVENT_THREAD. In case of
>>  104         // SUSPEND_ALL policy this is handled by vmInterrupted() 
>> method.
>>  105         if (be.request().suspendPolicy() == 
>> EventRequest.SUSPEND_EVENT_THREAD) {
>>  106             MessageOutput.println();
>>  107             MessageOutput.printPrompt();
> Normally the thread is suspended after the above code is executed:
> 
>      public void run() {
>          EventQueue queue = Env.vm().eventQueue();
>          while (connected) {
>              try {
>                  EventSet eventSet = queue.remove();
>                  boolean resumeStoppedApp = false;
>                  EventIterator it = eventSet.eventIterator();
>                  while (it.hasNext()) {
>                      resumeStoppedApp |= !handleEvent(it.nextEvent()); 
> <--- calls the modified code above
>                  }
> 
>                  if (resumeStoppedApp) {
>                      eventSet.resume();
>                  } else if (eventSet.suspendPolicy() == 
> EventRequest.SUSPEND_ALL) {
>                      setCurrentThread(eventSet);   <------
>                      notifier.vmInterrupted();
>                  }
> 
> However, it only calls setCurrentThread() for SUSPEND_ALL policies. So 
> what Daniil has done here is make it print a simple prompt if the policy 
> is SUSPEND_EVENT_THREAD. It's unclear to me what the actual debugger 
> behavior is in this case. Don't we still suspend and get a prompt from 
> which we can type in the next command? In this case, wouldn't you want a 
> full prompt? Related to that question, why is vmInterrupted() only 
> called if we suspend all threads, and not when we just suspend the 
> thread that the breakpoint came in on?

Did a quick test
- run debuggee (suspended), connect jdb:
===========
VM Started: No frames on the current call stack

main[1] stop thread at myclass:71
Deferring breakpoint myclass:71.
It will be set after the class is loaded.
main[1] run
 > Set deferred breakpoint myclass:71

Breakpoint hit:
=====
I.e. we get only "Breakpoint hit: " without any info about the thread 
(and this doesn't look good), current thread is not set ("next" reports 
"Nothing suspended.")

But if we handle SUSPEND_EVENT_THREAD like SUSPEND_ALL, we need to think 
how to handle possible race conditions when there are 2 threads, each of 
them is stopped by "stop thread at/in".

Some notes about the fix (actually about the tests as the fix itself 
should be implemented):
JdbStopThreadWithTraceOnTest.java
70         jdb.command("stop thread at " + DEBUGGEE_CLASS + ":" + bpLine);

It would be better to introduce
public static JdbCommand JdbCommand.stopThreadAt(String targetClass, int 
lineNum)
Actually may be it would be better to drop Jdb.command(String) - it's 
required only by CommandCommentDelimiter (need to update it to use "new 
JdbCommand(...)").

If the fix will require "simple prompt" reply, I'd make it JdbCommand 
property (like JdbCommand.allowExit)

--alex

> 
> Chris
>>
>>
>> In Jdb.java you allow the waiting for the simple prompt.
>> I don't see waitForSimplePrompt used in any existing tests.
>> Since it is only looking for a single character it could
>> produce false positives if the '>' was produced in the
>> output stream. Is this wait paired to the added prompt for the
>> break point event?
>>
>>  236         return waitForSimplePrompt ? waitForSimplePrompt(1, 
>> cmd.allowExit) : waitForPrompt(1, cmd.allowExit);
>>
>> It might be a good idea to include a test with multiple
>> threads each with a breakpoint that will trigger SUSPEND_EVENT_THREAD
>> behavior.
>>
>> On 10/4/18, 12:29 AM, Daniil Titov wrote:
>>> Please review the changes that fix the deadlock in the debugger when 
>>> the debugger is running with the tracing option on.
>>>
>>> The problem here is that when the tracing is on the "JDI Target VM 
>>> Interface" thread (the thread that reads all replies and then 
>>> notifies the thread that sent the request that the reply has been
>>> received) is waiting for a lock which is already acquired by the 
>>> thread that sent the request and is waiting for reply.
>>>
>>> The fix itself is in 
>>> src/jdk.jdi/share/classes/com/sun/tools/jdi/VMState.java.
>>>
>>> The patch also reverts the changes done in 8129348 "Debugger hangs in 
>>> trace mode with TRACE_SENDS" in 
>>> src/jdk.jdi/share/classes/com/sun/tools/jdi/InvokableTypeImpl.java 
>>> since they address only a specific case (VM is suspended and static 
>>> method is invoked) while the proposed fix also solves issue 8129348 
>>> as well as issue 8193801 "Debugger hangs in trace mode for non-static 
>>> methods".
>>>
>>> The changes include new tests for issues 8193879, 8193801 and 8129348.
>>>
>>> The changes in 
>>> src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTY.java 
>>> solve the problem that the prompt is not printed in the debugger 
>>> output when the breakpoint is hit and the suspend policy is 
>>> SUSPEND_EVENT_THREAD. This is required for new tests to detect that 
>>> command "stop thread at ..." is completed.
>>>
>>> Mach5 build and jdk_jdi tests succeeded.
>>>
>>> Webrev: http://cr.openjdk.java.net/~dtitov/8193879/webrev.01/
>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8193879
>>>
>>> Thanks,
>>> --Daniil
>>>
>>>
>>>
>>
>