[lldb] Add unit tests for Terminal API

[mgorny]

Remove obsolete TODO.

lldb/unittests/Host/CMakeLists.txt

27

FTR, I've tested it on FreeBSD and Linux so far. NetBSD in progress. Darwin and OpenBSD support is based on what I've found in gnulib but explicit testing would be helpful.

[labath]

Have you considered using the PseudoTerminal class to create the ptys ?

lldb/unittests/Host/CMakeLists.txt

27

At this point, I'd just go with NOT windows

[mgorny]

In D110962#3039428, @labath wrote:

Have you considered using the PseudoTerminal class to create the ptys ?

That's really stupid of me but for some reason I've assumed that if LLDB had any pty-related logic, it'd live in Terminal.h ;-).

lldb/unittests/Host/CMakeLists.txt

27

If I'm using PseudoTerminal, then I guess I don't need the platform check at all (and 'not Windows' is implied by LLDB_ENABLE_TERMIOS).

[mgorny]

Use PseudoTerminal class.

[labath]

lldb/unittests/Host/CMakeLists.txt

27

Yep.

Although, I have a feeling that this will actually not work (as in, it will trigger the llvm unrecognized-source-file cmake alarm) when termios is disabled. IIRC, the llvm solution is to pass this file in some "optionally compiled sources" argument. The lldb solution would be to put the file in a subdirectory (posix, I guess).
I'd probably go with the second one, since we already have a linux subfolder.

[mgorny]

lldb/unittests/Host/CMakeLists.txt

27

Sure, done that.

[davezarzycki]

This is failing on my Fedora 34 (x86-64) box. Is this expected?

FAIL: lldb-unit :: Host/./HostTests/TerminalTest.SaveRestoreRAII (78548 of 79402)
******************** TEST 'lldb-unit :: Host/./HostTests/TerminalTest.SaveRestoreRAII' FAILED ********************
Script:
--
/tmp/_update_lc/r/tools/lldb/unittests/Host/./HostTests --gtest_filter=TerminalTest.SaveRestoreRAII
--
Note: Google Test filter = TerminalTest.SaveRestoreRAII
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from TerminalTest
[ RUN      ] TerminalTest.SaveRestoreRAII
/home/dave/ro_s/lp/lldb/unittests/Host/posix/TerminalTest.cpp:93: Failure
Expected equality of these values:
  tcsetattr(m_pty.GetPrimaryFileDescriptor(), 0, &terminfo)
    Which is: -1
  0
[  FAILED  ] TerminalTest.SaveRestoreRAII (4 ms)
[----------] 1 test from TerminalTest (4 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (4 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] TerminalTest.SaveRestoreRAII

 1 FAILED TEST

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
FAIL: lldb-unit :: Host/./HostTests/TerminalTest.SaveRestore (78561 of 79402)
******************** TEST 'lldb-unit :: Host/./HostTests/TerminalTest.SaveRestore' FAILED ********************
Script:
--
/tmp/_update_lc/r/tools/lldb/unittests/Host/./HostTests --gtest_filter=TerminalTest.SaveRestore
--
Note: Google Test filter = TerminalTest.SaveRestore
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from TerminalTest
[ RUN      ] TerminalTest.SaveRestore
/home/dave/ro_s/lp/lldb/unittests/Host/posix/TerminalTest.cpp:121: Failure
Expected equality of these values:
  tcsetattr(m_pty.GetPrimaryFileDescriptor(), 0, &terminfo)
    Which is: -1
  0
[  FAILED  ] TerminalTest.SaveRestore (4 ms)
[----------] 1 test from TerminalTest (4 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (4 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] TerminalTest.SaveRestore

 1 FAILED TEST

[mgorny]

No, it's not. Could you add a quick printf() there to see what the value of errno is?

I need to check if gtest has a better method of checking for failure with errno.

[mgorny]

…and/or try removing lines from the // make some arbitrary changes block to see which one causes the error.

[davezarzycki]

errno == 22 (EINVAL)

The FD in question is 3. Here is the output from lsof: HostTests 80108 dave 3u CHR 5,2 0t0 231 /dev/ptmx

Can we/I please revert this for now?

[mgorny]

In D110962#3042270, @davezarzycki wrote:

errno == 22 (EINVAL)

The FD in question is 3. Here is the output from lsof: HostTests 80108 dave 3u CHR 5,2 0t0 231 /dev/ptmx

I don't think FD is the problem — requests for the same fd don't fail above. I think it's some terminfo flags.

Can we/I please revert this for now?

Feel free to revert if you're going to help me fix it afterwards. It doesn't fail here nor on the buildbot.

[davezarzycki]

"the buildbot"? There are many. I'd be surprised if this weren't failing on at least one of them. It doesn't seem like subtle terminfo behavior is essential to this test. Can we please trim down the adjustments to only changing the speed? For example:

diff --git i/lldb/unittests/Host/posix/TerminalTest.cpp w/lldb/unittests/Host/posix/TerminalTest.cpp
index ecdb5480216439903b9fc12c39b3d47cb62f9134..e865d44bf6cb9085f07c11e06be34f33a7bd32b9 100644
--- i/lldb/unittests/Host/posix/TerminalTest.cpp
+++ w/lldb/unittests/Host/posix/TerminalTest.cpp
@@ -80,14 +80,8 @@ TEST_F(TerminalTest, SaveRestoreRAII) {
     terminfo = orig_terminfo;

     // make some arbitrary changes
-    terminfo.c_iflag ^= IGNPAR | INLCR;
-    terminfo.c_oflag ^= OPOST | OCRNL;
-    terminfo.c_cflag ^= PARENB | PARODD;
-    terminfo.c_lflag ^= ICANON | ECHO;
-    terminfo.c_cc[VEOF] ^= 8;
-    terminfo.c_cc[VEOL] ^= 4;
-    cfsetispeed(&terminfo, B9600);
-    cfsetospeed(&terminfo, B9600);
+    cfsetispeed(&terminfo, cfgetispeed(&orig_terminfo) == B9600 ? B4800 : B9600);
+    cfsetospeed(&terminfo, cfgetospeed(&orig_terminfo) == B9600 ? B4800 : B9600);

     ASSERT_EQ(tcsetattr(m_pty.GetPrimaryFileDescriptor(), TCSANOW, &terminfo),
               0);
@@ -109,14 +103,8 @@ TEST_F(TerminalTest, SaveRestore) {
   terminfo = orig_terminfo;

   // make some arbitrary changes
-  terminfo.c_iflag ^= IGNPAR | INLCR;
-  terminfo.c_oflag ^= OPOST | OCRNL;
-  terminfo.c_cflag ^= PARENB | PARODD;
-  terminfo.c_lflag ^= ICANON | ECHO;
-  terminfo.c_cc[VEOF] ^= 8;
-  terminfo.c_cc[VEOL] ^= 4;
-  cfsetispeed(&terminfo, B9600);
-  cfsetospeed(&terminfo, B9600);
+  cfsetispeed(&terminfo, cfgetispeed(&orig_terminfo) == B9600 ? B4800 : B9600);
+  cfsetospeed(&terminfo, cfgetospeed(&orig_terminfo) == B9600 ? B4800 : B9600);

   ASSERT_EQ(tcsetattr(m_pty.GetPrimaryFileDescriptor(), TCSANOW, &terminfo), 0);

[davezarzycki]

79bf032fe103333546fd1d6e14c5ac8905f25c2b

[mgorny]

I suppose that's good enough.

[davezarzycki]

If you want to test more, please let me know and I can test them on my Fedora box.

lldb/unittests/Host/CMakeLists.txt

 set (FILES
   ConnectionFileDescriptorTest.cpp
   FileActionTest.cpp
   FileSystemTest.cpp
   FileTest.cpp
   HostInfoTest.cpp
   HostTest.cpp
   MainLoopTest.cpp
   NativeProcessProtocolTest.cpp
   PipeTest.cpp
   ProcessLaunchInfoTest.cpp
   SocketAddressTest.cpp
   SocketTest.cpp
   SocketTestUtilities.cpp
   XMLTest.cpp
 )
+set (EXTRA_LIBS)
 
 if (CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
   list(APPEND FILES
     linux/HostTest.cpp
     linux/SupportTest.cpp
   )
 endif()
 
+if (LLDB_ENABLE_TERMIOS AND
+    CMAKE_SYSTEM_NAME MATCHES "Darwin|FreeBSD|Linux|NetBSD|OpenBSD")
+  list(APPEND FILES
+    TerminalTest.cpp
+  )
+
+  if (NOT CMAKE_SYSTEM_NAME MATCHES "Darwin")
+    list(APPEND EXTRA_LIBS
+      util
+    )
+  endif()
+endif()
+
 add_lldb_unittest(HostTests
   ${FILES}
   LINK_LIBS
     lldbHost
     lldbUtilityHelpers
     LLVMTestingSupport
+    ${EXTRA_LIBS}
   )

lldb/unittests/Host/TerminalTest.cpp

+//===-- TerminalTest.cpp --------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Host/Terminal.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#if defined(__linux__)
+#include <pty.h>
+#elif defined(__FreeBSD__)
+#include <libutil.h>
+#else // Darwin, NetBSD, OpenBSD
+#include <util.h>
+#endif
+#include <termios.h>
+#include <unistd.h>
+
+using namespace lldb_private;
+
+class TerminalTest : public ::testing::Test {
+protected:
+  int m_master;
+  int m_slave;
+
+  void SetUp() override {
+    ASSERT_EQ(openpty(&m_master, &m_slave, nullptr, nullptr, nullptr), 0);
+  }
+
+  void TearDown() override {
+    close(m_slave);
+    close(m_master);
+  }
+};
+
+TEST_F(TerminalTest, PtyIsATerminal) {
+  Terminal term{m_slave};
+  EXPECT_EQ(term.IsATerminal(), true);
+}
+
+TEST_F(TerminalTest, PipeIsNotATerminal) {
+  int pipefd[2];
+  ASSERT_EQ(pipe(pipefd), 0);
+  Terminal pipeterm{pipefd[0]};
+  EXPECT_EQ(pipeterm.IsATerminal(), false);
+  close(pipefd[0]);
+  close(pipefd[1]);
+}
+
+TEST_F(TerminalTest, SetEcho) {
+  struct termios terminfo;
+  Terminal term{m_slave};
+
+  ASSERT_EQ(term.SetEcho(true), true);
+  ASSERT_EQ(tcgetattr(m_slave, &terminfo), 0);
+  EXPECT_NE(terminfo.c_lflag & ECHO, 0U);
+
+  ASSERT_EQ(term.SetEcho(false), true);
+  ASSERT_EQ(tcgetattr(m_slave, &terminfo), 0);
+  EXPECT_EQ(terminfo.c_lflag & ECHO, 0U);
+}
+
+TEST_F(TerminalTest, SetCanonical) {
+  struct termios terminfo;
+  Terminal term{m_slave};
+
+  ASSERT_EQ(term.SetCanonical(true), true);
+  ASSERT_EQ(tcgetattr(m_slave, &terminfo), 0);
+  EXPECT_NE(terminfo.c_lflag & ICANON, 0U);
+
+  ASSERT_EQ(term.SetCanonical(false), true);
+  ASSERT_EQ(tcgetattr(m_slave, &terminfo), 0);
+  EXPECT_EQ(terminfo.c_lflag & ICANON, 0U);
+}
+
+TEST_F(TerminalTest, SaveRestoreRAII) {
+  struct termios orig_terminfo;
+  struct termios terminfo;
+  ASSERT_EQ(tcgetattr(m_slave, &orig_terminfo), 0);
+
+  Terminal term{m_slave};
+
+  {
+    TerminalState term_state{term};
+    terminfo = orig_terminfo;
+
+    // make some arbitrary changes
+    terminfo.c_iflag ^= IGNPAR | INLCR;
+    terminfo.c_oflag ^= OPOST | OCRNL;
+    terminfo.c_cflag ^= PARENB | PARODD;
+    terminfo.c_lflag ^= ICANON | ECHO;
+    terminfo.c_cc[VEOF] ^= 8;
+    terminfo.c_cc[VEOL] ^= 4;
+    cfsetispeed(&terminfo, B9600);
+    cfsetospeed(&terminfo, B9600);
+
+    ASSERT_EQ(tcsetattr(m_slave, TCSANOW, &terminfo), 0);
+  }
+
+  ASSERT_EQ(tcgetattr(m_slave, &terminfo), 0);
+  ASSERT_EQ(memcmp(&terminfo, &orig_terminfo, sizeof(terminfo)), 0);
+}
+
+TEST_F(TerminalTest, SaveRestore) {
+  TerminalState term_state;
+
+  struct termios orig_terminfo;
+  struct termios terminfo;
+  ASSERT_EQ(tcgetattr(m_slave, &orig_terminfo), 0);
+
+  Terminal term{m_slave};
+  term_state.Save(term, false);
+  terminfo = orig_terminfo;
+
+  // make some arbitrary changes
+  terminfo.c_iflag ^= IGNPAR | INLCR;
+  terminfo.c_oflag ^= OPOST | OCRNL;
+  terminfo.c_cflag ^= PARENB | PARODD;
+  terminfo.c_lflag ^= ICANON | ECHO;
+  terminfo.c_cc[VEOF] ^= 8;
+  terminfo.c_cc[VEOL] ^= 4;
+  cfsetispeed(&terminfo, B9600);
+  cfsetospeed(&terminfo, B9600);
+
+  ASSERT_EQ(tcsetattr(m_slave, TCSANOW, &terminfo), 0);
+
+  term_state.Restore();
+  ASSERT_EQ(tcgetattr(m_slave, &terminfo), 0);
+  ASSERT_EQ(memcmp(&terminfo, &orig_terminfo, sizeof(terminfo)), 0);
+}