[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 ↗

(On Diff #375897)

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

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

468 ↗

(On Diff #375897)

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

20

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

20

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

20

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.

53

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

[mgorny]

lldb/include/lldb/Host/Terminal.h

53

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

53

implicit is fine

123

Maybe call the class Data as well?

lldb/source/Host/common/Terminal.cpp

83

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

83

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"
 
+#if LLDB_ENABLE_TERMIOS
+#include <termios.h>
+#endif
+
 struct termios;
 
 namespace lldb_private {
 
 class Terminal {
 public:
   Terminal(int fd = -1) : m_fd(fd) {}
 
   ~Terminal() = default;
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.
   ///
   /// \param[in] fd
   ///     The file descriptor to save the state of.
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.
+  Terminal m_tty;               ///< A terminal
+  int m_tflags = -1;            ///< Cached tflags information.
+  bool m_termios_saved = false; ///< Indication whether termios was saved.
 #if LLDB_ENABLE_TERMIOS
-  std::unique_ptr<struct termios>
-      m_termios_up; ///< Cached terminal state information.
+  struct termios m_termios; ///< 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/Host/common/Terminal.cpp

 
 #include "lldb/Host/Config.h"
 #include "lldb/Host/PosixApi.h"
 #include "llvm/ADT/STLExtras.h"
 
 #include <csignal>
 #include <fcntl.h>
 
-#if LLDB_ENABLE_TERMIOS
-#include <termios.h>
-#endif
-
 using namespace lldb_private;
 
 bool Terminal::IsATerminal() const { return m_fd >= 0 && ::isatty(m_fd); }
 
 bool Terminal::SetEcho(bool enabled) {
   if (FileDescriptorIsValid()) {
 #if LLDB_ENABLE_TERMIOS
     if (IsATerminal()) {
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();
   m_tflags = -1;
-#if LLDB_ENABLE_TERMIOS
-  m_termios_up.reset();
-#endif
+  m_termios_saved = false;
   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) {
+  Clear();
   m_tty.SetFileDescriptor(fd);
   if (m_tty.IsATerminal()) {
 #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
+    if (::tcgetattr(fd, &m_termios) == 0)
+      m_termios_saved = true;
+#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;
+#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_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());
 }
 
 // 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 m_termios_saved; }
 
 // Returns true if m_process_group is valid
 bool TerminalState::ProcessGroupIsValid() const {
   return static_cast<int32_t>(m_process_group) != -1;
 }