This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Show progress events in the command line driver
ClosedPublic

Authored by JDevlieghere on Mar 3 2022, 9:20 PM.

Details

Summary

This patch adds support for showing progress events when using lldb on the command line. It spawns a separate thread that listens for progress events and prints them to the debugger's output stream.

It's nothing fancy (yet), for now it just prints the progress message. If we know the total number of items being processed, we prefix the message with something like [1/100], similar to ninja's output. It doesn't use any fancy terminal manipulation: it uses a simple carriage return (\r) to bring the cursor to the front of the line. If we support ANSI escape codes, we use a vt100 escape code to clear the current line when a progress event is complete. Otherwise we just overwrite it with as many spaces as the current terminal is wide.

Here's a recording of what this looks like in practice: https://asciinema.org/a/HBMDelpb9XDKccXZ09mgfuJ8R

Diff Detail

Event Timeline

JDevlieghere created this revision.Mar 3 2022, 9:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 9:20 PM
JDevlieghere requested review of this revision.Mar 3 2022, 9:20 PM

Remove unrelated formatting changes

mib added a comment.Mar 3 2022, 9:35 PM

Awesome! I could see this getting improved in the future by stacking each progress event in the output with their completion amount and popping the ones that are completed.

lldb/source/Core/Debugger.cpp
1741
1741

typo

1829

Why ?

JDevlieghere marked 3 inline comments as done.
  • Fix typo
  • Reduce stack size
lldb/source/Core/Debugger.cpp
1829

I copied this from StartIOHandlerThread, but you're absolutely right, we don't need that big of stack to handle events.

  • Reduce flickering by not printing the last status message when the the progress is complete
  • Add ... as suggested by Adrian offline
  • Make ANSI (vt100) escape codes a requirement in order to show progress to simplify the code

I like the feature, but why does it need to have a thread of it's own? Could this be done from inside the regular event handler thread? The event handler thread also prints to stdout, and it seems like a bad idea to have two threads trying to do the same...

  • Move progress event handling in the regular event handler thread
  • Unstage lldb/include/lldb/Interpreter/CommandInterpreter.h.

This is pretty (and) awesome.

lldb/source/Core/Debugger.cpp
1670

side note: this function could benefit from some early exits.

1762

I guess Colors also implies it supports \r? Sure.
Can you comment what this escape sequence does?

aprantl added inline comments.Mar 4 2022, 3:24 PM
lldb/source/Core/Debugger.cpp
1757

And withColors also implies that it's an interactive TTY?

JDevlieghere marked 2 inline comments as done.Mar 4 2022, 3:40 PM
JDevlieghere added inline comments.
lldb/source/Core/Debugger.cpp
1757

Yup

1762

The comment on line 1760 explains that this clears the current line. I can move it down to make it more obvious.

JDevlieghere marked 2 inline comments as done.
  • Make sure the output is both interactive and supports colors
  • Add more comments
lldb/source/Core/Debugger.cpp
1757

Actually, I thought it did based on the docs, but looking at the implementation it's tracked by two different properties.

clayborg added inline comments.Mar 7 2022, 3:21 PM
lldb/source/Core/CoreProperties.td
137

Might be nice to clarify that this is for the CLI only?

Also, if this _is_ for the CLI only, the setting should probably be put into the "interpreter" settings as "interpreter.show-progress".

lldb/source/Core/Debugger.cpp
1756–1757

Move this to the top of the function so we don't do any work extracting anything from the event if it is disabled? Or is this code trying to limit the updates of a progress that reports many status updates for the same progress?

1763

If not interactive should we just dump the start and end progress events on a separate line?

1768

Do we want some sort of format string here that the user could modify as a setting? The idea would be there might be extra settings that the user could set like:

(lldb) setting set interpreter.progress-clear-line-format "${ansi....}"

and it could default to the above string. Not required, just thinking out loud here as I am reading the patch

1778–1779

If we did a format string for each message we could have something like:

"{${progress.is_start}...}{${progress.is_update}...}{${progress.is_end}...}"

where the "progress.is_start" variable would be true for the first progress event, "progress.is_update" would be true for any updates, and "progress.is_end" would be true if the progress is completed. This would allow people to customize how progress events get handled and printed. If someone just wants a start and end progress, then they can fill in the "..." after the "progress.is_start" and "progress.is_end". If they don't want updates, they can leave out the "{${progress.is_update}...}" section. It also would allow ansi colors to be used since we already support these. And this would allow non interactive sessions to still show progress if they want to (right now if it isn't interactive, it doesn't get shown).

JDevlieghere marked 3 inline comments as done.

Thanks for the review Greg!

lldb/source/Core/CoreProperties.td
137

I've updated the description to make it clear that this hinges of the debugger output being an interactive color enabled terminal.

lldb/source/Core/Debugger.cpp
1756–1757

Kind of. m_current_event_id ensures that we only deal with one event at the same time. If we moved this check before the m_current_event_id bookkeeping, someone could disable progress before the current progress event completes resulting in m_current_event_id never getting cleared.

For example:

event 1 begin -> m_current_event_id = 1
progress disabled 
event 1 end -> ignored
progress enabled
event 2 begin -> ignored because m_current_event_id == 1
and now every subsequent event is ignored because m_current_event_id will never get updated
1763

I (personally) think that will quickly generate too much output.

1768

Part of that is covered in https://reviews.llvm.org/D121062.

I didn't make the vt100 escape codes configurable. They're the same as what editline uses (which aren't configurable either) and to me are just implementation details.

1778–1779

I intentionally kept things simple for now, but this is definitely something we could make more sophisticated/configurable in the future.

This is fine as a starting point, my only question is if the setting should be "interpreter.show-progress"

This is fine as a starting point, my only question is if the setting should be "interpreter.show-progress"

Whether you see this output is really dependent on how the debugger's I/O is set up, which seems more like a feature of the Debugger than of the CommandInterpreter. This seems more like the stop display than like whether we prompt on quit or echo commands.

This is fine as a starting point, my only question is if the setting should be "interpreter.show-progress"

Whether you see this output is really dependent on how the debugger's I/O is set up, which seems more like a feature of the Debugger than of the CommandInterpreter. This seems more like the stop display than like whether we prompt on quit or echo commands.

Exactly, there's nothing that ties this to the interpreter, so I think it makes sense to keep it where it is. I also tried to make that more explicit by referencing the debugger's output in the help message.

clayborg accepted this revision.Mar 8 2022, 2:58 PM

This is fine as a starting point, my only question is if the setting should be "interpreter.show-progress"

Whether you see this output is really dependent on how the debugger's I/O is set up, which seems more like a feature of the Debugger than of the CommandInterpreter. This seems more like the stop display than like whether we prompt on quit or echo commands.

Exactly, there's nothing that ties this to the interpreter, so I think it makes sense to keep it where it is. I also tried to make that more explicit by referencing the debugger's output in the help message.

ok. Thanks for the info

This revision is now accepted and ready to land.Mar 8 2022, 2:58 PM
aprantl accepted this revision.Mar 8 2022, 5:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 6:27 PM
thakis added a subscriber: thakis.Mar 8 2022, 7:30 PM

This breaks building on Windows where GetMessage is defined to GetMessageW: http://45.33.8.238/win/54664/step_4.txt

Please fix (just rename to something else), or revert for now if it takes a while to fix.

This breaks building on Windows where GetMessage is defined to GetMessageW: http://45.33.8.238/win/54664/step_4.txt

Please fix (just rename to something else), or revert for now if it takes a while to fix.

This was fixed by 43374bee0e0641be417f22fc73c46a914de5ea71

labath added inline comments.Mar 9 2022, 4:36 AM
lldb/source/Core/Debugger.cpp
1614

Why is this conditional on someone else listening for this event? We don't e.g. suppress the "Thread #N" blurb if someone is listening for eBroadcastBitThreadSelected

1614

(listening for this event at a specific point in time)

JDevlieghere added inline comments.Mar 9 2022, 9:52 AM
lldb/source/Core/Debugger.cpp
1614

My reasoning was that I didn't want us to handle events if someone else already is. What would be the canonical way to do this? Are you suggestion that we check it in the loop/callback?

labath added inline comments.Mar 9 2022, 10:36 AM
lldb/source/Core/Debugger.cpp
1614

I think the canonical way is to not do that. :) Events (except eStateChanged events) can go to multiple listeners and each one can handle them in his own way.

And there already is a setting controlling whether to print them, so anyone not interested can turn them off that way.

I liked that part of the patch where it wouldn't report events through the debugger if someone is already handling them. lldb-vscode handles these events already, but we also set the debugger's out and error file handle to /dev/null

I liked that part of the patch where it wouldn't report events through the debugger if someone is already handling them. lldb-vscode handles these events already, but we also set the debugger's out and error file handle to /dev/null

This seems a little like the stop notification, where we don't emit that info when running under Xcode (or I presume vscode). In the case of the stop notification, we're gating that on the Debugger::GetAsyncOutput stream being set or not. In this case we're switching on whether the result of Debugger::GetOutputFile is interactive. In both cases, this seems more like something the client should turn on and off explicitly, on for the Driver, off for Xcode & VSCode? Doing it based on whether the terminal is interactive or has an async output channel seems a little indirect.

I liked that part of the patch where it wouldn't report events through the debugger if someone is already handling them. lldb-vscode handles these events already, but we also set the debugger's out and error file handle to /dev/null

This seems a little like the stop notification, where we don't emit that info when running under Xcode (or I presume vscode). In the case of the stop notification, we're gating that on the Debugger::GetAsyncOutput stream being set or not. In this case we're switching on whether the result of Debugger::GetOutputFile is interactive. In both cases, this seems more like something the client should turn on and off explicitly, on for the Driver, off for Xcode & VSCode? Doing it based on whether the terminal is interactive or has an async output channel seems a little indirect.

This is only in the built in event loop, which lldb-vscode and Xcode do not use, they run their own event loops. So the only thing we are left with is the command line driver which uses this event loop.

I liked that part of the patch where it wouldn't report events through the debugger if someone is already handling them. lldb-vscode handles these events already, but we also set the debugger's out and error file handle to /dev/null

This seems a little like the stop notification, where we don't emit that info when running under Xcode (or I presume vscode). In the case of the stop notification, we're gating that on the Debugger::GetAsyncOutput stream being set or not. In this case we're switching on whether the result of Debugger::GetOutputFile is interactive. In both cases, this seems more like something the client should turn on and off explicitly, on for the Driver, off for Xcode & VSCode? Doing it based on whether the terminal is interactive or has an async output channel seems a little indirect.

This is only in the built in event loop, which lldb-vscode and Xcode do not use, they run their own event loops. So the only thing we are left with is the command line driver which uses this event loop.

It's a little lower than just the event loop, since this gets done in Process::HandleProcessStateChangedEvent. That gets called in a small handful of places including Process::WaitForProcessToStop, which gets called from a bunch of other places.

But using the standard event loop and "I want to handle async notifications by hand" seem like they should be separable choices.

But using the standard event loop and "I want to handle async notifications by hand" seem like they should be separable choices.

Is the show-progress setting not enough of separation?

That's certainly good enough for users.

If you were the developer of another client, I think you'd want to keep people from inadvertently turning this back on. But that's probably a minor concern. And there isn't an equivalent good way to turn off the stop description from being printed if we ever make our way to HandleProgressEvent.

Jim

To summarize: I will remove the check because this doesn't matter for VSCode/Xcode because they don't use the default event loop and for those clients who might care about this, they should use the setting.