[lldb] Show progress events in the command line driver

[JDevlieghere]

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

[JDevlieghere]

Remove unrelated formatting changes

[mib]

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]

• 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.

[JDevlieghere]

• 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

[labath]

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...

[JDevlieghere]

• Move progress event handling in the regular event handler thread

[JDevlieghere]

• Unstage lldb/include/lldb/Interpreter/CommandInterpreter.h.

[aprantl]

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]

lldb/source/Core/Debugger.cpp

1757

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

[JDevlieghere]

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]

• 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]

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]

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.

[clayborg]

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

[jingham]

In D120972#3368008, @clayborg wrote:

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.

[JDevlieghere]

In D120972#3368055, @jingham wrote:

In D120972#3368008, @clayborg wrote:

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]

In D120972#3368244, @JDevlieghere wrote:

In D120972#3368055, @jingham wrote:

In D120972#3368008, @clayborg wrote:

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

[thakis]

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.

[JDevlieghere]

In D120972#3369002, @thakis wrote:

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]

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]

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]

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.

[clayborg]

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

[jingham]

In D120972#3370529, @clayborg wrote:

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.

[clayborg]

In D120972#3370688, @jingham wrote:

In D120972#3370529, @clayborg wrote:

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.

[jingham]

In D120972#3370814, @clayborg wrote:

In D120972#3370688, @jingham wrote:

In D120972#3370529, @clayborg wrote:

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.

[jingham]

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

[labath]

In D120972#3370850, @jingham wrote:

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?

[jingham]

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

[JDevlieghere]

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.

[labath]

Thanks.

lldb/include/lldb/Core/Debugger.h

public:
   bool GetUseExternalEditor() const;
 
   bool SetUseExternalEditor(bool use_external_editor_p);
 
   bool GetUseColor() const;
 
   bool SetUseColor(bool use_color);
 
+  bool GetShowProgress() const;
+
   bool GetUseAutosuggestion() const;
 
   bool GetUseSourceCache() const;
 
   bool SetUseSourceCache(bool use_source_cache);
 
   bool GetHighlightSource() const;
 
protected:
   ///   If this optional parameter has a value, it indicates the unique
   ///   debugger identifier that this progress should be delivered to. If this
   ///   optional parameter does not have a value, the progress will be
   ///   delivered to all debuggers.
   static void ReportProgress(uint64_t progress_id, const std::string &message,
                              uint64_t completed, uint64_t total,
                              llvm::Optional<lldb::user_id_t> debugger_id);
 
+  void PrintProgress(const Debugger::ProgressEventData &data);
+
   bool StartEventHandlerThread();
 
   void StopEventHandlerThread();
 
   void PushIOHandler(const lldb::IOHandlerSP &reader_sp,
                      bool cancel_top_handler = true);
 
   bool PopIOHandler(const lldb::IOHandlerSP &reader_sp);
protected:
   lldb::thread_result_t DefaultEventHandler();
 
   void HandleBreakpointEvent(const lldb::EventSP &event_sp);
 
   void HandleProcessEvent(const lldb::EventSP &event_sp);
 
   void HandleThreadEvent(const lldb::EventSP &event_sp);
 
+  void HandleProgressEvent(const lldb::EventSP &event_sp);
+
   // Ensures two threads don't attempt to flush process output in parallel.
   std::mutex m_output_flush_mutex;
   void FlushProcessOutput(Process &process, bool flush_stdout,
                           bool flush_stderr);
 
   SourceManager::SourceFileCache &GetSourceFileCache() {
     return m_source_file_cache;
   }
protected:
 
   std::recursive_mutex m_script_interpreter_mutex;
   std::array<lldb::ScriptInterpreterSP, lldb::eScriptLanguageUnknown>
       m_script_interpreters;
 
   IOHandlerStack m_io_handler_stack;
   std::recursive_mutex m_io_handler_synchronous_mutex;
 
+  llvm::Optional<uint64_t> m_current_event_id;
+
   llvm::StringMap<std::weak_ptr<llvm::raw_ostream>> m_log_streams;
   std::shared_ptr<llvm::raw_ostream> m_log_callback_stream_sp;
   ConstString m_instance_name;
   static LoadPluginCallbackType g_load_plugin_callback;
   typedef std::vector<llvm::sys::DynamicLibrary> LoadedPluginsList;
   LoadedPluginsList m_loaded_plugins;
   HostThread m_event_handler_thread;
   HostThread m_io_handler_thread;

lldb/source/Core/CoreProperties.td

let Definition = "debugger" in {
   def UseExternalEditor: Property<"use-external-editor", "Boolean">,
     Global,
     DefaultFalse,
     Desc<"Whether to use an external editor or not.">;
   def UseColor: Property<"use-color", "Boolean">,
     Global,
     DefaultTrue,
     Desc<"Whether to use Ansi color codes or not.">;
+  def ShowProgress: Property<"show-progress", "Boolean">,
+    Global,
+    DefaultTrue,
+    Desc<"Whether to show progress or not.">;
   def UseSourceCache: Property<"use-source-cache", "Boolean">,
     Global,
     DefaultTrue,
     Desc<"Whether to cache source files in memory or not.">;
   def AutoOneLineSummaries: Property<"auto-one-line-summaries", "Boolean">,
     Global,
     DefaultTrue,
     Desc<"If true, LLDB will automatically display small structs in one-liner format (default: true).">;

lldb/source/Core/Debugger.cpp

 
 bool Debugger::SetUseColor(bool b) {
   const uint32_t idx = ePropertyUseColor;
   bool ret = m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
   SetPrompt(GetPrompt());
   return ret;
 }
 
+bool Debugger::GetShowProgress() const {
+  const uint32_t idx = ePropertyShowProgress;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+      nullptr, idx, g_debugger_properties[idx].default_uint_value != 0);
+}
+
 bool Debugger::GetUseAutosuggestion() const {
   const uint32_t idx = ePropertyShowAutosuggestion;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
       nullptr, idx, g_debugger_properties[idx].default_uint_value != 0);
 }
 
 bool Debugger::GetUseSourceCache() const {
   const uint32_t idx = ePropertyUseSourceCache;
lldb::thread_result_t Debugger::DefaultEventHandler() {
   listener_sp->StartListeningForEventSpec(m_broadcaster_manager_sp,
                                           thread_event_spec);
   listener_sp->StartListeningForEvents(
       m_command_interpreter_up.get(),
       CommandInterpreter::eBroadcastBitQuitCommandReceived |
           CommandInterpreter::eBroadcastBitAsynchronousOutputData |
           CommandInterpreter::eBroadcastBitAsynchronousErrorData);
 
+  if (!m_broadcaster.EventTypeHasListeners(Debugger::eBroadcastBitProgress)) {
+    listener_sp->StartListeningForEvents(&m_broadcaster,
+                                         Debugger::eBroadcastBitProgress);
+  }
+
   // Let the thread that spawned us know that we have started up and that we
   // are now listening to all required events so no events get missed
   m_sync_broadcaster.BroadcastEvent(eBroadcastBitEventThreadIsListening);
 
   bool done = false;
   while (!done) {
     EventSP event_sp;
     if (listener_sp->GetEvent(event_sp, llvm::None)) {
if (listener_sp->GetEvent(event_sp, llvm::None)) {
               if (data && data[0]) {
                 StreamSP output_sp(GetAsyncOutputStream());
                 if (output_sp) {
                   output_sp->PutCString(data);
                   output_sp->Flush();
                 }
               }
             }
+          } else if (broadcaster == &m_broadcaster) {
+            if (event_type & Debugger::eBroadcastBitProgress)
+              HandleProgressEvent(event_sp);
           }
         }
 
         if (m_forward_listener_sp)
           m_forward_listener_sp->AddEvent(event_sp);
       }
     }
   }

 }
 
 lldb::thread_result_t Debugger::IOHandlerThread() {
   RunIOHandlers();
   StopEventHandlerThread();
   return {};
 }
 
+void Debugger::HandleProgressEvent(const lldb::EventSP &event_sp) {
+  auto *data =
+      Debugger::ProgressEventData::GetEventDataFromEvent(event_sp.get());
+  if (!data)
+    return;
+
+  // Do some bookkeeping for the current event, regardless of whether we're
+  // going to show the progress.
+  const uint64_t id = data->GetID();
+  if (m_current_event_id) {
+    if (id != *m_current_event_id)
+      return;
+    if (data->GetCompleted())
+      m_current_event_id.reset();
+  } else {
+    m_current_event_id = id;
+  }
+
+  // Decide whether we actually are going to show the progress. This decision
+  // can change between iterations so check it inside the loop.
+  if (!GetShowProgress())
+    return;
+
+  // Determine whether the current output file is an interactive terminal with
+  // color support. We assume that if we support ANSI escape codes we support
+  // vt100 escape codes.
+  File &output = GetOutputFile();
+  if (!output.GetIsInteractive() || !output.GetIsTerminalWithColors())
+    return;
+
+  if (data->GetCompleted()) {
+    // Clear the current line.
+    output.Printf("\33[2K\r");
+    return;
+  }
+
+  // Print over previous line, if any.
+  output.Printf("\r");
+
+  // Print the progress message.
+  std::string message = data->GetMessage();
+  if (data->GetTotal() != UINT64_MAX) {
+    output.Printf("[%llu/%llu] %s...", data->GetCompleted(), data->GetTotal(),
+                  message.c_str());
+  } else {
+    output.Printf("%s...", message.c_str());
+  }
+
+  // Clear until the end of the line.
+  output.Printf("\x1B[K");
+
+  // Flush the output.
+  output.Flush();
+}
+
 bool Debugger::HasIOHandlerThread() { return m_io_handler_thread.IsJoinable(); }
 
 bool Debugger::StartIOHandlerThread() {
   if (!m_io_handler_thread.IsJoinable()) {
     llvm::Expected<HostThread> io_handler_thread = ThreadLauncher::LaunchThread(
         "lldb.debugger.io-handler", [this] { return IOHandlerThread(); },
         8 * 1024 * 1024); // Use larger 8MB stack for this thread
     if (io_handler_thread) {

 }
 
 Target &Debugger::GetSelectedOrDummyTarget(bool prefer_dummy) {
   if (!prefer_dummy) {
     if (TargetSP target = m_target_list.GetSelectedTarget())
       return *target;
   }
   return GetDummyTarget();
 }
 
 Status Debugger::RunREPL(LanguageType language, const char *repl_options) {
   Status err;
   FileSpec repl_executable;
 
   if (language == eLanguageTypeUnknown)
     language = GetREPLLanguage();
 

lldb/source/Symbol/LocateSymbolFile.cpp

 //===-- LocateSymbolFile.cpp ----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Symbol/LocateSymbolFile.h"
 
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
+#include "lldb/Core/Progress.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Reproducer.h"

 FileSpec
 Symbols::LocateExecutableSymbolFile(const ModuleSpec &module_spec,
                                     const FileSpecList &default_search_paths) {
   FileSpec symbol_file_spec = module_spec.GetSymbolFileSpec();
   if (symbol_file_spec.IsAbsolute() &&
       FileSystem::Instance().Exists(symbol_file_spec))
     return symbol_file_spec;
 
+  Progress progress(llvm::formatv(
+      "Locating external symbol file for {0}",
+      module_spec.GetFileSpec().GetFilename().AsCString("<Unknown>")));
+
   FileSpecList debug_file_search_paths = default_search_paths;
 
   // Add module directory.
   FileSpec module_file_spec = module_spec.GetFileSpec();
   // We keep the unresolved pathname if it fails.
   FileSystem::Instance().ResolveSymbolicLink(module_file_spec,
                                              module_file_spec);