[lldb] [Host] Refactor TerminalState

[mgorny]

Summary

Refactor TerminalState to make the code simpler. Move 'struct termios'
to a PImpl-style subclass. Add an RAII interface to automatically store
and restore the state.

[mgorny]

lldb/source/Host/common/Terminal.cpp

124

I think passing 0 here was a mistake. The Restore() method passed fd instead.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

358

This instance didn't seem to be used at all.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h

464–465

I'm wondering if we could use the RAII approach here and restore state (if saved) in the destructor. I'm just not 100% sure if that's what Debugger expects.

[mgorny]

Ok, let's start with smaller refactoring and see where it gets us.

[mgorny]

Accept Terminal arg in place of fd.

[shafik]

lldb/include/lldb/Host/Terminal.h

15–16

I don't think we need this forward declaration anymore.

[mgorny]

Fix tcgetpgrp() call (again). Remove unneeded forward decl.

[mgorny]

lldb/include/lldb/Host/Terminal.h

15–16

You are correct, thanks!

[labath]

Overall, this seems like an improvement, though the class is still quite far from what I would consider an ideal state.

lldb/include/lldb/Host/Terminal.h

15–16

This is kind of a step backwards, as in llvm one tries to not expose system headers (https://llvm.org/docs/SupportLibrary.html#don-t-expose-system-headers). Obviously, lldb is quite far from that.

I think this is actually what the this code whas trying to achieve, though forward-declaring 3rd party entities is also a somewhat questionable practice. Pimpling it is the canonical solution, although it has its (mostly mental) overhead.

47–49

I guess it would be good to delete the copy operations as well.

[mgorny]

lldb/include/lldb/Host/Terminal.h

47–49

This is now implicit via PImpl; or do you prefer explicit?

[mgorny]

Move termios data to PImpl-style subclass.

[mgorny]

Move saving/restoring termios back into TerminalState, make Impl a trivial struct.

[labath]

Is anyone passing save_process_group = true now? If not, I'd really like to delete it, as the implementation is quite scary.

lldb/include/lldb/Host/Terminal.h

47–49

implicit is fine

130

Maybe call the class Data as well?

lldb/source/Host/common/Terminal.cpp

92

Could we make Save private now? Or even inline it into the constructor ?

[mgorny]

In D110721#3035879, @labath wrote:

Is anyone passing save_process_group = true now? If not, I'd really like to delete it, as the implementation is quite scary.

Yes, Debugger::SaveInputTerminalState() does.

lldb/source/Host/common/Terminal.cpp

92

Unfortunately, no. Debugger still manually saves/restores.

lldb/include/lldb/Host/Terminal.h

 //===-- Terminal.h ----------------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_HOST_TERMINAL_H
 #define LLDB_HOST_TERMINAL_H
 #if defined(__cplusplus)
 
 #include "lldb/Host/Config.h"
 #include "lldb/lldb-private.h"
 
-struct termios;
-
 namespace lldb_private {
 
 class Terminal {
 public:
   Terminal(int fd = -1) : m_fd(fd) {}
 
   ~Terminal() = default;
 
   bool IsATerminal() const;
public:
   bool SetEcho(bool enabled);
 
   bool SetCanonical(bool enabled);
 
 protected:
   int m_fd; // This may or may not be a terminal file descriptor
 };
 
-/// \class State Terminal.h "lldb/Host/Terminal.h"
-/// A terminal state saving/restoring class.
+/// \class TerminalState Terminal.h "lldb/Host/Terminal.h"
+/// A RAII-friendly terminal state saving/restoring class.
 ///
 /// This class can be used to remember the terminal state for a file
 /// descriptor and later restore that state as it originally was.
 class TerminalState {
+  class Impl;
+
 public:
-  /// Default constructor
-  TerminalState();
+  /// Construct a new instance and optionally save terminal state.
+  ///
+  /// \param[in] term
+  ///     The Terminal instance holding the file descriptor to save the state
+  ///     of.  If the instance is not associated with a fd, no state will
+  ///     be saved.
+  ///
+  /// \param[in] save_process_group
+  ///     If \b true, save the process group settings, else do not
+  ///     save the process group settings for a TTY.
+  TerminalState(Terminal term = -1, bool save_process_group = false);
 
-  /// Destructor
+  /// Destroy the instance, restoring terminal state if saved.  If restoring
+  /// state is undesirable, the instance needs to be reset before destruction.
   ~TerminalState();
 
   /// Save the TTY state for \a fd.
   ///
   /// Save the current state of the TTY for the file descriptor "fd" and if
   /// "save_process_group" is true, attempt to save the process group info for
   /// the TTY.
   ///
-  /// \param[in] fd
-  ///     The file descriptor to save the state of.
+  /// \param[in] term
+  ///     The Terminal instance holding fd to save.
   ///
   /// \param[in] save_process_group
   ///     If \b true, save the process group settings, else do not
   ///     save the process group settings for a TTY.
   ///
   /// \return
   ///     Returns \b true if \a fd describes a TTY and if the state
   ///     was able to be saved, \b false otherwise.
-  bool Save(int fd, bool save_process_group);
+  bool Save(Terminal term, bool save_process_group);
 
   /// Restore the TTY state to the cached state.
   ///
   /// Restore the state of the TTY using the cached values from a previous
   /// call to TerminalState::Save(int,bool).
   ///
   /// \return
   ///     Returns \b true if the TTY state was successfully restored,
protected:
   /// Test if the process group information is valid.
   ///
   /// \return
   ///     Returns \b true if \a m_process_group is valid and can be
   ///     restored, \b false otherwise.
   bool ProcessGroupIsValid() const;
 
   // Member variables
-  Terminal m_tty; ///< A terminal
-  int m_tflags = -1; ///< Cached tflags information.
-#if LLDB_ENABLE_TERMIOS
-  std::unique_ptr<struct termios>
-      m_termios_up; ///< Cached terminal state information.
-#endif
+  Terminal m_tty;               ///< A terminal
+  int m_tflags = -1;            ///< Cached tflags information.
+  std::unique_ptr<Impl> m_data; ///< Platform-specific implementation.
   lldb::pid_t m_process_group = -1; ///< Cached process group information.
 };
 
 } // namespace lldb_private
 
 #endif // #if defined(__cplusplus)
 #endif // LLDB_HOST_TERMINAL_H

lldb/source/Host/common/Terminal.cpp

if (IsATerminal()) {
         return ::tcsetattr(m_fd, TCSANOW, &fd_termios) == 0;
       }
     }
 #endif // #if LLDB_ENABLE_TERMIOS
   }
   return false;
 }
 
-// Default constructor
-TerminalState::TerminalState()
-    : m_tty()
+struct TerminalState::Impl {
 #if LLDB_ENABLE_TERMIOS
-      ,
-      m_termios_up()
+  struct termios m_termios; ///< Cached terminal state information.
 #endif
-{
+};
+
+TerminalState::TerminalState(Terminal term, bool save_process_group)
+    : m_tty(term) {
+  Save(term, save_process_group);
 }
 
-// Destructor
-TerminalState::~TerminalState() = default;
+TerminalState::~TerminalState() { Restore(); }
 
 void TerminalState::Clear() {
   m_tty.Clear();
   m_tflags = -1;
-#if LLDB_ENABLE_TERMIOS
-  m_termios_up.reset();
-#endif
+  m_data.reset();
   m_process_group = -1;
 }
 
-// Save the current state of the TTY for the file descriptor "fd" and if
-// "save_process_group" is true, attempt to save the process group info for the
-// TTY.
-bool TerminalState::Save(int fd, bool save_process_group) {
-  m_tty.SetFileDescriptor(fd);
+bool TerminalState::Save(Terminal term, bool save_process_group) {
+  Clear();
+  m_tty = term;
   if (m_tty.IsATerminal()) {
+    int fd = m_tty.GetFileDescriptor();
 #if LLDB_ENABLE_POSIX
     m_tflags = ::fcntl(fd, F_GETFL, 0);
-#endif
 #if LLDB_ENABLE_TERMIOS
-    if (m_termios_up == nullptr)
-      m_termios_up.reset(new struct termios);
-    int err = ::tcgetattr(fd, m_termios_up.get());
-    if (err != 0)
-      m_termios_up.reset();
-#endif // #if LLDB_ENABLE_TERMIOS
-#if LLDB_ENABLE_POSIX
+    std::unique_ptr<Impl> new_data{new Impl()};
+    if (::tcgetattr(fd, &new_data->m_termios) != 0)
+      m_data = std::move(new_data);
+#endif // LLDB_ENABLE_TERMIOS
     if (save_process_group)
-      m_process_group = ::tcgetpgrp(0);
-    else
-      m_process_group = -1;
-#endif
-  } else {
-    m_tty.Clear();
-    m_tflags = -1;
-#if LLDB_ENABLE_TERMIOS
-    m_termios_up.reset();
-#endif
-    m_process_group = -1;
+      m_process_group = ::tcgetpgrp(fd);
+#endif // LLDB_ENABLE_POSIX
   }
   return IsValid();
 }
 
-// Restore the state of the TTY using the cached values from a previous call to
-// Save().
 bool TerminalState::Restore() const {
 #if LLDB_ENABLE_POSIX
   if (IsValid()) {
     const int fd = m_tty.GetFileDescriptor();
     if (TFlagsIsValid())
       fcntl(fd, F_SETFL, m_tflags);
 
 #if LLDB_ENABLE_TERMIOS
     if (TTYStateIsValid())
-      tcsetattr(fd, TCSANOW, m_termios_up.get());
-#endif // #if LLDB_ENABLE_TERMIOS
+      tcsetattr(fd, TCSANOW, &m_data->m_termios);
+#endif // LLDB_ENABLE_TERMIOS
 
     if (ProcessGroupIsValid()) {
       // Save the original signal handler.
       void (*saved_sigttou_callback)(int) = nullptr;
       saved_sigttou_callback = (void (*)(int))signal(SIGTTOU, SIG_IGN);
       // Set the process group
       tcsetpgrp(fd, m_process_group);
       // Restore the original signal handler.
       signal(SIGTTOU, saved_sigttou_callback);
     }
     return true;
   }
-#endif
+#endif // LLDB_ENABLE_POSIX
   return false;
 }
 
-// Returns true if this object has valid saved TTY state settings that can be
-// used to restore a previous state.
 bool TerminalState::IsValid() const {
   return m_tty.FileDescriptorIsValid() &&
-         (TFlagsIsValid() || TTYStateIsValid());
+         (TFlagsIsValid() || TTYStateIsValid() || ProcessGroupIsValid());
 }
 
-// Returns true if m_tflags is valid
 bool TerminalState::TFlagsIsValid() const { return m_tflags != -1; }
 
-// Returns true if m_ttystate is valid
-bool TerminalState::TTYStateIsValid() const {
-#if LLDB_ENABLE_TERMIOS
-  return m_termios_up != nullptr;
-#else
-  return false;
-#endif
-}
+bool TerminalState::TTYStateIsValid() const { return bool(m_data); }
 
-// Returns true if m_process_group is valid
 bool TerminalState::ProcessGroupIsValid() const {
   return static_cast<int32_t>(m_process_group) != -1;
 }

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

if (PyEval_ThreadsInitialized()) {
                 m_gil_state == PyGILState_UNLOCKED ? "un" : "");
       return;
     }
 
     // InitThreads acquires the GIL if it hasn't been called before.
     PyEval_InitThreads();
   }
 
-  TerminalState m_stdin_tty_state;
   PyGILState_STATE m_gil_state = PyGILState_UNLOCKED;
   bool m_was_already_initialized = false;
 };
 } // namespace
 
 void ScriptInterpreterPython::ComputePythonDirForApple(
     llvm::SmallVectorImpl<char> &path) {
   auto style = llvm::sys::path::Style::posix;

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h

ConstString GetControlSequence(char ch) override {
     return ConstString();
   }
 
   void Run() override {
     if (m_python) {
       int stdin_fd = GetInputFD();
       if (stdin_fd >= 0) {
         Terminal terminal(stdin_fd);
-        TerminalState terminal_state;
-        const bool is_a_tty = terminal.IsATerminal();
+        TerminalState terminal_state(terminal);
 
-        if (is_a_tty) {
-          terminal_state.Save(stdin_fd, false);
+        if (terminal.IsATerminal()) {
           terminal.SetCanonical(false);
           terminal.SetEcho(true);
         }
 
         ScriptInterpreterPythonImpl::Locker locker(
             m_python,
             ScriptInterpreterPythonImpl::Locker::AcquireLock |
                 ScriptInterpreterPythonImpl::Locker::InitSession |
if (m_python) {
         // above). This is because Python has a global lock which must be held
         // whenever we want to touch any Python objects. Otherwise, if the user
         // calls Python code, the interpreter state will be off, and things
         // could hang (it's happened before).
 
         StreamString run_string;
         run_string.Printf("run_python_interpreter (%s)",
                           m_python->GetDictionaryName());
         PyRun_SimpleString(run_string.GetData());
-
-        if (is_a_tty)
-          terminal_state.Restore();
       }
     }
     SetIsDone(true);
   }
 
   void Cancel() override {}
 
   bool Interrupt() override { return m_python->Interrupt(); }
 

lldb/source/Target/Process.cpp

void Run() override {
     if (!m_read_file.IsValid() || !m_write_file.IsValid() ||
         !m_pipe.CanRead() || !m_pipe.CanWrite()) {
       SetIsDone(true);
       return;
     }
 
     SetIsDone(false);
     const int read_fd = m_read_file.GetDescriptor();
-    TerminalState terminal_state;
-    terminal_state.Save(read_fd, false);
     Terminal terminal(read_fd);
+    TerminalState terminal_state(terminal, false);
     terminal.SetCanonical(false);
     terminal.SetEcho(false);
 // FD_ZERO, FD_SET are not supported on windows
 #ifndef _WIN32
     const int pipe_read_fd = m_pipe.GetReadFileDescriptor();
     m_is_running = true;
     while (!GetIsDone()) {
       SelectHelper select_helper;
while (!GetIsDone()) {
               break;
             }
           }
         }
       }
     }
     m_is_running = false;
 #endif
-    terminal_state.Restore();
   }
 
   void Cancel() override {
     SetIsDone(true);
     // Only write to our pipe to cancel if we are in
     // IOHandlerProcessSTDIO::Run(). We can end up with a python command that
     // is being run from the command interpreter:
     //