[lldb-dap] Ensure we do not print the close sentinel when closing stdout.

[ashgti]

If you have an lldb-dap log file you'll almost always see a final message like:

<-- 
Content-Length: 94

{
  "body": {
    "category": "stdout",
    "output": "\u0000\u0000"
  },
  "event": "output",
  "seq": 0,
  "type": "event"
}
<-- 
Content-Length: 94

{
  "body": {
    "category": "stderr",
    "output": "\u0000\u0000"
  },
  "event": "output",
  "seq": 0,
  "type": "event"
}

The OutputRedirect is always writing the "\0" byte as a final stdout message during shutdown. Instead, I adjusted this to detect the sentinel value and break out of the read loop as if we detected EOF.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: John Harrison (ashgti)

Changes

If you have an lldb-dap log file you'll almost always see a final message like:

<-- 
Content-Length: 94

{
  "body": {
    "category": "stdout",
    "output": "\u0000\u0000"
  },
  "event": "output",
  "seq": 0,
  "type": "event"
}
<-- 
Content-Length: 94

{
  "body": {
    "category": "stderr",
    "output": "\u0000\u0000"
  },
  "event": "output",
  "seq": 0,
  "type": "event"
}

The OutputRedirect is always writing the "\0" byte as a final stdout message during shutdown. Instead, I adjusted this to detect the sentinel value and break out of the read loop as if we detected EOF.

---

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

1 Files Affected:

• (modified) lldb/tools/lldb-dap/OutputRedirector.cpp (+10-3)

diff --git a/lldb/tools/lldb-dap/OutputRedirector.cpp b/lldb/tools/lldb-dap/OutputRedirector.cpp
index a23572ab7ae04..c5090338cfd1c 100644
--- a/lldb/tools/lldb-dap/OutputRedirector.cpp
+++ b/lldb/tools/lldb-dap/OutputRedirector.cpp
@@ -10,6 +10,7 @@
 #include "DAP.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
+#include <cstring>
 #include <system_error>
 #if defined(_WIN32)
 #include <fcntl.h>
@@ -25,6 +26,10 @@ using llvm::Expected;
 using llvm::inconvertibleErrorCode;
 using llvm::StringRef;
 
+namespace {
+char kCloseSentinel[] = "\0";
+} // namespace
+
 namespace lldb_dap {
 
 int OutputRedirector::kInvalidDescriptor = -1;
@@ -59,7 +64,10 @@ Error OutputRedirector::RedirectTo(std::function<void(StringRef)> callback) {
     while (!m_stopped) {
       ssize_t bytes_count = ::read(read_fd, &buffer, sizeof(buffer));
       // EOF detected.
-      if (bytes_count == 0)
+      if (bytes_count == 0 ||
+          (bytes_count == sizeof(kCloseSentinel) &&
+           std::memcmp(buffer, kCloseSentinel, sizeof(kCloseSentinel)) == 0 &&
+           m_fd == kInvalidDescriptor))
         break;
       if (bytes_count == -1) {
         // Skip non-fatal errors.
@@ -85,8 +93,7 @@ void OutputRedirector::Stop() {
     // Closing the pipe may not be sufficient to wake up the thread in case the
     // write descriptor is duplicated (to stdout/err or to another process).
     // Write a null byte to ensure the read call returns.
-    char buf[] = "\0";
-    (void)::write(fd, buf, sizeof(buf));
+    (void)::write(fd, kCloseSentinel, sizeof(kCloseSentinel));
     ::close(fd);
     m_forwarder.join();
   }

[labath]

I can't make this an inline suggestion, but this would flow better if you replaced this with the following code after the -1 check:

StringRef data(buffer, bytes_count);
if (data.empty() || (m_stopped && data == kCloseSentinel)) break;

I'm also noting this will not always remove the sentinel as it can end up attached to some other output. That could be handled by something like

if (m_stopped) data = data.consume_back(kCloseSentinel);
if (data.empty()); break;

.. but that has the potential to remove an unrelated piece of text. Since neither of them is perfect, I'll leave it up to you to choose.

[ashgti]

I used the second suggestion and added some tests to try to validate we don't incorrectly consume any null bytes produced by the debuggee.