[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

468

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

16–18

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

16–18

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

16–18

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.

50–51

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

[mgorny]

lldb/include/lldb/Host/Terminal.h

50–51

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

50–51

implicit is fine

115

Maybe call the class Data as well?

lldb/source/Host/common/Terminal.cpp

87

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

87

Unfortunately, no. Debugger still manually saves/restores.

lldb/include/lldb/Core/Debugger.h

protected:
   repro::DataRecorder *m_input_recorder;
 
   lldb::BroadcasterManagerSP m_broadcaster_manager_sp; // The debugger acts as a
                                                        // broadcaster manager of
                                                        // last resort.
   // It needs to get constructed before the target_list or any other member
   // that might want to broadcast through the debugger.
 
-  TerminalState m_terminal_state;
+  Terminal m_terminal;
   TargetList m_target_list;
 
   PlatformList m_platform_list;
   lldb::ListenerSP m_listener_sp;
   std::unique_ptr<SourceManager> m_source_manager_up; // This is a scratch
                                                       // source manager that we
                                                       // return if we have no
                                                       // targets.

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;
+#if LLDB_ENABLE_TERMIOS
+#include <termios.h>
+#endif
 
 namespace lldb_private {
 
+/// \class Terminal Terminal.h "lldb/Host/Terminal.h"
+/// A terminal wrapper with state saving support.
+///
+/// This class provides a portable wrapper for low-level terminal properties.
+/// It can also be used to store the initial terminal state and restore it
+/// later.
 class Terminal {
 public:
   Terminal(int fd = -1) : m_fd(fd) {}
 
   ~Terminal() = default;
 
   bool IsATerminal() const;
 
   int GetFileDescriptor() const { return m_fd; }
 
-  void SetFileDescriptor(int fd) { m_fd = fd; }
+  void SetFileDescriptor(int fd) {
+    m_fd = fd;
+    ClearSavedState();
+  }
 
   bool FileDescriptorIsValid() const { return m_fd != -1; }
 
-  void Clear() { m_fd = -1; }
+  void Clear() { SetFileDescriptor(-1); }
 
   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.
-///
-/// 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 {
-public:
-  /// Default constructor
-  TerminalState();
-
-  /// Destructor
-  ~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.
+  /// Save the TTY state for the terminal.
   ///
-  /// \param[in] fd
-  ///     The file descriptor to save the state of.
+  /// Save the current state of the current TTY and if "save_process_group"
+  /// is true, attempt to save the process group info for the TTY.
   ///
   /// \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
+  ///     Returns \b true if the 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 SaveState(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).
+  /// call to TerminalState::SaveState(bool).
   ///
   /// \return
   ///     Returns \b true if the TTY state was successfully restored,
   ///     \b false otherwise.
-  bool Restore() const;
+  bool RestoreState() const;
 
   /// Test for valid cached TTY state information.
   ///
   /// \return
   ///     Returns \b true if this object has valid saved TTY state
   ///     settings that can be used to restore a previous state,
   ///     \b false otherwise.
-  bool IsValid() const;
+  bool IsStateValid() const;
 
-  void Clear();
+  void ClearSavedState();
 
 protected:
   /// Test if tflags is valid.
   ///
   /// \return
   ///     Returns \b true if \a m_tflags is valid and can be restored,
   ///     \b false otherwise.
   bool TFlagsIsValid() const;
 
   /// Test if ttystate is valid.
   ///
   /// \return
   ///     Returns \b true if \a m_ttystate is valid and can be
   ///     restored, \b false otherwise.
   bool TTYStateIsValid() const;
 
   /// 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_fd;          ///< Current file descriptor
   int m_tflags = -1; ///< Cached tflags information.
 #if LLDB_ENABLE_TERMIOS
   std::unique_ptr<struct termios>
       m_termios_up; ///< Cached terminal state information.
 #endif
   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/Core/Debugger.cpp

 Debugger::Debugger(lldb::LogOutputCallback log_callback, void *baton)
     : UserID(g_unique_id++),
       Properties(std::make_shared<OptionValueProperties>()),
       m_input_file_sp(std::make_shared<NativeFile>(stdin, false)),
       m_output_stream_sp(std::make_shared<StreamFile>(stdout, false)),
       m_error_stream_sp(std::make_shared<StreamFile>(stderr, false)),
       m_input_recorder(nullptr),
       m_broadcaster_manager_sp(BroadcasterManager::MakeBroadcasterManager()),
-      m_terminal_state(), m_target_list(*this), m_platform_list(),
+      m_terminal(), m_target_list(*this), m_platform_list(),
       m_listener_sp(Listener::MakeListener("lldb.Debugger")),
       m_source_manager_up(), m_source_file_cache(),
       m_command_interpreter_up(
           std::make_unique<CommandInterpreter>(*this, false)),
       m_io_handler_stack(), m_instance_name(), m_loaded_plugins(),
       m_event_handler_thread(), m_io_handler_thread(),
       m_sync_broadcaster(nullptr, "lldb.debugger.sync"),
       m_broadcaster(m_broadcaster_manager_sp,
for (TargetSP target_sp : m_target_list.Targets()) {
           process_sp->Finalize();
         target_sp->Destroy();
       }
     }
     m_broadcaster_manager_sp->Clear();
 
     // Close the input file _before_ we close the input read communications
     // class as it does NOT own the input file, our m_input_file does.
-    m_terminal_state.Clear();
+    m_terminal.Clear();
     GetInputFile().Close();
 
     m_command_interpreter_up->Clear();
   });
 }
 
 bool Debugger::GetCloseInputOnEOF() const {
   //    return m_input_comm.GetCloseOnEOF();

 
 void Debugger::SetErrorFile(FileSP file_sp) {
   assert(file_sp && file_sp->IsValid());
   m_error_stream_sp = std::make_shared<StreamFile>(file_sp);
 }
 
 void Debugger::SaveInputTerminalState() {
   int fd = GetInputFile().GetDescriptor();
-  if (fd != File::kInvalidDescriptor)
-    m_terminal_state.Save(fd, true);
+  if (fd != File::kInvalidDescriptor) {
+    m_terminal.SetFileDescriptor(fd);
+    m_terminal.SaveState(true);
+  }
 }
 
-void Debugger::RestoreInputTerminalState() { m_terminal_state.Restore(); }
+void Debugger::RestoreInputTerminalState() { m_terminal.RestoreState(); }
 
 ExecutionContext Debugger::GetSelectedExecutionContext() {
   bool adopt_selected = true;
   ExecutionContextRef exe_ctx_ref(GetSelectedTarget().get(), adopt_selected);
   return ExecutionContext(exe_ctx_ref);
 }
 
 void Debugger::DispatchInputInterrupt() {

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()
-#if LLDB_ENABLE_TERMIOS
-      ,
-      m_termios_up()
-#endif
-{
-}
-
-// Destructor
-TerminalState::~TerminalState() = default;
-
-void TerminalState::Clear() {
-  m_tty.Clear();
+void Terminal::ClearSavedState() {
   m_tflags = -1;
 #if LLDB_ENABLE_TERMIOS
   m_termios_up.reset();
 #endif
   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);
-  if (m_tty.IsATerminal()) {
+bool Terminal::SaveState(bool save_process_group) {
+  if (IsATerminal()) {
 #if LLDB_ENABLE_POSIX
-    m_tflags = ::fcntl(fd, F_GETFL, 0);
+    m_tflags = ::fcntl(m_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());
+    int err = ::tcgetattr(m_fd, m_termios_up.get());
     if (err != 0)
       m_termios_up.reset();
 #endif // #if LLDB_ENABLE_TERMIOS
 #if LLDB_ENABLE_POSIX
     if (save_process_group)
-      m_process_group = ::tcgetpgrp(0);
+      m_process_group = ::tcgetpgrp(m_fd);
     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;
   }
-  return IsValid();
+  return IsStateValid();
 }
 
-// Restore the state of the TTY using the cached values from a previous call to
-// Save().
-bool TerminalState::Restore() const {
+bool Terminal::RestoreState() const {
 #if LLDB_ENABLE_POSIX
-  if (IsValid()) {
-    const int fd = m_tty.GetFileDescriptor();
+  if (IsStateValid()) {
     if (TFlagsIsValid())
-      fcntl(fd, F_SETFL, m_tflags);
+      fcntl(m_fd, F_SETFL, m_tflags);
 
 #if LLDB_ENABLE_TERMIOS
     if (TTYStateIsValid())
-      tcsetattr(fd, TCSANOW, m_termios_up.get());
+      tcsetattr(m_fd, TCSANOW, m_termios_up.get());
 #endif // #if 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);
+      tcsetpgrp(m_fd, m_process_group);
       // Restore the original signal handler.
       signal(SIGTTOU, saved_sigttou_callback);
     }
     return true;
   }
 #endif
   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());
+bool Terminal::IsStateValid() const {
+  return FileDescriptorIsValid() && (TFlagsIsValid() || TTYStateIsValid());
 }
 
-// Returns true if m_tflags is valid
-bool TerminalState::TFlagsIsValid() const { return m_tflags != -1; }
+bool Terminal::TFlagsIsValid() const { return m_tflags != -1; }
 
-// Returns true if m_ttystate is valid
-bool TerminalState::TTYStateIsValid() const {
+bool Terminal::TTYStateIsValid() const {
 #if LLDB_ENABLE_TERMIOS
   return m_termios_up != nullptr;
 #else
   return false;
 #endif
 }
 
-// Returns true if m_process_group is valid
-bool TerminalState::ProcessGroupIsValid() const {
+bool Terminal::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();
 
         if (is_a_tty) {
-          terminal_state.Save(stdin_fd, false);
+          terminal.SaveState(false);
           terminal.SetCanonical(false);
           terminal.SetEcho(true);
         }
 
         ScriptInterpreterPythonImpl::Locker locker(
             m_python,
             ScriptInterpreterPythonImpl::Locker::AcquireLock |
                 ScriptInterpreterPythonImpl::Locker::InitSession |
if (m_python) {
         // 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();
+          terminal.RestoreState();
       }
     }
     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);
+    terminal.SaveState(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();
+    terminal.RestoreState();
   }
 
   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:
     //