Page MenuHomePhabricator

Fix multiple problems of lldb-mi blocking on input monitoring and needing a return.
ClosedPublic

Authored by abidh on Feb 10 2015, 2:42 AM.

Details

Summary

One of the problem is reported here.
http://llvm.org/bugs/show_bug.cgi?id=22411

A fix was committed for this problem that works only for OSX. This revision
extends that fix to other system. The select system call has some limitation with
multi-threaded application which have been addresses here.

LLDB-mi exits if quit command is given but needs an extra retur if -gdb-exit is
given. That issue has also been addressed.

Diff Detail

Event Timeline

abidh updated this revision to Diff 19655.Feb 10 2015, 2:42 AM
abidh retitled this revision from to Fix multiple problems of lldb-mi blocking on input monitoring and needing a return..
abidh updated this object.
abidh edited the test plan for this revision. (Show Details)
abidh added reviewers: emaste, ki.stfu.
abidh added a subscriber: Unknown Object (MLST).
ki.stfu added inline comments.Feb 10 2015, 3:18 AM
test/tools/lldb-mi/TestMiExit.py
33–34

It checks that lldb-mi really exited. If it doesn't work on Linux we can skip test or it should be commented out and we should mark it as "FIXME" for future fixes.
I think "skip test" is preferable.

tools/lldb-mi/MICmnStreamStdinLinux.cpp
162–164

Have you tried to monitor errors using 3rd arg of select?

165

timeout is 4th argument

tools/lldb-mi/MIDriver.cpp
524 ↗(On Diff #19655)

I do not like this "if", but you have extended its functionality. Maybe it would be better to call CMICmnStreamStdinLinux::InterruptReadLine() from -gdb-exit command? or use CMIDriver::GetExitApplicationFlag() instead of m_waitForInput in CMICmnStreamStdinLinux::InputAvailable()?

ki.stfu added inline comments.Feb 10 2015, 3:25 AM
test/tools/lldb-mi/TestMiExit.py
31

please remove it also from test_lldbmi_stopped_when_stopatentry_remote (test/tools/lldb-mi/TestMiNotification.py).

abidh added inline comments.Feb 10 2015, 3:32 AM
test/tools/lldb-mi/TestMiExit.py
33–34

I will restore the checks. It was failing when I have not done the select fix. Now it passes on Linux.

tools/lldb-mi/MICmnStreamStdinLinux.cpp
162–164

The documentation in man pages is quite explicit.
"On Linux (and some other systems), closing the file descriptor in another thread has no

effect on select()."
165

Thanks. I will fix this. It is actually 5th :-)

tools/lldb-mi/MIDriver.cpp
524 ↗(On Diff #19655)

Yes. I dont like it either. It was like a quick fix. I will look at the alternate options.

abidh added a comment.Feb 10 2015, 3:47 AM

@emaste, @ki.stfu
There is a problem with this revision. Dont review/test it for now. I will post an updated one later. Sorry for inconvenience.

ki.stfu edited edge metadata.Feb 10 2015, 3:47 AM
In D7529#121171, @abidh wrote:

Yes. I dont like it either. It was like a quick fix. I will look at the alternate options.

lgtm except the line #524.

abidh planned changes to this revision.Feb 10 2015, 4:32 AM
abidh updated this revision to Diff 19671.Feb 10 2015, 5:30 AM
abidh edited edge metadata.

Following changes are done in this revision.

  1. Initialize values to the arguments of select inside loop as they can be changed by select call.
  2. Assign values of m_waitForInput correctly.
  3. Remove sepcial check for -gdb-exit. Above 2 are enough to make sure lldb-mi exits without needing an extra return.
  4. Remove some changes from test file as suggested in review.
ki.stfu accepted this revision.Feb 10 2015, 5:58 AM
ki.stfu edited edge metadata.

Nice. Lgtm.

I will check it on OS X later.

This revision is now accepted and ready to land.Feb 10 2015, 5:58 AM
ki.stfu added inline comments.Feb 10 2015, 6:00 AM
tools/lldb-mi/MICmnStreamStdinLinux.cpp
191

Set vwbAwail to false before this return, ant return success here.

ki.stfu requested changes to this revision.Feb 10 2015, 6:01 AM
ki.stfu edited edge metadata.
This revision now requires changes to proceed.Feb 10 2015, 6:01 AM
ki.stfu accepted this revision.Feb 10 2015, 6:15 AM
ki.stfu edited edge metadata.
ki.stfu added inline comments.
tools/lldb-mi/MICmnStreamStdinLinux.cpp
191

no. it is right.

This revision is now accepted and ready to land.Feb 10 2015, 6:15 AM

All tests work on OS X.

emaste edited edge metadata.Feb 10 2015, 9:01 AM

I think the comment at the beginning of CMICmnStreamStdinLinux::InputAvailable is more verbose than necessary now. It was important to explain why things were done in a certain way when the ifdef APPLE guards were in place, but now I think having of all of the history described there may just be confusing.

I think this change is generally an improvement, although the tests still hang on FreeBSD with this change in place.

abidh closed this revision.Feb 10 2015, 9:12 AM

I think this change is generally an improvement, although the tests still hang on FreeBSD with this change in place.

Actually, I think this is incorrect: there are still four lldb-mi tests that hang on FreeBSD, but prior to this change I think the vast majority of them failed. So thank you, this seems like a big improvement. I will investigate further and either update the details in the PR or submit a new PR as appropriate.

abidh added a comment.Feb 12 2015, 1:18 AM

The real problem is that lldb-mi has a very poor design regarding threading. It is using 3 threads.

  1. To wait on input
  2. To process commands
  3. To handle event.

These problems mostly occur due to interaction of 1 & 2. I also found out that running 2 & 3 at the same time produces race condition so I have put a mutex on them so that they dont run at the same time. The signal handling in totally messed up too. I think lldb-mi should be changed to be a single threaded app (or first 2 threads should be merged at least). It will get rid of a lot of complexity and these problems.

In D7529#122418, @abidh wrote:

The real problem is that lldb-mi has a very poor design regarding threading. It is using 3 threads.

  1. To wait on input
  2. To process commands
  3. To handle event.

    These problems mostly occur due to interaction of 1 & 2. I also found out that running 2 & 3 at the same time produces race condition so I have put a mutex on them so that they dont run at the same time. The signal handling in totally messed up too. I think lldb-mi should be changed to be a single threaded app (or first 2 threads should be merged at least). It will get rid of a lot of complexity and these problems.

I agree that we should merge 1st and 2nd threads.
Is there any chance that you will be working on it?

abidh added a comment.Feb 12 2015, 2:09 AM

I am working on cleanup of GetVariableInfo () family of functions and making sure that struct/array are printed correctly by the -stack-list* commands. But I will get to these threading issues sometime next week.