[lldb] Restore tty attributes on disconnect

[mgorny]

Summary

When connecting to a teletype, LLDB sets baud rate and switches it to
raw mode. Restore the original attributes on disconnect.

[mgorny]

Make use of Terminal class as well.

[labath]

Why is it important to restore the tty settings? (just asking, I have no clue about how this is supposed to work)

[mgorny]

In D110997#3039540, @labath wrote:

Why is it important to restore the tty settings? (just asking, I have no clue about how this is supposed to work)

Well, maybe my logic is wrong but the idea is that since LLDB overrides serial port parameters, it should restore them to the original state when it's done with the serial port. Roughly the principle that the program cleans up after itself.

[labath]

In D110997#3039575, @mgorny wrote:

In D110997#3039540, @labath wrote:

Why is it important to restore the tty settings? (just asking, I have no clue about how this is supposed to work)

Well, maybe my logic is wrong but the idea is that since LLDB overrides serial port parameters, it should restore them to the original state when it's done with the serial port. Roughly the principle that the program cleans up after itself.

That does not sound unreasonable, though I'm not sure how much is it actually useful. Do you know if its like standard practice when working with serial ports, ttys and stuff?

TTTT, I'm not very happy that we have _any_ tty code inside the ConnectionFileDescriptorPosix class, so I'm not enthusiastic about expanding it. Let's see how the RFC goes first...

[mgorny]

In D110997#3041802, @labath wrote:

That does not sound unreasonable, though I'm not sure how much is it actually useful. Do you know if its like standard practice when working with serial ports, ttys and stuff?

To be honest, I don't really know. GDB doesn't seem to do it. minicom does restore the initial state.

TTTT, I'm not very happy that we have _any_ tty code inside the ConnectionFileDescriptorPosix class, so I'm not enthusiastic about expanding it. Let's see how the RFC goes first...

Do you think we should move it somewhere else? With the parameters being configurable via settings, it will probably make sense to move it into gdb-remote anyway.

[mgorny]

If at all, this will be part of the new serial port API.

lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h

 
 #include <atomic>
 #include <memory>
 #include <mutex>
 
 #include "lldb/lldb-forward.h"
 
 #include "lldb/Host/Pipe.h"
+#include "lldb/Host/Terminal.h"
 #include "lldb/Utility/Connection.h"
 #include "lldb/Utility/IOObject.h"
 #include "lldb/Utility/Predicate.h"
 
 namespace lldb_private {
 
 class Status;
 class Socket;
protected:
 
   Pipe m_pipe;
   std::recursive_mutex m_mutex;
   std::atomic<bool> m_shutting_down; // This marks that we are shutting down so
                                      // if we get woken up from
   // BytesAvailable to disconnect, we won't try to read again.
   bool m_waiting_for_accept = false;
   bool m_child_processes_inherit;
+  TerminalState m_saved_term_state;
 
   std::string m_uri;
 
 private:
   void InitializeSocket(Socket *socket);
 
   ConnectionFileDescriptor(const ConnectionFileDescriptor &) = delete;
   const ConnectionFileDescriptor &
   operator=(const ConnectionFileDescriptor &) = delete;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_HOST_POSIX_CONNECTIONFILEDESCRIPTORPOSIX_H

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

else if ((addr = GetURLAddress(path, FD_SCHEME))) {
       // file:///PATH
       int fd = llvm::sys::RetryAfterSignal(-1, ::open, addr_str.c_str(), O_RDWR);
       if (fd == -1) {
         if (error_ptr)
           error_ptr->SetErrorToErrno();
         return eConnectionStatusError;
       }
 
-      if (::isatty(fd)) {
+      Terminal term{fd};
+      if (term.IsATerminal()) {
+        m_saved_term_state.Save(term, false);
         // Set up serial terminal emulation
         struct termios options;
         ::tcgetattr(fd, &options);
 
         // Set port speed to maximum
         ::cfsetospeed(&options, B115200);
         ::cfsetispeed(&options, B115200);
 
if (m_pipe.CanWrite()) {
                 static_cast<void *>(this));
     }
     locker.lock();
   }
 
   // Prevents reads and writes during shutdown.
   m_shutting_down = true;
 
+  m_saved_term_state.Restore();
+  m_saved_term_state.Clear();
+
   Status error = m_read_sp->Close();
   Status error2 = m_write_sp->Close();
   if (error.Fail() || error2.Fail())
     status = eConnectionStatusError;
   if (error_ptr)
     *error_ptr = error.Fail() ? error : error2;
 
   // Close any pipes we were using for async interrupts

lldb/test/API/functionalities/gdb_remote_client/TestPty.py

 import lldb
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test.decorators import *
 from gdbclientutils import *
 
 
 @skipIfWindows
 class TestPty(GDBRemoteTestBase):
     mydir = TestBase.compute_mydir(__file__)
     server_socket_class = PtyServerSocket
 
+    def setUp(self):
+        super().setUp()
+        import termios
+        self.orig_attr = termios.tcgetattr(self.server._socket._slave)
+
+    def assert_attr_restored(self):
+        import termios
+        self.assertEqual(termios.tcgetattr(self.server._socket._slave),
+                         self.orig_attr)
+
     def test_process_connect_sync(self):
         """Test the process connect command in synchronous mode"""
         try:
             self.dbg.SetAsync(False)
             self.expect("platform select remote-gdb-server",
                         substrs=['Platform: remote-gdb-server', 'Connected: no'])
             self.expect("process connect " + self.server.get_connect_url(),
                         substrs=['Process', 'stopped'])
         finally:
             self.dbg.GetSelectedTarget().GetProcess().Kill()
+        self.assert_attr_restored()
 
     def test_process_connect_async(self):
         """Test the process connect command in asynchronous mode"""
         try:
             self.dbg.SetAsync(True)
             self.expect("platform select remote-gdb-server",
                         substrs=['Platform: remote-gdb-server', 'Connected: no'])
             self.expect("process connect " + self.server.get_connect_url(),
                         matching=False,
                         substrs=['Process', 'stopped'])
             lldbutil.expect_state_changes(self, self.dbg.GetListener(),
                                           self.process(), [lldb.eStateStopped])
         finally:
             self.dbg.GetSelectedTarget().GetProcess().Kill()
         lldbutil.expect_state_changes(self, self.dbg.GetListener(),
                                       self.process(), [lldb.eStateExited])
+        self.assert_attr_restored()