This is an archive of the discontinued LLVM Phabricator instance.

Remove quit hook in CMIDriver::DoMainLoop (MI)
ClosedPublic

Authored by ki.stfu on Apr 25 2015, 3:03 PM.

Details

Summary

This patch removes quit hook and fixes 1 bug:

  1. Fix "quit" hook in CMIDriver::DoMainLoop (MI)
  2. Fix bug when the handler thread exits without any notification (MI)
  3. Fix a race condition in CMICmnLLDBDebugger::MonitorSBListenerEvents (MI)

Diff Detail

Event Timeline

ki.stfu updated this revision to Diff 24434.Apr 25 2015, 3:03 PM
ki.stfu retitled this revision from to Remove quit hook in CMIDriver::DoMainLoop (MI).
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu added a reviewer: abidh.
ki.stfu added subscribers: abidh, Unknown Object (MLST).
abidh added inline comments.Apr 27 2015, 2:05 AM
tools/lldb-mi/MIDriver.cpp
554

How is this supposed to work. Will this thread block until the other thread gets an event. what happens when a command does not generate any event?

ki.stfu added inline comments.Apr 27 2015, 2:53 AM
tools/lldb-mi/MIDriver.cpp
554

It will block only if there are events that should be handled, otherwise it will return immediately.

abidh added inline comments.Apr 28 2015, 10:29 AM
tools/lldb-mi/MIDriver.cpp
554

I may be misunderstanding but is it not a race condition. You are expecting that you would get the exit event when you reach this line.
CMICmnLLDBDebugger::Instance().WaitForHandleEvent();

What if lldb takes more time and you find the event queue empty?

ki.stfu added inline comments.Apr 28 2015, 12:09 PM
tools/lldb-mi/MIDriver.cpp
554

The quit command puts the exit event into queue, therefore there is no race condition. It will work always.

abidh added inline comments.Apr 29 2015, 1:34 AM
tools/lldb-mi/MIDriver.cpp
554

I am not saying that quit will not create the event. My point is that "will event always be there" before this thread goes to check the queue. Because if it is late, this thread will block in fgets and then you will need an extra enter key for lldb-mi to quit.

Because that event is generated by a separate thread in lldb, it does not seem to me that we can guarantee that event will always be there when we go and check the queue.

ki.stfu added inline comments.Apr 29 2015, 1:45 AM
tools/lldb-mi/MIDriver.cpp
554

I am not saying that quit will not create the event. My point is that "will event always be there" before this thread goes to check the queue. Because if it is late, this thread will block in fgets and then you will need an extra enter key for lldb-mi to quit.

As I said above, the quit command (i.e. CommandObjectQuit::DoExecute) queues the exit event, and therefore if we look into this queue later from another thread then this event will be there.

ki.stfu planned changes to this revision.Apr 29 2015, 10:02 PM
ki.stfu updated this revision to Diff 24695.Apr 30 2015, 4:15 AM

Fix a race condition in CMICmnLLDBDebugger::MonitorSBListenerEvents (MI)

ki.stfu updated this object.Apr 30 2015, 4:15 AM

Friendly ping.

abidh edited edge metadata.May 5 2015, 2:38 AM

Friendly ping.

I will try to test it later today to make sure that no race condition exist.

abidh accepted this revision.May 5 2015, 8:15 AM
abidh edited edge metadata.

OK I can see how it is working. The command processing thread would have put the event in the queue before returning. So there is no chance of us not finding it. Apart from a few inline comments, looks good.

tools/lldb-mi/MICmnLLDBDebugger.cpp
225

It would be better to add a few lines of comment to explain how it waits for empty queue which means that all events have been processed by the MonitorSBListenerEvents.

681

I am not sure what our policy is about using assert.

This revision is now accepted and ready to land.May 5 2015, 8:15 AM
ki.stfu updated this revision to Diff 25133.May 6 2015, 11:47 PM
ki.stfu edited edge metadata.

Add explanation how does the WaitForHandleEvent work

ki.stfu closed this revision.May 6 2015, 11:49 PM