This is an archive of the discontinued LLVM Phabricator instance.

Make lldb-mi handle only MI commands
ClosedPublic

Authored by abidh on Mar 17 2015, 9:42 AM.

Details

Summary

Previously lldb-mi can also act as a driver like normal lldb. It needed a lot
of redundant code in the lldb-mi for that. I think that if a user wants command
line lldb driver then he/she should use the lldb for it. This change will cleanup
the code a lot. When this change goes in, it will allow us to remove some more
redundant code too.

All tests pass on Linux. Did some sanity testing on command line and with eclipse.

Diff Detail

Event Timeline

abidh updated this revision to Diff 22099.Mar 17 2015, 9:42 AM
abidh retitled this revision from to Make lldb-mi handle only MI commands.
abidh updated this object.
abidh edited the test plan for this revision. (Show Details)
abidh added a reviewer: ki.stfu.
abidh added a subscriber: Unknown Object (MLST).
ki.stfu edited edge metadata.EditedMar 17 2015, 10:46 AM

I don't mind about cleaning, but I disagree about CLI support in lldb-mi. I think support of mixed commands is useful (sometimes), and actually I planned to add this for lldb-mi one fine day.
For example, lldb-mi supports "quit" command (aka stdin hack) to be more friendly with users.

I tried to add this support using the CMIDriver::SetEnableFallThru(true), but this realization contained a lot of bugs. Therefore I preferred to wrap all CLI commands into "-interpreter-exec" and I don't mind about cleaning of lldb-mi.

ki.stfu accepted this revision.Mar 17 2015, 10:50 AM
ki.stfu edited edge metadata.

All tests pass on OS X.

As I understood you will remove Driver.cpp later, right?

This revision is now accepted and ready to land.Mar 17 2015, 10:50 AM
abidh added a comment.Mar 18 2015, 3:09 AM

All tests pass on OS X.

As I understood you will remove Driver.cpp later, right?

Yes. I will remove it later. Thanks for review.

abidh closed this revision.Mar 18 2015, 3:10 AM
ted added a subscriber: ted.EditedMar 20 2015, 7:47 AM

gdb handles command line gdb and MI in one executable. I think lldb should do the same. We ship one executable, called hexagon-lldb, that is actually lldb-mi. Pass --interpreter and it runs MI, otherwise it runs the standard LLDB CLI.

I'd really prefer one executable instead of two.

If you go through with this, PLEASE make sure the ctrl-c handler works on all platforms. This has been a problem for us on Windows, so I want to make sure after this change it still works. Also, ctrl-c shouldn't quit MI if a target isn't running. Imagine accidentally hitting ctrl-c twice to stop a target, but the second one quits.

Hello Ted,

As I said above I planned to add support for CLI commands in lldb-mi. Here
is my patch for that: http://reviews.llvm.org/D8483

Thanks,
Ilia

abidh added a comment.Mar 20 2015, 9:12 AM

I don't mind about cleaning, but I disagree about CLI support in lldb-mi.

In D8381#144097, @ted wrote:

gdb handles command line gdb and MI in one executable. I think lldb should do the same. We ship one executable, called hexagon-lldb, that is actually lldb-mi. Pass --interpreter and it runs MI, otherwise it runs the standard LLDB CLI.

I'd really prefer one executable instead of two.

If you go through with this, PLEASE make sure the ctrl-c handler works on all platforms. This has been a problem for us on Windows, so I want to make sure after this change it still works. Also, ctrl-c shouldn't quit MI if a target isn't running. Imagine accidentally hitting ctrl-c twice to stop a target, but the second one quits.

Ted,
Thanks for your concerns. I also faced this double ctrl-c issue in my internal branch that I build with mingw and I think I fixed it. The issue was non persistent single handling on Windows using signal API. MSVC uses SetConsoleCtrlHandler and this should not be a problem there. But I will keep an eye.

Regarding single executable, unfortunately, we already have 2. Trying to make lldb-mi also behave as lldb causes a lot of code duplication and #ifdef mess. I think it is better that it does one job and does it well.

In D8381#144097, @ted wrote:

Also, ctrl-c shouldn't quit MI if a target isn't running. Imagine accidentally hitting ctrl-c twice to stop a target, but the second one quits.

I checked it in GDB/MI on Windows and it works as you said it shouldn't work. or did you mean that GDB/MI also doesn't work the way you want?

ted added a comment.Mar 20 2015, 9:49 AM

I would like ctrl-c to not exit lldb-mi. "quit" exits, and it's too easy to hit ctrl-c when the target isn't running and accidentally exit lldb-mi.

I get my desired behavior by removing:

m_bExitApp = true;
m_rStdin.OnExitHandler();

from the bottom of CMIDriver::SetExitApplicationFlag() in MIDriver.cpp.