The attached patch implements 'u' and 'd' keyboard shortcuts in lldb gui, similar to gdb tui's shortcuts.
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.
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?
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!
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.
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")
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.
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".
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.
|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.
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!