[lldb/Core] Fix "sticky" long progress messages

[mib]

Summary

When the terminal window is too small, lldb would wrap progress messages
accross multiple lines which would break the progress event handling
code that is supposed to clear the message once the progress is completed.

This causes the progress message to remain on the screen, sometimes partially,
which can be confusing for the user.

To fix this issue, this patch trims the progress message to the terminal
width taking into account the progress counter leading the message for
finite progress events and also the trailing ....

rdar://91993836

Signed-off-by: Med Ismail Bennani <[e-mail]>

[JDevlieghere]

Can we add a pexpect test for this?

lldb/source/Core/Debugger.cpp

1884–1886

Should this account for the control characters? Do they cause a line wrap?

[mib]

lldb/source/Core/Debugger.cpp

1884–1886

In my understanding, if the control character is complete, it shouldn't print anything to the screen so they shouldn't cause a line wrap

[mib]

Add test

[JDevlieghere]

lldb/source/Core/Debugger.cpp

1884–1886

Instead of computing the length string like this, would it make sense to write the message to a temporary stream/buffer and then trim the string dumping it to the real async output stream? I'm worried about the two accidentally getting out of sync.

lldb/test/API/functionalities/progress_reporting/TestTrimmedProgressReporting.py

26–35

I don't understand how this guarantees that we cut off the string at the exact width of the window?

[mib]

Add test

[JDevlieghere]

lldb/test/API/functionalities/progress_reporting/TestTrimmedProgressReporting.py

30–31

I understand the motivation, but I think it's a bad idea to intentionally introduce non-determinism in a test. Let's just make this a constant or repeat the test for two (fixed) widths.

37–60

Why not use self.child.expect_exact with the exact sequence (including escape characters) that you're trying to match? This seems to reimplementing part of the pexpect functionality and I'm not sure we actually need it.

[mib]

Address @JDevlieghere comments

[JDevlieghere]

lldb/test/API/functionalities/progress_reporting/TestTrimmedProgressReporting.py

50–51

These two things are pretty Apple specific so maybe use the skipUnlessDarwin decorator? I think the locating symbol files will be printed on all platforms, but if we need to skip the test on Linux anyway that doesn't really buy us much to reduce the test coverage.

[JDevlieghere]

LGTM with the decorator

[mib]

lldb/test/API/functionalities/progress_reporting/TestTrimmedProgressReporting.py

50–51

Indeed! Thanks!

[mib]

Only run test on Darwin platforms

lldb/include/lldb/Core/DebuggerEvents.h

public:
   static ConstString GetFlavorString();
 
   ConstString GetFlavor() const override;
 
   void Dump(Stream *s) const override;
 
   static const ProgressEventData *GetEventDataFromEvent(const Event *event_ptr);
   uint64_t GetID() const { return m_id; }
+  bool IsFinite() const { return m_total != UINT64_MAX; }
   uint64_t GetCompleted() const { return m_completed; }
   uint64_t GetTotal() const { return m_total; }
   const std::string &GetMessage() const { return m_message; }
   bool IsDebuggerSpecific() const { return m_debugger_specific; }
 
 private:
   std::string m_message;
   const uint64_t m_id;

lldb/source/Core/Debugger.cpp

void Debugger::HandleProgressEvent(const lldb::EventSP &event_sp) {
 
   if (data->GetCompleted()) {
     // Clear the current line.
     output->Printf("\x1B[2K");
     output->Flush();
     return;
   }
 
+  // Trim the progress message if it exceeds the window's width and print it.
+  std::string message = data->GetMessage();
+  if (data->IsFinite())
+    message = llvm::formatv("[{0}/{1}] {2}", data->GetCompleted(),
+                            data->GetTotal(), message)
+                  .str();
+
+  // Trim the progress message if it exceeds the window's width and print it.
+  const uint32_t term_width = GetTerminalWidth();
+  const uint32_t ellipsis = 3;
+  if (message.size() + ellipsis >= term_width)
+    message = message.substr(0, term_width - ellipsis);
+
   const bool use_color = GetUseColor();
   llvm::StringRef ansi_prefix = GetShowProgressAnsiPrefix();
   if (!ansi_prefix.empty())
     output->Printf(
         "%s", ansi::FormatAnsiTerminalCodes(ansi_prefix, use_color).c_str());
 
-  // Print the progress message.
-  std::string message = data->GetMessage();
-  if (data->GetTotal() != UINT64_MAX) {
-    output->Printf("[%" PRIu64 "/%" PRIu64 "] %s...", data->GetCompleted(),
-                   data->GetTotal(), message.c_str());
-  } else {
-    output->Printf("%s...", message.c_str());
-  }
 
   llvm::StringRef ansi_suffix = GetShowProgressAnsiSuffix();
   if (!ansi_suffix.empty())
     output->Printf(
         "%s", ansi::FormatAnsiTerminalCodes(ansi_suffix, use_color).c_str());
 
   // Clear until the end of the line.
   output->Printf("\x1B[K\r");
 
   // Flush the output.
   output->Flush();
 }
 
 void Debugger::HandleDiagnosticEvent(const lldb::EventSP &event_sp) {
   auto *data = DiagnosticEventData::GetEventDataFromEvent(event_sp.get());
   if (!data)
     return;
 
   StreamSP stream = GetAsyncErrorStream();
   data->Dump(stream.get());

lldb/test/API/functionalities/progress_reporting/TestTrimmedProgressReporting.py

+"""
+Test trimming long progress report in tiny terminal windows
+"""
+
+import os
+import pexpect
+import tempfile
+import re
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.lldbpexpect import PExpectTest
+
+class TestTrimmedProgressReporting(PExpectTest):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def do_test(self, term_width, pattern_list):
+        self.build()
+        # Start with a small window
+        self.launch(use_colors=True)
+        self.expect("set set show-progress true")
+        self.expect("set show show-progress", substrs=["show-progress (boolean) = true"])
+        self.expect("set set term-width " + str(term_width))
+        self.expect("set show term-width", substrs=["term-width (int) = " + str(term_width)])
+
+        self.child.send("file " + self.getBuildArtifact("a.out") + "\n")
+        self.child.expect(pattern_list)
+
+
+    # PExpect uses many timeouts internally and doesn't play well
+    # under ASAN on a loaded machine..
+    @skipIfAsan
+    @skipUnlessDarwin
+    @skipIfEditlineSupportMissing
+    def test_trimmed_progress_message(self):
+        self.do_test(19, ['Locating externa...',
+                          'Loading Apple DW...',
+                          'Parsing symbol t...'])
+
+    # PExpect uses many timeouts internally and doesn't play well
+    # under ASAN on a loaded machine..
+    @skipIfAsan
+    @skipUnlessDarwin
+    @skipIfEditlineSupportMissing
+    def test_long_progress_message(self):
+        self.do_test(80, ['Locating external symbol file for a.out...',
+                          'Loading Apple DWARF index for a.out...',
+                          'Parsing symbol table for dyld...'])