Page MenuHomePhabricator

Implement 'up' and 'down' shortcuts in lldb gui
ClosedPublic

Authored by llunak on Oct 5 2019, 12:24 PM.

Details

Summary

The attached patch implements 'u' and 'd' keyboard shortcuts in lldb gui, similar to gdb tui's shortcuts.

Diff Detail

Event Timeline

llunak created this revision.Oct 5 2019, 12:24 PM

Would it be possible to add a test for this? Maybe you can extend TestGuiBasic.py.

Would it be possible to add a test for this? Maybe you can extend TestGuiBasic.py.

I can, although to me it looks like a new test would be better. But first of all I wanted some feedback on this, as the patch in this form doesn't even compile. I'd actually personally prefer to just nuke the "Detach and resume process" shortcut, as I not only fail to see its usefulness but I also find it dangerous (I certainly don't want to hit it by mistake and ruin my debugging session), but then I assume there probably was a reason why it was added.

It would be fine to remove the 'd' shortcut for detach and resume if we need to by moving this functionality up into the menu bar. Right now the Process menu has "Detach" only, so it might be good to first change the "Process" menu to have "Detach" or "Detach suspended". I agree that accidentally hitting 'd' would be bad.

llunak added a comment.Oct 8 2019, 1:49 PM

It would be fine to remove the 'd' shortcut for detach and resume if we need to by moving this functionality up into the menu bar. Right now the Process menu has "Detach" only, so it might be good to first change the "Process" menu to have "Detach" or "Detach suspended". I agree that accidentally hitting 'd' would be bad.

Ok, I can try to do that. And 'k' for "Kill process" should probably move to the menu as well.

But another shortcut I'd like to change is 'o' for "Step out", as that one is 'f' for "Finish" in gdb tui, and I'm rather used to that. Unless you'd be willing to accept a patch changing that, can you comment on the idea of making the shortcuts configurable?

It would be fine to remove the 'd' shortcut for detach and resume if we need to by moving this functionality up into the menu bar. Right now the Process menu has "Detach" only, so it might be good to first change the "Process" menu to have "Detach" or "Detach suspended". I agree that accidentally hitting 'd' would be bad.

Ok, I can try to do that. And 'k' for "Kill process" should probably move to the menu as well.

Sounds fine to me!

But another shortcut I'd like to change is 'o' for "Step out", as that one is 'f' for "Finish" in gdb tui, and I'm rather used to that.

I am fine with this as well.

Unless you'd be willing to accept a patch changing that, can you comment on the idea of making the shortcuts configurable?

Configurability would be nice, happy to accept patches that implement that.

I created "gui" mode many many years ago and hoped people would jump in and start making patches, so I am very happy to see this patch and look forward to more. It won't take too much work for us to make "gui" mode really useful, just a bit of work!

llunak updated this revision to Diff 224737.Oct 12 2019, 4:46 AM
llunak edited the summary of this revision. (Show Details)

Added a unittest.

Also, the other discussed changes are at https://reviews.llvm.org/D68908 and https://reviews.llvm.org/D68909 .

labath added a subscriber: labath.Oct 14 2019, 1:31 AM
labath added inline comments.
lldb/packages/Python/lldbsuite/test/commands/gui/basicdebug/TestGuiBasicDebug.py
32

What's the reason for that? It'd be best to not mess with the timeout in the test, and particularly to not set such a small time out as it will likely lead to flakyness on slow/heavily loaded machines.

llunak marked an inline comment as done.Oct 15 2019, 10:07 AM
llunak added inline comments.
lldb/packages/Python/lldbsuite/test/commands/gui/basicdebug/TestGuiBasicDebug.py
32

I added it because writing and debugging the unittest without this was a pain, but fair enough.

llunak updated this revision to Diff 225064.Oct 15 2019, 10:08 AM

Removed the explicit test timeout.

clayborg requested changes to this revision.Oct 15 2019, 11:41 AM
clayborg added inline comments.
lldb/packages/Python/lldbsuite/test/commands/gui/basicdebug/TestGuiBasicDebug.py
33

Just checking for the source is probably not enough here. We need to check for the source _and_ the stop reason and ensure they are on the same line. If the output to the screen was:

return 1; // In function
other code here;  <<< Thread 1: step in

We would want this to fail (it will currently succeed), but we want it to succeed if we get it all on one line:

return 1; // In function           <<< Thread 1: step in

This applies to all expect_exact calls below as well.

So this should probably be something that uses a regex like:

self.child.expect("return 1; // In function[^\r\n]+<<< Thread 1: step in")
This revision now requires changes to proceed.Oct 15 2019, 11:41 AM
llunak updated this revision to Diff 225102.Oct 15 2019, 12:59 PM

Adjusted the testcase.

llunak marked an inline comment as done.Oct 15 2019, 12:59 PM
clayborg accepted this revision.Oct 15 2019, 1:08 PM

Looks good! With a little work, "gui" mode can really become great. I hope to see more patches? Happy to discuss next stops on the mailing list if you are interested!

This revision is now accepted and ready to land.Oct 15 2019, 1:08 PM

Looks good!

Can you please ACK also the other patches linked above that this one depends on? Or can I take the discussion here as being enough?

With a little work, "gui" mode can really become great. I hope to see more patches? Happy to discuss next stops on the mailing list if you are interested!

That's kind of the plan. I rather miss a variables view in gdb tui and I've decided that it should be simpler to implement a command prompt in lldb gui than a variables view in gdb tui. But I'm still evaluating, I've got other things on my plate, only so much time and it's a question if it'd be really just a little work to make lldb meet my needs (and I've already run into few lame problems such as one of those harmless signals like SIGTSTP simply killing the debugging without any useful feedback). We'll see how this goes.

labath added inline comments.Oct 16 2019, 3:48 AM
lldb/packages/Python/lldbsuite/test/commands/gui/basicdebug/TestGuiBasicDebug.py
32

Yeah, the pexpect tests leave a lot to be desired, and there are ways to avoid waiting for the entire timeout period when the output does not match (in the common cases), but it needs someone willing to implement it.

The deficiencies of pexpect become particularly obivous in the "gui" tests as there the raw stream of bytes coming out of the pty is way too low-level to be able to match it reliably. If we take the [^\r\n] pattern above, it encodes a lot of assumptions about how lldb and ncurses choose to print to the screen -- ncurses could perfectly well decide to insert some line breaks after printing the source code, but then go back (via ansi cursor movement codes), and print the stop reason. The result would be exactly the same as far as the user is concerned, but the test would fail.

For testing the gui mode, I think we should build (or reuse, if something suitable exists) a very simple "terminal emulator" which "understands" cursor movement codes (and maybe some other stuff). Then we could assert that the contents of the line as the user would see it instead of how it happened to be printed out.

Anyway, I don't think you have to do that now, but it is something to think about, should you want to invest more heavily into the lldb "gui".

llunak added a comment.Nov 2 2019, 2:25 AM

Can you please ACK also the other patches linked above that this one depends on? Or can I take the discussion here as being enough?

Ping?

What is the state of the patch? Does lldb support cgdb-style u/d/f/b etc now?

Yikes, sorry for not responding for so long. In the fall I was out on medical leave due to a head injury. Please feel free to ping more often if I do this again. I commented in the other patches (one accepted, and questions in the other). Let me know your thoughts.

lldb/source/Core/IOHandler.cpp
4433 ↗(On Diff #225102)

Clang format this?

if (thread->SetSelectedFrameByIndex(frame_idx, true))

My usual experience with Clang submissions is that I have to be really persistent to get a reaction, let alone an approval, so I assumed it was the same here and I didn't feel like pushing this that much. I'll update the patches to match current git and have a look at the review that still has questions.

llunak updated this revision to Diff 281705.Jul 29 2020, 12:38 PM

Updated for current git, and clang-format-ed.

clayborg accepted this revision.Jul 29 2020, 1:02 PM

Thanks again for working on this. I will review any subsequent patches as they come in if you have any more contributions. Sorry for the long delay again, I had 3 months that I was out due to the head injury, hopefully this won't happen again!