[lldb-dap] Improving EOF handling on stream input and adding new unit tests

[ashgti]

This should improve the handling of EOF on stdin and adding some new unit tests to malformed requests.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: John Harrison (ashgti)

Changes

This should improve the handling of EOF on stdin and adding some new unit tests to malformed requests.

---

Full diff: https://github.com/llvm/llvm-project/pull/129581.diff

3 Files Affected:

• (added) lldb/test/API/tools/lldb-dap/io/TestDAP_io.py (+72)
• (modified) lldb/tools/lldb-dap/DAP.cpp (+5-1)
• (modified) lldb/tools/lldb-dap/IOStream.cpp (+10-2)

diff --git a/lldb/test/API/tools/lldb-dap/io/TestDAP_io.py b/lldb/test/API/tools/lldb-dap/io/TestDAP_io.py
new file mode 100644
index 0000000000000..a39bd17ceb3b3
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/io/TestDAP_io.py
@@ -0,0 +1,72 @@
+"""
+Test lldb-dap IO handling.
+"""
+
+from lldbsuite.test.decorators import *
+import lldbdap_testcase
+import dap_server
+
+
+class TestDAP_io(lldbdap_testcase.DAPTestCaseBase):
+    def launch(self):
+        log_file_path = self.getBuildArtifact("dap.txt")
+        process, _ = dap_server.DebugAdapterServer.launch(
+            executable=self.lldbDAPExec, log_file=log_file_path
+        )
+
+        def cleanup():
+            # If the process is still alive, terminate it.
+            if process.poll() is None:
+                process.terminate()
+                stdout_data = process.stdout.read()
+                stderr_data = process.stderr.read()
+                print("========= STDOUT =========")
+                print(stdout_data)
+                print("========= END =========")
+                print("========= STDERR =========")
+                print(stderr_data)
+                print("========= END =========")
+                print("========= DEBUG ADAPTER PROTOCOL LOGS =========")
+                with open(log_file_path, "r") as file:
+                    print(file.read())
+                print("========= END =========")
+
+        # Execute the cleanup function during test case tear down.
+        self.addTearDownHook(cleanup)
+
+        return process
+
+    def test_eof_immediately(self):
+        """
+        lldb-dap handles EOF without any other input.
+        """
+        process = self.launch()
+        process.stdin.close()
+        self.assertEqual(process.wait(timeout=5.0), 0)
+
+    def test_partial_header(self):
+        """
+        lldb-dap handles parital message headers.
+        """
+        process = self.launch()
+        process.stdin.write(b"Content-Length: ")
+        process.stdin.close()
+        self.assertEqual(process.wait(timeout=5.0), 0)
+
+    def test_incorrect_content_length(self):
+        """
+        lldb-dap handles malformed content length headers.
+        """
+        process = self.launch()
+        process.stdin.write(b"Content-Length: abc")
+        process.stdin.close()
+        self.assertEqual(process.wait(timeout=5.0), 0)
+
+    def test_partial_content_length(self):
+        """
+        lldb-dap handles partial messages.
+        """
+        process = self.launch()
+        process.stdin.write(b"Content-Length: 10{")
+        process.stdin.close()
+        self.assertEqual(process.wait(timeout=5.0), 0)
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 53c514b790f38..3dc9d6f5ca0a4 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -856,7 +856,11 @@ lldb::SBError DAP::Disconnect(bool terminateDebuggee) {
 }
 
 llvm::Error DAP::Loop() {
-  auto cleanup = llvm::make_scope_exit([this]() { StopEventHandlers(); });
+  auto cleanup = llvm::make_scope_exit([this]() {
+    if (output.descriptor)
+      output.descriptor->Close();
+    StopEventHandlers();
+  });
   while (!disconnecting) {
     llvm::json::Object object;
     lldb_dap::PacketStatus status = GetNextObject(object);
diff --git a/lldb/tools/lldb-dap/IOStream.cpp b/lldb/tools/lldb-dap/IOStream.cpp
index c6f1bfaf3b799..ee22a297ec248 100644
--- a/lldb/tools/lldb-dap/IOStream.cpp
+++ b/lldb/tools/lldb-dap/IOStream.cpp
@@ -35,16 +35,23 @@ bool InputStream::read_full(std::ofstream *log, size_t length,
   if (status.Fail())
     return false;
 
-  text += data;
+  text += data.substr(0, length);
   return true;
 }
 
 bool InputStream::read_line(std::ofstream *log, std::string &line) {
   line.clear();
   while (true) {
-    if (!read_full(log, 1, line))
+    std::string next;
+    if (!read_full(log, 1, next))
       return false;
 
+    // If EOF is encoutnered, '' is returned, break out of this loop.
+    if (next.empty())
+      return false;
+
+    line += next;
+
     if (llvm::StringRef(line).ends_with("\r\n"))
       break;
   }
@@ -60,6 +67,7 @@ bool InputStream::read_expected(std::ofstream *log, llvm::StringRef expected) {
     if (log)
       *log << "Warning: Expected '" << expected.str() << "', got '" << result
            << "\n";
+    return false;
   }
   return true;
 }

[oontvoo]

This would fix a test failure that's blocking our release, so would really appreciate it if we could get this submitted soon :) Thanks!

[JDevlieghere]

I'm a little worried about the lack of abstraction here. Should {Input,Output}Stream be a class instead of a struct and encapsulate this operation? We close the descriptor (if it's owned) when we destroy the IOObjectSP. So should we just reset the IOObjectSP pointer instead?

[JDevlieghere]

I think I'd prefer to get rid of the InputStream and OutputStream classes altogether. They seem to add very little on top of IOObjectSP so maybe we can just store those directly, or alternative, make the relation an "is a" instead of a "has a".

[ashgti]

I have a patch to do just that in ashgti@a57a16f but I was waiting to do that until after I submitted #129155

[ashgti]

I also wasn't sure if I should submit the logging refactor because of #129294 (comment) so that is another PR I was waiting on figuring out first.

[JDevlieghere]

Ah fair enough, let me take another look at that PR.

[JDevlieghere]

Haven't had a chance to investigate this yet, but this is failing consistently for me locally:

FAIL: test_partial_header (TestDAP_io.TestDAP_io)
   lldb-dap handles parital message headers.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jonas/llvm/llvm-project/lldb/test/API/tools/lldb-dap/io/TestDAP_io.py", line 54, in test_partial_header
    self.assertEqual(process.wait(timeout=5.0), 0)
AssertionError: -13 != 0
Config=arm64-/Users/jonas/llvm/build-ra/bin/clang
----------------------------------------------------------------------

Seeing the same thing for TestDAP_launch.TestDAP_launch:

FAIL: test_termination (TestDAP_launch.TestDAP_launch)
    Tests the correct termination of lldb-dap upon a 'disconnect'
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jonas/llvm/llvm-project/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py", line 47, in test_termination
    self.assertEqual(self.dap_server.process.poll(), 0)
AssertionError: -13 != 0
Config=arm64-/Users/jonas/llvm/build-ra/bin/clang
----------------------------------------------------------------------

[ashgti]

That is a SIGPIPE, so something is writing after the output is closed.

I can try that locally and see if I have the same issue.