This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Add unit tests for Terminal API
ClosedPublic

Authored by mgorny on Oct 1 2021, 12:29 PM.

Diff Detail

Event Timeline

mgorny requested review of this revision.Oct 1 2021, 12:29 PM
mgorny updated this revision to Diff 376607.
mgorny created this revision.

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 added a comment.Oct 4 2021, 2:51 AM

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 marked an inline comment as done.Oct 4 2021, 3:47 AM

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 updated this revision to Diff 376861.Oct 4 2021, 4:33 AM
mgorny marked an inline comment as done.

Use PseudoTerminal class.

labath accepted this revision.Oct 4 2021, 4:53 AM
labath added inline comments.
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.

This revision is now accepted and ready to land.Oct 4 2021, 4:53 AM
mgorny marked an inline comment as done.Oct 4 2021, 5:19 AM
mgorny added inline comments.
lldb/unittests/Host/CMakeLists.txt
27

Sure, done that.

This revision was automatically updated to reflect the committed changes.
mgorny marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2021, 5:20 AM

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 added a comment.Oct 5 2021, 4:21 AM

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 added a comment.Oct 5 2021, 4:25 AM

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

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 added a comment.Oct 5 2021, 6:48 AM

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.

"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);
mgorny added a comment.Oct 5 2021, 8:17 AM

I suppose that's good enough.

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