[lldb] Add serial:// protocol for connecting to serial port

[mgorny]

Summary

Add a new serial:// protocol along with SerialPort that provides a new
API to open serial ports. The URL consists of serial device path
followed by URL-style options, e.g.:

serial:///dev/ttyS0?baud=115200&parity=even

If no options are provided, the serial port is only set to raw mode
and the other attributes remain unchanged. Attributes provided via
options are modified to the specified values. Upon closing the serial
port, its original attributes are restored.

[labath]

This looks reasonable to me. In case the url does not specify port parameters, it might be better to set them to known sane defaults instead of leaving them unchanged. That way lldb and lldb-server could "just connect". However, I don't know much about serial ports so I'm leaving that up to you.

[mgorny]

I don't really have a strong opinion about this. I think we should leave the option of leaving port settings unchanged. I'll check what gdb does, as I suppose interoperability with gdbserver is also useful. I guess people are more likely to connect to other servers than lldb over serial port after all.

[mgorny]

Move serial:// URL parsing to SerialPort class. Improve error handling.

[labath]

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

772–774

It would be better to hide this by making the constructor private and have a static factory function returning Expected<unique_ptr<SerialPort>>. Bonus points if you can move all the fallible operations into the factory function so that the (now private) constructor does not need the Error argument at all

[mgorny]

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

772–774

I know but I couldn't make this work — the compiler just won't find a way to convert the new SerialPort instance into Expected<SerialPort>. I suspect I would have to add move ctors and assignment operators all the way up the class hierarchy.

[labath]

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

772–774

That's where the unique_ptr part comes in. :)

[mgorny]

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

772–774

I presume you mean shared_ptr here, or know some dirty trick.

[mgorny]

Use a fallible ctor approach.

[labath]

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

772–774

a unique_ptr is convertible to a shared_ptr, but not the other way around, so it's better to return the former

[mgorny]

Use unique_ptr.

[mgorny]

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

772–774

Ah, indeed. I was missing std::move.

[labath]

lldb/source/Host/common/File.cpp

847

Empty line here

lldb/include/lldb/Host/File.h

 //===-- File.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_FILE_H
 #define LLDB_HOST_FILE_H
 
 #include "lldb/Host/PosixApi.h"
+#include "lldb/Host/Terminal.h"
 #include "lldb/Utility/IOObject.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/lldb-private.h"
 #include "llvm/ADT/BitmaskEnum.h"
 
 #include <cstdarg>
 #include <cstdio>
 #include <mutex>
protected:
   bool m_own_stream = false;
   std::mutex offset_access_mutex;
 
 private:
   NativeFile(const NativeFile &) = delete;
   const NativeFile &operator=(const NativeFile &) = delete;
 };
 
+class SerialPort : public NativeFile {
+public:
+  struct Options {
+    llvm::Optional<unsigned int> BaudRate = llvm::None;
+    llvm::Optional<Terminal::Parity> Parity = llvm::None;
+    llvm::Optional<unsigned int> StopBits = llvm::None;
+  };
+
+  // Obtain Options corresponding to the passed URL query string
+  // (i.e. the part after '?').
+  static llvm::Expected<Options> OptionsFromURL(llvm::StringRef urlqs);
+
+  static llvm::Expected<std::unique_ptr<SerialPort>>
+  Create(int fd, OpenOptions options, Options serial_options,
+         bool transfer_ownership);
+
+  bool IsValid() const override {
+    return NativeFile::IsValid() && m_is_interactive == eLazyBoolYes;
+  }
+
+  Status Close() override;
+
+  static char ID;
+  virtual bool isA(const void *classID) const override {
+    return classID == &ID || File::isA(classID);
+  }
+  static bool classof(const File *file) { return file->isA(&ID); }
+
+private:
+  SerialPort(int fd, OpenOptions options, Options serial_options,
+             bool transfer_ownership);
+
+  SerialPort(const SerialPort &) = delete;
+  const SerialPort &operator=(const SerialPort &) = delete;
+
+  TerminalState m_state;
+};
+
 } // namespace lldb_private
 
 #endif // LLDB_HOST_FILE_H

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

protected:
 
   lldb::ConnectionStatus UnixAbstractSocketConnect(llvm::StringRef socket_name,
                                                    Status *error_ptr);
 
   lldb::ConnectionStatus ConnectFD(llvm::StringRef args, Status *error_ptr);
 
   lldb::ConnectionStatus ConnectFile(llvm::StringRef args, Status *error_ptr);
 
+  lldb::ConnectionStatus ConnectSerialPort(llvm::StringRef args,
+                                           Status *error_ptr);
+
   lldb::IOObjectSP m_io_sp;
 
   Predicate<uint16_t>
       m_port_predicate; // Used when binding to port zero to wait for the thread
                         // that creates the socket, binds and listens to
                         // resolve the port number.
 
   Pipe m_pipe;

lldb/source/Host/common/File.cpp

mode_t File::ConvertOpenOptionsForPOSIXOpen(OpenOptions open_options) {
   if (open_options & eOpenOptionCanCreateNewOnly)
     mode |= O_CREAT | O_EXCL;
   else if (open_options & eOpenOptionCanCreate)
     mode |= O_CREAT;
 
   return mode;
 }
 
+llvm::Expected<SerialPort::Options>
+SerialPort::OptionsFromURL(llvm::StringRef urlqs) {
+  SerialPort::Options serial_options;
+  for (llvm::StringRef x : llvm::Split(urlqs, '&')) {
+    if (x.consume_front("baud=")) {
+      unsigned int baud_rate;
+      if (!llvm::to_integer(x, baud_rate, 10))
+        return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                       "Invalid baud rate: %s",
+                                       x.str().c_str());
+      serial_options.BaudRate = baud_rate;
+    } else if (x.consume_front("parity=")) {
+      serial_options.Parity =
+          llvm::StringSwitch<llvm::Optional<Terminal::Parity>>(x)
+              .Case("no", Terminal::Parity::No)
+              .Case("even", Terminal::Parity::Even)
+              .Case("odd", Terminal::Parity::Odd)
+              .Case("mark", Terminal::Parity::Mark)
+              .Case("space", Terminal::Parity::Space)
+              .Default(llvm::None);
+      if (!serial_options.Parity)
+        return llvm::createStringError(
+            llvm::inconvertibleErrorCode(),
+            "Invalid parity (must be no, even, odd, mark or space): %s",
+            x.str().c_str());
+    } else if (x.consume_front("stop-bits=")) {
+      unsigned int stop_bits;
+      if (!llvm::to_integer(x, stop_bits, 10) ||
+          (stop_bits != 1 && stop_bits != 2))
+        return llvm::createStringError(
+            llvm::inconvertibleErrorCode(),
+            "Invalid stop bit number (must be 1 or 2): %s", x.str().c_str());
+      serial_options.StopBits = stop_bits;
+    } else
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "Unknown parameter: %s", x.str().c_str());
+  }
+  return serial_options;
+}
+
+llvm::Expected<std::unique_ptr<SerialPort>>
+SerialPort::Create(int fd, OpenOptions options, Options serial_options,
+                   bool transfer_ownership) {
+  std::unique_ptr<SerialPort> out{
+      new SerialPort(fd, options, serial_options, transfer_ownership)};
+
+  if (!out->GetIsInteractive())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "the specified file is not a teletype");
+
+  Terminal term{fd};
+  if (llvm::Error error = term.SetRaw())
+    return error;
+  if (serial_options.BaudRate) {
+    if (llvm::Error error =
+            term.SetBaudRate(serial_options.BaudRate.getValue()))
+      return error;
+  }
+  if (serial_options.Parity) {
+    if (llvm::Error error = term.SetParity(serial_options.Parity.getValue()))
+      return error;
+  }
+  if (serial_options.StopBits) {
+    if (llvm::Error error =
+            term.SetStopBits(serial_options.StopBits.getValue()))
+      return error;
+  }
+
+  return out;
+}
+
+SerialPort::SerialPort(int fd, OpenOptions options,
+                       SerialPort::Options serial_options,
+                       bool transfer_ownership)
+    : NativeFile(fd, options, transfer_ownership), m_state(fd) {}
+
+Status SerialPort::Close() {
+  m_state.Restore();
+  return NativeFile::Close();
+}
+
 char File::ID = 0;
 char NativeFile::ID = 0;
+char SerialPort::ID = 0;

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

auto method =
                    &ConnectionFileDescriptor::ConnectTCP)
             .Case("udp", &ConnectionFileDescriptor::ConnectUDP)
             .Case("unix-connect", &ConnectionFileDescriptor::NamedSocketConnect)
             .Case("unix-abstract-connect",
                   &ConnectionFileDescriptor::UnixAbstractSocketConnect)
 #if LLDB_ENABLE_POSIX
             .Case("fd", &ConnectionFileDescriptor::ConnectFD)
             .Case("file", &ConnectionFileDescriptor::ConnectFile)
+            .Case("serial", &ConnectionFileDescriptor::ConnectSerialPort)
 #endif
             .Default(nullptr);
 
     if (method)
       return (this->*method)(path, error_ptr);
   }
 
   if (error_ptr)
#if LLDB_ENABLE_POSIX
   }
   m_io_sp =
       std::make_shared<NativeFile>(fd, File::eOpenOptionReadWrite, true);
   return eConnectionStatusSuccess;
 #endif // LLDB_ENABLE_POSIX
   llvm_unreachable("this function should be only called w/ LLDB_ENABLE_POSIX");
 }
 
+ConnectionStatus
+ConnectionFileDescriptor::ConnectSerialPort(llvm::StringRef s,
+                                            Status *error_ptr) {
+#if LLDB_ENABLE_POSIX
+  llvm::StringRef path, qs;
+  // serial:///PATH?k1=v1&k2=v2...
+  std::tie(path, qs) = s.split('?');
+
+  llvm::Expected<SerialPort::Options> serial_options =
+      SerialPort::OptionsFromURL(qs);
+  if (!serial_options) {
+    if (error_ptr)
+      *error_ptr = serial_options.takeError();
+    else
+      llvm::consumeError(serial_options.takeError());
+    return eConnectionStatusError;
+  }
+
+  int fd = llvm::sys::RetryAfterSignal(-1, ::open, path.str().c_str(), O_RDWR);
+  if (fd == -1) {
+    if (error_ptr)
+      error_ptr->SetErrorToErrno();
+    return eConnectionStatusError;
+  }
+
+  int flags = ::fcntl(fd, F_GETFL, 0);
+  if (flags >= 0) {
+    if ((flags & O_NONBLOCK) == 0) {
+      flags |= O_NONBLOCK;
+      ::fcntl(fd, F_SETFL, flags);
+    }
+  }
+
+  llvm::Expected<std::unique_ptr<SerialPort>> serial_sp = SerialPort::Create(
+      fd, File::eOpenOptionReadWrite, serial_options.get(), true);
+  if (!serial_sp) {
+    if (error_ptr)
+      *error_ptr = serial_sp.takeError();
+    else
+      llvm::consumeError(serial_sp.takeError());
+    return eConnectionStatusError;
+  }
+  m_io_sp = std::move(serial_sp.get());
+
+  return eConnectionStatusSuccess;
+#endif // LLDB_ENABLE_POSIX
+  llvm_unreachable("this function should be only called w/ LLDB_ENABLE_POSIX");
+}
+
 uint16_t
 ConnectionFileDescriptor::GetListeningPort(const Timeout<std::micro> &timeout) {
   auto Result = m_port_predicate.WaitForValueNotEqualTo(0, timeout);
   return Result ? *Result : 0;
 }
 
 bool ConnectionFileDescriptor::GetChildProcessesInherit() const {
   return m_child_processes_inherit;

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 get_term_attrs(self):
+        import termios
+        return termios.tcgetattr(self.server._socket._slave)
+
+    def setUp(self):
+        super().setUp()
+        self.orig_attr = self.get_term_attrs()
+
+    def assert_raw_mode(self, current_attr):
+        import termios
+        self.assertEqual(current_attr[0] & (termios.BRKINT |
+                                            termios.PARMRK |
+                                            termios.ISTRIP | termios.INLCR |
+                                            termios.IGNCR | termios.ICRNL |
+                                            termios.IXON),
+                         0)
+        self.assertEqual(current_attr[1] & termios.OPOST, 0)
+        self.assertEqual(current_attr[2] & termios.CSIZE, termios.CS8)
+        self.assertEqual(current_attr[3] & (termios.ICANON | termios.ECHO |
+                                            termios.ISIG | termios.IEXTEN),
+                         0)
+        self.assertEqual(current_attr[6][termios.VMIN], 1)
+        self.assertEqual(current_attr[6][termios.VTIME], 0)
+
+    def get_parity_flags(self, attr):
+        import termios
+        return attr[2] & (termios.PARENB | termios.PARODD)
+
+    def get_stop_bit_flags(self, attr):
+        import termios
+        return attr[2] & termios.CSTOPB
+
     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'])
+
+            current_attr = self.get_term_attrs()
+            # serial:// should set raw mode
+            self.assert_raw_mode(current_attr)
+            # other parameters should be unmodified
+            self.assertEqual(current_attr[4:6], self.orig_attr[4:6])
+            self.assertEqual(self.get_parity_flags(current_attr),
+                             self.get_parity_flags(self.orig_attr))
+            self.assertEqual(self.get_stop_bit_flags(current_attr),
+                             self.get_stop_bit_flags(self.orig_attr))
         finally:
             self.dbg.GetSelectedTarget().GetProcess().Kill()
+        # original mode should be restored on exit
+        self.assertEqual(self.get_term_attrs(), self.orig_attr)
 
     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])
+
+            current_attr = self.get_term_attrs()
+            # serial:// should set raw mode
+            self.assert_raw_mode(current_attr)
+            # other parameters should be unmodified
+            self.assertEqual(current_attr[4:6], self.orig_attr[4:6])
+            self.assertEqual(self.get_parity_flags(current_attr),
+                             self.get_parity_flags(self.orig_attr))
+            self.assertEqual(self.get_stop_bit_flags(current_attr),
+                             self.get_stop_bit_flags(self.orig_attr))
         finally:
             self.dbg.GetSelectedTarget().GetProcess().Kill()
         lldbutil.expect_state_changes(self, self.dbg.GetListener(),
                                       self.process(), [lldb.eStateExited])
+        # original mode should be restored on exit
+        self.assertEqual(self.get_term_attrs(), self.orig_attr)
+
+    def test_connect_via_file(self):
+        """Test connecting via the legacy file:// URL"""
+        import termios
+        try:
+            self.expect("platform select remote-gdb-server",
+                        substrs=['Platform: remote-gdb-server', 'Connected: no'])
+            self.expect("process connect file://" +
+                        self.server.get_connect_address(),
+                        substrs=['Process', 'stopped'])
+
+            # file:// sets baud rate and some raw-related flags
+            current_attr = self.get_term_attrs()
+            self.assertEqual(current_attr[3] & (termios.ICANON | termios.ECHO |
+                                                termios.ECHOE | termios.ISIG),
+                             0)
+            self.assertEqual(current_attr[4], termios.B115200)
+            self.assertEqual(current_attr[5], termios.B115200)
+            self.assertEqual(current_attr[6][termios.VMIN], 1)
+            self.assertEqual(current_attr[6][termios.VTIME], 0)
+        finally:
+            self.dbg.GetSelectedTarget().GetProcess().Kill()
+
+    def test_process_connect_params(self):
+        """Test serial:// URL with parameters"""
+        import termios
+        try:
+            self.expect("platform select remote-gdb-server",
+                        substrs=['Platform: remote-gdb-server', 'Connected: no'])
+            self.expect("process connect " + self.server.get_connect_url() +
+                        "?baud=115200&stop-bits=2",
+                        substrs=['Process', 'stopped'])
+
+            current_attr = self.get_term_attrs()
+            self.assert_raw_mode(current_attr)
+            self.assertEqual(current_attr[4:6], 2 * [termios.B115200])
+            self.assertEqual(self.get_parity_flags(current_attr),
+                             self.get_parity_flags(self.orig_attr))
+            self.assertEqual(self.get_stop_bit_flags(current_attr),
+                             termios.CSTOPB)
+        finally:
+            self.dbg.GetSelectedTarget().GetProcess().Kill()
+        # original mode should be restored on exit
+        self.assertEqual(self.get_term_attrs(), self.orig_attr)

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

class PtyServerSocket(ServerSocket):
 
     def get_connect_address(self):
         libc = ctypes.CDLL(None)
         libc.ptsname.argtypes = (ctypes.c_int,)
         libc.ptsname.restype = ctypes.c_char_p
         return libc.ptsname(self._master.fileno()).decode()
 
     def get_connect_url(self):
-        return "file://" + self.get_connect_address()
+        return "serial://" + self.get_connect_address()
 
     def close_server(self):
         self._slave.close()
         self._master.close()
 
     def recv(self):
         try:
             return self._master.read(4096)