[lldb] [Host] Make Terminal methods return llvm::Error

[mgorny]

Move common code and error handling to GetData() and SetData() methods.

[mgorny]

lldb/source/Host/common/Terminal.cpp

75

My gut feeling is telling me that this can be simplified somehow… or we could assume tcsetattr() is cheap and just do the change unconditionally.

[labath]

lldb/include/lldb/Host/Terminal.h

23

Move this into the protected section

lldb/source/Host/common/Terminal.cpp

75

yeah, I think it'd be fine to call tcsetattr unconditionally

[mgorny]

lldb/include/lldb/Host/Terminal.h

23

If I do that, I can't use it from TerminalState.

[mgorny]

Simplify the internal setting logic.

[labath]

lldb/include/lldb/Host/Terminal.h

23

I see. I was expecting that TerminalState was a friend or something. Given the new setup I think it would make sense to make it a friend so that it can use Get/SetData instead of raw tc[gs]etattr calls (and hopefully avoid some ifdefs).

[mgorny]

Make Terminal::Data private and befriend TerminalState.

[thakis]

This causes lots of warnings on Windows, where TERMIOS is false: http://45.33.8.238/win/47744/step_4.txt (look for "Terminal.cpp"). Could you take a look? It looks like return statements are genuinely missing there.

[mgorny]

In D111890#3090632, @thakis wrote:

This causes lots of warnings on Windows, where TERMIOS is false: http://45.33.8.238/win/47744/step_4.txt (look for "Terminal.cpp"). Could you take a look? It looks like return statements are genuinely missing there.

When LLDB_ENABLE_TERMIOS is false, GetData() always returns an error, so the code below should not be reachable. I suppose I could add llvm_unreachable() for that. I'll try that and make a diff if it works.

[thakis]

In D111890#3090653, @mgorny wrote:

In D111890#3090632, @thakis wrote:

This causes lots of warnings on Windows, where TERMIOS is false: http://45.33.8.238/win/47744/step_4.txt (look for "Terminal.cpp"). Could you take a look? It looks like return statements are genuinely missing there.

When LLDB_ENABLE_TERMIOS is false, GetData() always returns an error, so the code below should not be reachable. I suppose I could add llvm_unreachable() for that. I'll try that and make a diff if it works.

Maybe it'd be easier to read if you move

return llvm::createStringError(llvm::inconvertibleErrorCode(),
                               "termios support missing in LLDB");

into some function and then do

llvm::Error Terminal::SetCanonical(bool enabled) {
#if LLDB_ENABLE_TERMIOS
  ...actual body...
#else
  return ThatErrorFunction();
#endif
``

(etc) in all the functions instead of hiding it in GetData()? Then it's obvious in every function what's going on.

[mgorny]

Thanks, that sounds like a good idea. Changed D112632 to do that.

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"
+#include "llvm/Support/Error.h"
 
 namespace lldb_private {
 
+class TerminalState;
+
 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; }
 
   bool FileDescriptorIsValid() const { return m_fd != -1; }
 
   void Clear() { m_fd = -1; }
 
-  bool SetEcho(bool enabled);
+  llvm::Error SetEcho(bool enabled);
 
-  bool SetCanonical(bool enabled);
+  llvm::Error SetCanonical(bool enabled);
 
 protected:
+  struct Data;
+
   int m_fd; // This may or may not be a terminal file descriptor
+
+  llvm::Expected<Data> GetData();
+  llvm::Error SetData(const Data &data);
+
+  friend class TerminalState;
 };
 
 /// \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 {
-  struct Data;
-
 public:
   /// 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.
   ///
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.
-  std::unique_ptr<Data> m_data; ///< Platform-specific implementation.
-  lldb::pid_t m_process_group = -1; ///< Cached process group information.
+  Terminal m_tty;                         ///< A terminal
+  int m_tflags = -1;                      ///< Cached tflags information.
+  std::unique_ptr<Terminal::Data> 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

 #include <fcntl.h>
 
 #if LLDB_ENABLE_TERMIOS
 #include <termios.h>
 #endif
 
 using namespace lldb_private;
 
+struct Terminal::Data {
+#if LLDB_ENABLE_TERMIOS
+  struct termios m_termios; ///< Cached terminal state information.
+#endif
+};
+
 bool Terminal::IsATerminal() const { return m_fd >= 0 && ::isatty(m_fd); }
 
-bool Terminal::SetEcho(bool enabled) {
-  if (FileDescriptorIsValid()) {
-#if LLDB_ENABLE_TERMIOS
-    if (IsATerminal()) {
-      struct termios fd_termios;
-      if (::tcgetattr(m_fd, &fd_termios) == 0) {
-        bool set_corectly = false;
-        if (enabled) {
-          if (fd_termios.c_lflag & ECHO)
-            set_corectly = true;
-          else
-            fd_termios.c_lflag |= ECHO;
-        } else {
-          if (fd_termios.c_lflag & ECHO)
-            fd_termios.c_lflag &= ~ECHO;
-          else
-            set_corectly = true;
-        }
+llvm::Expected<Terminal::Data> Terminal::GetData() {
+  if (!FileDescriptorIsValid())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "invalid fd");
+
+#if LLDB_ENABLE_TERMIOS
+  if (!IsATerminal())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "fd not a terminal");
+
+  Data data;
+  if (::tcgetattr(m_fd, &data.m_termios) != 0)
+    return llvm::createStringError(
+        std::error_code(errno, std::generic_category()),
+        "unable to get teletype attributes");
+  return data;
+#else // !LLDB_ENABLE_TERMIOS
+  return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                 "termios support missing in LLDB");
+#endif // LLDB_ENABLE_TERMIOS
 
-        if (set_corectly)
-          return true;
-        return ::tcsetattr(m_fd, TCSANOW, &fd_termios) == 0;
-      }
-    }
-#endif // #if LLDB_ENABLE_TERMIOS
-  }
-  return false;
+llvm::Error Terminal::SetData(const Terminal::Data &data) {
+#if LLDB_ENABLE_TERMIOS
+  assert(FileDescriptorIsValid());
+  assert(IsATerminal());
+
+  if (::tcsetattr(m_fd, TCSANOW, &data.m_termios) != 0)
+    return llvm::createStringError(
+        std::error_code(errno, std::generic_category()),
+        "unable to set teletype attributes");
+  return llvm::Error::success();
+#else // !LLDB_ENABLE_TERMIOS
+  llvm_unreachable("SetData() should not be called if !LLDB_ENABLE_TERMIOS");
+#endif // LLDB_ENABLE_TERMIOS
 }
 
-bool Terminal::SetCanonical(bool enabled) {
-  if (FileDescriptorIsValid()) {
+llvm::Error Terminal::SetEcho(bool enabled) {
+  llvm::Expected<Data> data = GetData();
+  if (!data)
+    return data.takeError();
+
 #if LLDB_ENABLE_TERMIOS
-    if (IsATerminal()) {
-      struct termios fd_termios;
-      if (::tcgetattr(m_fd, &fd_termios) == 0) {
-        bool set_corectly = false;
-        if (enabled) {
-          if (fd_termios.c_lflag & ICANON)
-            set_corectly = true;
-          else
-            fd_termios.c_lflag |= ICANON;
-        } else {
-          if (fd_termios.c_lflag & ICANON)
-            fd_termios.c_lflag &= ~ICANON;
-          else
-            set_corectly = true;
-        }
+  struct termios &fd_termios = data->m_termios;
+  fd_termios.c_lflag &= ~ECHO;
+  if (enabled)
+    fd_termios.c_lflag |= ECHO;
+  return SetData(data.get());
+#endif // LLDB_ENABLE_TERMIOS
 
-        if (set_corectly)
-          return true;
-        return ::tcsetattr(m_fd, TCSANOW, &fd_termios) == 0;
-      }
-    }
-#endif // #if LLDB_ENABLE_TERMIOS
-  }
-  return false;
-}
+llvm::Error Terminal::SetCanonical(bool enabled) {
+  llvm::Expected<Data> data = GetData();
+  if (!data)
+    return data.takeError();
 
-struct TerminalState::Data {
 #if LLDB_ENABLE_TERMIOS
-  struct termios m_termios; ///< Cached terminal state information.
-#endif
-};
+  struct termios &fd_termios = data->m_termios;
+  fd_termios.c_lflag &= ~ICANON;
+  if (enabled)
+    fd_termios.c_lflag |= ICANON;
+  return SetData(data.get());
+#endif // LLDB_ENABLE_TERMIOS
+}
 
 TerminalState::TerminalState(Terminal term, bool save_process_group)
     : m_tty(term) {
   Save(term, save_process_group);
 }
 
 TerminalState::~TerminalState() { Restore(); }
 
 void TerminalState::Clear() {
   m_tty.Clear();
   m_tflags = -1;
   m_data.reset();
   m_process_group = -1;
 }
 
 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);
 #if LLDB_ENABLE_TERMIOS
-    std::unique_ptr<Data> new_data{new Data()};
+    std::unique_ptr<Terminal::Data> new_data{new Terminal::Data()};
     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(fd);
 #endif // LLDB_ENABLE_POSIX
   }
   return IsValid();

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

public:
   void Run() override {
     if (m_python) {
       int stdin_fd = GetInputFD();
       if (stdin_fd >= 0) {
         Terminal terminal(stdin_fd);
         TerminalState terminal_state(terminal);
 
         if (terminal.IsATerminal()) {
-          terminal.SetCanonical(false);
-          terminal.SetEcho(true);
+          // FIXME: error handling?
+          llvm::consumeError(terminal.SetCanonical(false));
+          llvm::consumeError(terminal.SetEcho(true));
         }
 
         ScriptInterpreterPythonImpl::Locker locker(
             m_python,
             ScriptInterpreterPythonImpl::Locker::AcquireLock |
                 ScriptInterpreterPythonImpl::Locker::InitSession |
                 ScriptInterpreterPythonImpl::Locker::InitGlobals,
             ScriptInterpreterPythonImpl::Locker::FreeAcquiredLock |

lldb/source/Target/Process.cpp

if (!m_read_file.IsValid() || !m_write_file.IsValid() ||
       SetIsDone(true);
       return;
     }
 
     SetIsDone(false);
     const int read_fd = m_read_file.GetDescriptor();
     Terminal terminal(read_fd);
     TerminalState terminal_state(terminal, false);
-    terminal.SetCanonical(false);
-    terminal.SetEcho(false);
+    // FIXME: error handling?
+    llvm::consumeError(terminal.SetCanonical(false));
+    llvm::consumeError(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;
       select_helper.FDSetRead(read_fd);
       select_helper.FDSetRead(pipe_read_fd);

lldb/unittests/Host/posix/TerminalTest.cpp

TEST_F(TerminalTest, PipeIsNotATerminal) {
   EXPECT_EQ(pipeterm.IsATerminal(), false);
   close(pipefd[0]);
   close(pipefd[1]);
 }
 
 TEST_F(TerminalTest, SetEcho) {
   struct termios terminfo;
 
-  ASSERT_EQ(m_term.SetEcho(true), true);
+  ASSERT_THAT_ERROR(m_term.SetEcho(true), llvm::Succeeded());
   ASSERT_EQ(tcgetattr(m_fd, &terminfo), 0);
   EXPECT_NE(terminfo.c_lflag & ECHO, 0U);
 
-  ASSERT_EQ(m_term.SetEcho(false), true);
+  ASSERT_THAT_ERROR(m_term.SetEcho(false), llvm::Succeeded());
   ASSERT_EQ(tcgetattr(m_fd, &terminfo), 0);
   EXPECT_EQ(terminfo.c_lflag & ECHO, 0U);
 }
 
 TEST_F(TerminalTest, SetCanonical) {
   struct termios terminfo;
 
-  ASSERT_EQ(m_term.SetCanonical(true), true);
+  ASSERT_THAT_ERROR(m_term.SetCanonical(true), llvm::Succeeded());
   ASSERT_EQ(tcgetattr(m_fd, &terminfo), 0);
   EXPECT_NE(terminfo.c_lflag & ICANON, 0U);
 
-  ASSERT_EQ(m_term.SetCanonical(false), true);
+  ASSERT_THAT_ERROR(m_term.SetCanonical(false), llvm::Succeeded());
   ASSERT_EQ(tcgetattr(m_fd, &terminfo), 0);
   EXPECT_EQ(terminfo.c_lflag & ICANON, 0U);
 }
 
 TEST_F(TerminalTest, SaveRestoreRAII) {
   struct termios orig_terminfo;
   struct termios terminfo;
   ASSERT_EQ(tcgetattr(m_fd, &orig_terminfo), 0);