Hi Severin,
On 10/9/18 8:50 AM, Severin Gehwolf wrote:
Hi Seguei,
On Mon, 2018-10-08 at 17:57 -0700, serguei.spit...@oracle.com wrote:
Hi Severin,
You already fixed a couple of deadlocks in the debugger method
invocation and have an expertise in this area.
Do you have any time to review the webrev from Daniil?
Sorry, not at the moment. It's also been a while since spelunking in
that area.
Otherwise, I'd like to keep you aware about this fix.
OK, thanks! I'll try to have a look at it post-push.
Okay, thanks!
Any particular reason why the bug is private? I don't seem to be able
to view it:
https://bugs.openjdk.java.net/browse/JDK-8193879
Most likely, it is because it is a shadow bug from the internal
bug tracking system reflecting external support.
These bugs are confidential by default.
this bug was from JetBrains folks.
If you want, I could send you the pieces you need separately.
Thanks,
Serguei
Thanks,
Severin
Thanks,
Serguei
On 10/8/18 17:53, serguei.spit...@oracle.com wrote:
Hi Daniil,
It seems to me, this fix is going to work.
The freeze() method only cares there are no pending resume
commands:
99 synchronized void freeze() {
100 if (cache == null &&
(pendingResumeCommands.isEmpty())) {
101 /*
102 * No pending resumes to worry about. The VM is
suspended
103 * and additional state can be cached. Notify all
104 * interested listeners.
105 */
106 processVMAction(new VMAction(vm,
VMAction.VM_SUSPENDED));
107 enableCache();
108 }
109 }
With new version of the notifyCommandComplete:
95 void notifyCommandComplete(int id) {
96 pendingResumeCommands.remove(id);
97 }
a pending resume command can be deleted from the
pendingResumeCommands set.
This does not matter if the collection is already empty.
The only other place for a potential conflict is the method:
111 synchronized PacketStream thawCommand(CommandSender
sender) {
112 PacketStream stream = sender.send();
113 pendingResumeCommands.add(stream.id());
114 thaw();
115 return stream;
116 }
However, there is no problem here as the pendingResumeCommands is a
synchronized set.
- return (BreakpointEvent)waitForRequestedEvent(request);
+ return (BreakpointEvent) waitForRequestedEvent(request);
Not sure why have you added space after the cast.
We should not have it by coding convention.
Also, the local style does not have it as well.
Examples are:
761 StepEvent retEvent =
(StepEvent)waitForRequestedEvent(sr);
835 return (Location)locs.get(0);
873 return
(ClassPrepareEvent)waitForRequestedEvent(request);
Otherwise, it looks pretty good to me.
No need in another webrev if you fix the above.
Thanks,
Serguei
On 10/4/18 14:19, Daniil Titov wrote:
Hi Gary and Chris,
Please review an updated version of the patch that has newly
added test for the case when suspend policy is set to
SUSPEND_EVENT_THREAD reimplemented using JDI API. Thus, the
changes in
src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTY.jav
a are no longer required.
I think vmInterrupted() is not invoked when suspend policy is set
to SUSPEND_EVENT_THREAD to address the case when different
threads keep hitting the same breakpoint and to avoid the
switching the current thread in the background.
The actual behavior of the debugger in the case when the
breakpoint is hit and suspend policy is set to
SUSPEND_EVENT_THREAD is just to print "Breakpoint hit:" in the
output without adding any additional information or new line
character. After that you need to set the current thread by
entering "thread" command , e.g. "thread 1" and only then jdb
prints the prompt (e.g. "main[1]") . The behavior looks as
confusing since it is not obvious for the user that some input is
expected from him (e.g. to set the current thread). I created a
separated issue for that at
https://bugs.openjdk.java.net/browse/JDK-8211736 .
Webrev: http://cr.openjdk.java.net/~dtitov/8193879/webrev.02/
Issue: https://bugs.openjdk.java.net/browse/JDK-8193879
Thanks,
--Daniil
On 10/4/18, 10:28 AM, "Chris Plummer" <chris.plum...@oracle.com>
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?
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.jav
a
>> 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.jav
a
>> 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
>>
>>
>>
>
