[lldb/Driver] Support terminal resizing

[friss]

Summary

The comment in the Editine.h header made it sound like editline was
just unable to handle terminal resizing. We were not ever telling
editline that the terminal had changed size, which might explain why
it wasn't working.

This patch threads a TerminalSizeChanged() callback through the
IOHandler and invokes it from the SIGWINCH handler in the driver. Our
Editline class already had a TerminalSizeChanged() method which
was invoked only when editline was configured.

This patch also changes Editline to not apply the changes right away
in TerminalSizeChanged(), but instead defer that to the next
character read. During my testing, it happened once that the signal
was received while our ConnectionFileDescriptor::Read was allocating
memory. As el_resize seems to allocate memory too, this crashed.

[labath]

Yes, resizing the window (or doing any other nontrivial task) from within a signal handler is a bad idea. Making a note of the signal and then bailing out is the right approach. Though, to be fully strictly correct, the variable ought to be a volatile std::sig_atomic_t instead of a plain bool. This still won't make the whole thing async-signal-safe (I haven't inspected the whole SIGWINCH call stack, but I am sure there are still some unsafe operations there), but it's a step towards that.

If we wanted to avoid delaying the change to the next keystroke, we could reuse the same mechanism that ^C/SIGINT uses (m_input_connection.InterruptRead()). That would probably require introduction of a new EditorStatus enum, and a careful modification to the code handling the input interruption. I don't expect that to be _too_ hard, but I also don't think that's required for this change.

[friss]

Use sig_atomic_t as Pavel suggested

[friss]

In D79654#2029131, @labath wrote:

Yes, resizing the window (or doing any other nontrivial task) from within a signal handler is a bad idea. Making a note of the signal and then bailing out is the right approach. Though, to be fully strictly correct, the variable ought to be a volatile std::sig_atomic_t instead of a plain bool. This still won't make the whole thing async-signal-safe (I haven't inspected the whole SIGWINCH call stack, but I am sure there are still some unsafe operations there), but it's a step towards that.

I updated the patch with this change. I'll commit it tomorrow unless you can explain this:

If we wanted to avoid delaying the change to the next keystroke, we could reuse the same mechanism that ^C/SIGINT uses (m_input_connection.InterruptRead()). That would probably require introduction of a new EditorStatus enum, and a careful modification to the code handling the input interruption. I don't expect that to be _too_ hard, but I also don't think that's required for this change.

I gave this a try, and indeed the changes do not seem too involved, but there's a bunch of details I don't get. Editline::Interrupt() and Editline::Cancel() take m_output_mutex before interrupting. If I do a similar thing in a resize handler, I don't see what prevents the signal to be delivered while the lock is held by GetLines but before it is released in GetCharacter.

[friss]

In D79654#2030635, @friss wrote:

If we wanted to avoid delaying the change to the next keystroke, we could reuse the same mechanism that ^C/SIGINT uses (m_input_connection.InterruptRead()). That would probably require introduction of a new EditorStatus enum, and a careful modification to the code handling the input interruption. I don't expect that to be _too_ hard, but I also don't think that's required for this change.

I gave this a try, and indeed the changes do not seem too involved, but there's a bunch of details I don't get. Editline::Interrupt() and Editline::Cancel() take m_output_mutex before interrupting. If I do a similar thing in a resize handler, I don't see what prevents the signal to be delivered while the lock is held by GetLines but before it is released in GetCharacter.

Sorry, this really wasn't clear... let's try again with more words: The mutex in Editline::Interrupt() and Editline::Cancel() make it look like there's a need for synchronization (m_output_mutex) before calling m_input_connection.InterruptRead(). However those methods cannot be called on the same thread, or they have a chance to deadlock. I actually don't see how sigint_handler in Driver.cpp is correct in this regard. I suppose we put the terminal in raw mode and thus do not generate SIGINT on Ctrl-C. But it seems like an externally sent SIGINT could deadlock the driver pretty easily.

[labath]

Yes, you're right that the current implementation of Editline::Interrupt is not correct. It's kind of my fault since I added that locking code (in my defense, the code was pretty messy to begin with). The implementation could indeed deadlock if the signal is delivered on the thread which does the editline reading. The reason why this does not happen in practice is probably because the signal is indeed not delievered on the editline thread. Posix leaves this behavior unspecified, but implementations have to actually make a choice, and I believe most do it in such a way that they first try the main thread, and only if that has the signal blocked, they try searching for other victims. The problem could also be reproduced with an well-timed manual SIGINT, if the os supports sending signals to a specific thread (linux does via non-standard tgkill(2), I don't know about the rest).

Right now, I don't really remember whether that locking was solving any specific problem or if it was a case of "well, this code is touching variables that may be accessed concurrently, so I better lock it...". If I had to guess, I would say it's the latter. The fprintf operation, and messing with m_editor_status are indeed not safe to perform without additional synchronization. But as we've established, a mutex is not a good method of synchronization with a signal handler. There are several ways to fix this. The fix will probably won't require touching a lot of code, but it will need to be done with a steady hand. It will likely involve moving things out of the signal handler (fprintf("^C") could easily be done in the main thread), making things std::atomic/std::sig_atomic or blocking signals in some critical sections. Most likely a combination of approaches would be needed.

So, anyway, I think it's fine to either keep the current form of the patch, do it similarly to how SIGINT is handled (knowing that it's not fully posix compliant, and hoping that someone will eventually make both things compliant), or try to make everything posix compliant right now.

lldb/include/lldb/Core/IOHandler.h

public:
   virtual bool GetIsDone() { return m_done; }
 
   Type GetType() const { return m_type; }
 
   virtual void Activate() { m_active = true; }
 
   virtual void Deactivate() { m_active = false; }
 
+  virtual void TerminalSizeChanged() {}
+
   virtual const char *GetPrompt() {
     // Prompt support isn't mandatory
     return nullptr;
   }
 
   virtual bool SetPrompt(llvm::StringRef prompt) {
     // Prompt support isn't mandatory
     return false;
public:
   bool Interrupt() override;
 
   void GotEOF() override;
 
   void Activate() override;
 
   void Deactivate() override;
 
+  void TerminalSizeChanged() override;
+
   ConstString GetControlSequence(char ch) override {
     return m_delegate.IOHandlerGetControlSequence(ch);
   }
 
   const char *GetCommandPrefix() override {
     return m_delegate.IOHandlerGetCommandPrefix();
   }
 

lldb/include/lldb/Host/Editline.h

 // paste the following text, without
 //    the quotes:
 //    "This is a test of the input system missing Hello, World!  Do you
 //    disappear when it gets to a particular length?"
 //    Now press ^A to move to the start and type 3 characters, and you'll see a
 //    good amount of the text will
 //    disappear.  It's still in the buffer, just invisible.
 // b) The prompt printing logic for dealing with ANSI formatting characters is
-// broken, which is why we're
-//    working around it here.
-// c) When resizing the terminal window, if the cursor moves between rows
-// libedit will get confused. d) The incremental search uses escape to cancel
-// input, so it's confused by
+// broken, which is why we're working around it here.
+// c) The incremental search uses escape to cancel input, so it's confused by
 // ANSI sequences starting with escape.
-// e) Emoji support is fairly terrible, presumably it doesn't understand
+// d) Emoji support is fairly terrible, presumably it doesn't understand
 // composed characters?
 
 #ifndef LLDB_HOST_EDITLINE_H
 #define LLDB_HOST_EDITLINE_H
 #if defined(__cplusplus)
 
 #include "lldb/Host/Config.h"
 
 #if LLDB_EDITLINE_USE_WCHAR
 #include <codecvt>
 #endif
 #include <locale>
 #include <sstream>
 #include <vector>
 
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/lldb-private.h"
 
 #if defined(_WIN32)
 #include "lldb/Host/windows/editlinewin.h"
 #elif !defined(__ANDROID__)
 #include <histedit.h>
 #endif
 
+#include <csignal>
 #include <mutex>
 #include <string>
 #include <vector>
 
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Utility/CompletionRequest.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Predicate.h"
public:
   /// form a prompt.
   void SetPrompt(const char *prompt);
 
   /// Sets an alternate string to be used as a prompt for the second line and
   /// beyond in multi-line
   /// editing scenarios.
   void SetContinuationPrompt(const char *continuation_prompt);
 
-  /// Required to update the width of the terminal registered for I/O.  It is
-  /// critical that this
-  /// be correct at all times.
+  /// Call when the terminal size changes
   void TerminalSizeChanged();
 
   /// Returns the prompt established by SetPrompt()
   const char *GetPrompt();
 
   /// Returns the index of the line currently being edited
   uint32_t GetCurrentLine();
 
private:
   unsigned char RevertLineCommand(int ch);
 
   /// Ensures that the current EditLine instance is properly configured for
   /// single or multi-line editing.
   void ConfigureEditor(bool multiline);
 
   bool CompleteCharacter(char ch, EditLineGetCharType &out);
 
+  void ApplyTerminalSizeChange();
+
 private:
 #if LLDB_EDITLINE_USE_WCHAR
   std::wstring_convert<std::codecvt_utf8<wchar_t>> m_utf8conv;
 #endif
   ::EditLine *m_editline = nullptr;
   EditlineHistorySP m_history_sp;
   bool m_in_history = false;
   std::vector<EditLineStringType> m_live_history_lines;
   bool m_multiline_enabled = false;
   std::vector<EditLineStringType> m_input_lines;
   EditorStatus m_editor_status;
   bool m_color_prompts = true;
   int m_terminal_width = 0;
   int m_base_line_number = 0;
   unsigned m_current_line_index = 0;
   int m_current_line_rows = -1;
   int m_revert_cursor_index = 0;
   int m_line_number_digits = 3;
   std::string m_set_prompt;
   std::string m_set_continuation_prompt;
   std::string m_current_prompt;
   bool m_needs_prompt_repaint = false;
+  volatile std::sig_atomic_t m_terminal_size_has_changed = 0;
   std::string m_editor_name;
   FILE *m_input_file;
   FILE *m_output_file;
   FILE *m_error_file;
   ConnectionFileDescriptor m_input_connection;
   IsInputCompleteCallbackType m_is_input_complete_callback = nullptr;
   void *m_is_input_complete_callback_baton = nullptr;
   FixIndentationCallbackType m_fix_indentation_callback = nullptr;

lldb/source/Core/Debugger.cpp

 
 uint32_t Debugger::GetTerminalWidth() const {
   const uint32_t idx = ePropertyTerminalWidth;
   return m_collection_sp->GetPropertyAtIndexAsSInt64(
       nullptr, idx, g_debugger_properties[idx].default_uint_value);
 }
 
 bool Debugger::SetTerminalWidth(uint32_t term_width) {
+  if (auto handler_sp = m_io_handler_stack.Top())
+    handler_sp->TerminalSizeChanged();
+
   const uint32_t idx = ePropertyTerminalWidth;
   return m_collection_sp->SetPropertyAtIndexAsSInt64(nullptr, idx, term_width);
 }
 
 bool Debugger::GetUseExternalEditor() const {
   const uint32_t idx = ePropertyUseExternalEditor;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
       nullptr, idx, g_debugger_properties[idx].default_uint_value != 0);

lldb/source/Core/IOHandler.cpp

void IOHandlerEditline::Activate() {
   m_delegate.IOHandlerActivated(*this, GetIsInteractive());
 }
 
 void IOHandlerEditline::Deactivate() {
   IOHandler::Deactivate();
   m_delegate.IOHandlerDeactivated(*this);
 }
 
+void IOHandlerEditline::TerminalSizeChanged() {
+#if LLDB_ENABLE_LIBEDIT
+  m_editline_up->TerminalSizeChanged();
+#endif
+}
+
 // Split out a line from the buffer, if there is a full one to get.
 static Optional<std::string> SplitLine(std::string &line_buffer) {
   size_t pos = line_buffer.find('\n');
   if (pos == std::string::npos)
     return None;
   std::string line =
       std::string(StringRef(line_buffer.c_str(), pos).rtrim("\n\r"));
   line_buffer = line_buffer.substr(pos + 1);

lldb/source/Host/common/Editline.cpp

if (m_multiline_enabled) {
     m_current_line_rows = new_line_rows;
   }
 
   // Read an actual character
   while (true) {
     lldb::ConnectionStatus status = lldb::eConnectionStatusSuccess;
     char ch = 0;
 
+    if (m_terminal_size_has_changed)
+      ApplyTerminalSizeChange();
+
     // This mutex is locked by our caller (GetLine). Unlock it while we read a
     // character (blocking operation), so we do not hold the mutex
     // indefinitely. This gives a chance for someone to interrupt us. After
     // Read returns, immediately lock the mutex again and check if we were
     // interrupted.
     m_output_mutex.unlock();
     int read_count =
         m_input_connection.Read(&ch, 1, llvm::None, status, nullptr);
if (m_editline) {
     // the call to el_end() since we expect to have multiple editline instances
     // in this program.
     el_set(m_editline, EL_EDITMODE, 0);
     el_end(m_editline);
   }
 
   m_editline =
       el_init(m_editor_name.c_str(), m_input_file, m_output_file, m_error_file);
-  TerminalSizeChanged();
+  ApplyTerminalSizeChange();
 
   if (m_history_sp && m_history_sp->IsValid()) {
     if (!m_history_sp->Load()) {
         fputs("Could not load history file\n.", m_output_file);
     }
     el_wset(m_editline, EL_HIST, history, m_history_sp->GetHistoryPtr());
   }
   el_set(m_editline, EL_CLIENTDATA, this);
void Editline::SetPrompt(const char *prompt) {
   m_set_prompt = prompt == nullptr ? "" : prompt;
 }
 
 void Editline::SetContinuationPrompt(const char *continuation_prompt) {
   m_set_continuation_prompt =
       continuation_prompt == nullptr ? "" : continuation_prompt;
 }
 
-void Editline::TerminalSizeChanged() {
-  if (m_editline != nullptr) {
-    el_resize(m_editline);
-    int columns;
-    // This function is documenting as taking (const char *, void *) for the
-    // vararg part, but in reality in was consuming arguments until the first
-    // null pointer. This was fixed in libedit in April 2019
-    // <http://mail-index.netbsd.org/source-changes/2019/04/26/msg105454.html>,
-    // but we're keeping the workaround until a version with that fix is more
-    // widely available.
-    if (el_get(m_editline, EL_GETTC, "co", &columns, nullptr) == 0) {
-      m_terminal_width = columns;
-      if (m_current_line_rows != -1) {
-        const LineInfoW *info = el_wline(m_editline);
-        int lineLength =
-            (int)((info->lastchar - info->buffer) + GetPromptWidth());
-        m_current_line_rows = (lineLength / columns) + 1;
-      }
-    } else {
-      m_terminal_width = INT_MAX;
-      m_current_line_rows = 1;
-    }
-  }
-}
+void Editline::TerminalSizeChanged() { m_terminal_size_has_changed = 1; }
+
+void Editline::ApplyTerminalSizeChange() {
+  if (!m_editline)
+    return;
+
+  m_terminal_size_has_changed = 0;
 
 const char *Editline::GetPrompt() { return m_set_prompt.c_str(); }
 
 uint32_t Editline::GetCurrentLine() { return m_current_line_index; }
 
 bool Editline::Interrupt() {
   bool result = true;
   std::lock_guard<std::mutex> guard(m_output_mutex);

lldb/test/API/iohandler/resize/TestIOHandlerResize.py

+"""
+Test resizing in our IOHandlers.
+"""
+
+import os
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.lldbpexpect import PExpectTest
+
+class IOHandlerCompletionTest(PExpectTest):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    # PExpect uses many timeouts internally and doesn't play well
+    # under ASAN on a loaded machine..
+    @skipIfAsan
+    @skipIfEditlineSupportMissing
+    def test_resize(self):
+
+        # Start with a small window
+        self.launch(dimensions=(10,10))
+
+        self.child.send("his is a long sentence missing its first letter.")
+
+        # Now resize to something bigger
+        self.child.setwinsize(100,500)
+
+        # Hit "left" 60 times (to go to the beginning of the line) and insert
+        # a character.
+        self.child.send(60 * "\033[D")
+        self.child.send("T")
+
+        self.child.expect_exact("(lldb) This is a long sentence missing its first letter.")
+        self.quit()