[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

1873–1875

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

[mib]

lldb/source/Core/Debugger.cpp

1873–1875

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

1873–1875

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

25–34

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) {
   }
 
   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.
+  // Trim the progress message if it exceeds the window's width and print it.
   std::string message = data->GetMessage();
-  if (data->GetTotal() != UINT64_MAX) {
+  uint64_t progress_total = data->GetTotal();
+  uint32_t term_width = GetTerminalWidth();
+
+  size_t prefix_width = 0;
+  if (data->IsFinite()) {
+    prefix_width += 4; // '[%PRIu64/%PRIu64] %s'
+    prefix_width += std::to_string(progress_total).size() * 2;
+  }
+
+  const size_t suffix_width = 3; // %s...
+
+  if (message.size() + prefix_width + suffix_width >= term_width)
+    message.erase(message.begin() + term_width - prefix_width - suffix_width,
+                  message.end());
+
+  if (data->IsFinite()) {
     output->Printf("[%" PRIu64 "/%" PRIu64 "] %s...", data->GetCompleted(),
-                   data->GetTotal(), message.c_str());
+                   progress_total, 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());

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

+"""
+Test resizing in our IOHandlers.
+"""
+
+import os
+import pexpect
+
+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__)
+
+    # PExpect uses many timeouts internally and doesn't play well
+    # under ASAN on a loaded machine..
+    @skipIfAsan
+    @skipIfEditlineSupportMissing
+    @skipIf(oslist=["linux"], archs=["arm", "aarch64"])
+    def test_resize(self):
+        self.build()
+        # Start with a small window
+        self.launch(executable=self.getBuildArtifact("a.out"),
+                    dimensions=(5,5), timeout=1)
+
+        # Now resize to something bigger
+        self.child.setwinsize(100,500)
+
+        patterns = ['Locating external symbol file for',
+                    'Parsing symbol table for', pexpect.EOF, pexpect.TIMEOUT]
+
+        match_index = self.child.expect(patterns)
+
+        self.quit()
+
+        self.assertTrue(match_index, len(patterns) - 1) # match TIMEOUT