Looks good to me.
--alex
On 10/04/2018 15:13, Daniil Titov wrote:
Hi Alex,
Just several minutes ago in another email I sent an updated patch on review
(please see email attached). The patch has reimplemented tests and no longer
uses JdbTest , so the changes you mentioned are no longer there. I also created
issue https://bugs.openjdk.java.net/browse/JDK-8211736 to address the missing
prompt problem.
Thanks!
--Daniil
On 10/4/18, 2:53 PM, "Alex Menkov" <alexey.men...@oracle.com> wrote:
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
>>>
>>>
>>>
>>
>
