Moving the output redirection logic back into the OutputRedirector.cpp file and making it into an object instead.

This removes some of the duplicate code that was happening between stdout/stderr redirection.

Author: ashgti

lldb/tools/lldb-dap/CMakeLists.txt

@@ -22,18 +22,19 @@ add_lldb_tool(lldb-dap
   lldb-dap.cpp
   Breakpoint.cpp
   BreakpointBase.cpp
+  DAP.cpp
   ExceptionBreakpoint.cpp
   FifoFiles.cpp
   FunctionBreakpoint.cpp
+  InstructionBreakpoint.cpp
   IOStream.cpp
   JSONUtils.cpp
   LLDBUtils.cpp
+  OutputRedirector.cpp
   ProgressEvent.cpp
   RunInTerminal.cpp
   SourceBreakpoint.cpp
-  DAP.cpp
   Watchpoint.cpp
-  InstructionBreakpoint.cpp
 
   LINK_LIBS
     liblldb

lldb/tools/lldb-dap/DAP.cpp

@@ -9,6 +9,7 @@
 #include "DAP.h"
 #include "JSONUtils.h"
 #include "LLDBUtils.h"
+#include "OutputRedirector.h"
 #include "lldb/API/SBBreakpoint.h"
 #include "lldb/API/SBCommandInterpreter.h"
 #include "lldb/API/SBCommandReturnObject.h"
@@ -49,8 +50,8 @@ using namespace lldb_dap;
 
 namespace lldb_dap {
 
-DAP::DAP(llvm::StringRef path, std::optional<std::ofstream> &log,
-         ReplMode repl_mode, StreamDescriptor input, StreamDescriptor output)
+DAP::DAP(llvm::StringRef path, std::ofstream *log, ReplMode repl_mode,
+         StreamDescriptor input, StreamDescriptor output)
     : debug_adaptor_path(path), log(log), input(std::move(input)),
       output(std::move(output)), broadcaster("lldb-dap"),
       exception_breakpoints(), focus_tid(LLDB_INVALID_THREAD_ID),
@@ -178,63 +179,55 @@ ExceptionBreakpoint *DAP::GetExceptionBreakpoint(const lldb::break_id_t bp_id) {
   return nullptr;
 }
 
-llvm::Error DAP::ConfigureIO(std::optional<std::FILE *> overrideOut,
-                             std::optional<std::FILE *> overrideErr) {
+llvm::Error DAP::ConfigureIO(std::FILE *overrideOut, std::FILE *overrideErr) {
   auto *inull = lldb_private::FileSystem::Instance().Fopen(
       lldb_private::FileSystem::DEV_NULL, "w");
   in = lldb::SBFile(inull, true);
 
-  lldb_private::Status status;
-  status = pout.CreateNew(/*child_process_inherit=*/false);
-  if (status.Fail())
-    return status.takeError();
-  status = perr.CreateNew(/*child_process_inherit=*/false);
-  if (status.Fail())
-    return status.takeError();
+  if (auto Error = out.RedirectTo([this](llvm::StringRef output) {
+        SendOutput(OutputType::Stdout, output);
+      }))
+    return Error;
 
   if (overrideOut) {
-    if (dup2(pout.GetWriteFileDescriptor(), fileno(*overrideOut)) == -1) {
+    auto fd = out.GetWriteFileDescriptor();
+    if (auto Error = fd.takeError())
+      return Error;
+
+    if (dup2(*fd, fileno(overrideOut)) == -1)
       return llvm::make_error<llvm::StringError>(
           llvm::errnoAsErrorCode(),
-          llvm::formatv("override fd=%d failed", fileno(*overrideOut))
+          llvm::formatv("override fd=%d failed", fileno(overrideOut))
               .str()
               .c_str());
-    }
   }
 
+  if (auto Error = err.RedirectTo([this](llvm::StringRef output) {
+        SendOutput(OutputType::Stderr, output);
+      }))
+    return Error;
+
   if (overrideErr) {
-    if (dup2(perr.GetWriteFileDescriptor(), fileno(*overrideErr)) == -1) {
+    auto fd = err.GetWriteFileDescriptor();
+    if (auto Error = fd.takeError())
+      return Error;
+
+    if (dup2(*fd, fileno(overrideErr)) == -1)
       return llvm::make_error<llvm::StringError>(
           llvm::errnoAsErrorCode(),
-          llvm::formatv("override fd=%d failed", fileno(*overrideErr))
+          llvm::formatv("override fd=%d failed", fileno(overrideErr))
               .str()
               .c_str());
-    }
   }
 
-  auto forwarder = [&](lldb_private::Pipe &pipe, OutputType outputType) {
-    char buffer[4098];
-    size_t bytes_read;
-    while (pipe.CanRead()) {
-      lldb_private::Status error = pipe.ReadWithTimeout(
-          &buffer, sizeof(buffer), std::chrono::seconds(1), bytes_read);
-      if (error.Success()) {
-        // zero bytes returned on EOF.
-        if (bytes_read == 0)
-          break;
-        SendOutput(outputType, llvm::StringRef(buffer, bytes_read));
-      }
-    }
-  };
-
-  stdout_forward_thread =
-      std::thread(forwarder, std::ref(pout), OutputType::Stdout);
-  stderr_forward_thread =
-      std::thread(forwarder, std::ref(perr), OutputType::Stderr);
-
   return llvm::Error::success();
 }
 
+void DAP::StopIO() {
+  out.Stop();
+  err.Stop();
+}
+
 // Send the JSON in "json_str" to the "out" stream. Correctly send the
 // "Content-Length:" field followed by the length, followed by the raw
 // JSON bytes.

lldb/tools/lldb-dap/DAP.h

@@ -14,6 +14,7 @@
 #include "FunctionBreakpoint.h"
 #include "IOStream.h"
 #include "InstructionBreakpoint.h"
+#include "OutputRedirector.h"
 #include "ProgressEvent.h"
 #include "SourceBreakpoint.h"
 #include "lldb/API/SBBroadcaster.h"
@@ -27,7 +28,6 @@
 #include "lldb/API/SBThread.h"
 #include "lldb/API/SBValue.h"
 #include "lldb/API/SBValueList.h"
-#include "lldb/Host/Pipe.h"
 #include "lldb/lldb-types.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
@@ -140,12 +140,12 @@ struct SendEventRequestHandler : public lldb::SBCommandPluginInterface {
 
 struct DAP {
   llvm::StringRef debug_adaptor_path;
-  std::optional<std::ofstream> &log;
+  std::ofstream *log;
   InputStream input;
   OutputStream output;
   lldb::SBFile in;
-  lldb_private::Pipe pout;
-  lldb_private::Pipe perr;
+  OutputRedirector out;
+  OutputRedirector err;
   lldb::SBDebugger debugger;
   lldb::SBTarget target;
   Variables variables;
@@ -205,8 +205,8 @@ struct DAP {
   // will contain that expression.
   std::string last_nonempty_var_expression;
 
-  DAP(llvm::StringRef path, std::optional<std::ofstream> &log,
-      ReplMode repl_mode, StreamDescriptor input, StreamDescriptor output);
+  DAP(llvm::StringRef path, std::ofstream *log, ReplMode repl_mode,
+      StreamDescriptor input, StreamDescriptor output);
   ~DAP();
   DAP(const DAP &rhs) = delete;
   void operator=(const DAP &rhs) = delete;
@@ -217,8 +217,10 @@ struct DAP {
   ///
   /// Errors in this operation will be printed to the log file and the IDE's
   /// console output as well.
-  llvm::Error ConfigureIO(std::optional<std::FILE *> overrideOut,
-                          std::optional<std::FILE *> overrideErr);
+  llvm::Error ConfigureIO(std::FILE *overrideOut, std::FILE *overrideErr);
+
+  /// Stop the redirected IO threads and associated pipes.
+  void StopIO();
 
   // Serialize the JSON value into a string and send the JSON packet to
   // the "out" stream.

lldb/tools/lldb-dap/IOStream.cpp

@@ -87,7 +87,7 @@ bool OutputStream::write_full(llvm::StringRef str) {
   return true;
 }
 
-bool InputStream::read_full(std::optional<std::ofstream> &log, size_t length,
+bool InputStream::read_full(std::ofstream *log, size_t length,
                             std::string &text) {
   std::string data;
   data.resize(length);
@@ -131,8 +131,7 @@ bool InputStream::read_full(std::optional<std::ofstream> &log, size_t length,
   return true;
 }
 
-bool InputStream::read_line(std::optional<std::ofstream> &log,
-                            std::string &line) {
+bool InputStream::read_line(std::ofstream *log, std::string &line) {
   line.clear();
   while (true) {
     if (!read_full(log, 1, line))
@@ -145,8 +144,7 @@ bool InputStream::read_line(std::optional<std::ofstream> &log,
   return true;
 }
 
-bool InputStream::read_expected(std::optional<std::ofstream> &log,
-                                llvm::StringRef expected) {
+bool InputStream::read_expected(std::ofstream *log, llvm::StringRef expected) {
   std::string result;
   if (!read_full(log, expected.size(), result))
     return false;

lldb/tools/lldb-dap/IOStream.h

@@ -55,13 +55,11 @@ struct InputStream {
   explicit InputStream(StreamDescriptor descriptor)
       : descriptor(std::move(descriptor)) {};
 
-  bool read_full(std::optional<std::ofstream> &log, size_t length,
-                 std::string &text);
+  bool read_full(std::ofstream *log, size_t length, std::string &text);
 
-  bool read_line(std::optional<std::ofstream> &log, std::string &line);
+  bool read_line(std::ofstream *log, std::string &line);
 
-  bool read_expected(std::optional<std::ofstream> &log,
-                     llvm::StringRef expected);
+  bool read_expected(std::ofstream *log, llvm::StringRef expected);
 };
 
 struct OutputStream {

lldb/tools/lldb-dap/OutputRedirector.cpp

@@ -0,0 +1,77 @@
+//===-- OutputRedirector.cpp -----------------------------------*- 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
+//
+//===----------------------------------------------------------------------===/
+
+#include "llvm/Support/Error.h"
+#include <system_error>
+#if defined(_WIN32)
+#include <fcntl.h>
+#include <io.h>
+#else
+#include <unistd.h>
+#endif
+
+#include "DAP.h"
+#include "OutputRedirector.h"
+#include "llvm/ADT/StringRef.h"
+
+using lldb_private::Pipe;
+using lldb_private::Status;
+using llvm::createStringError;
+using llvm::Error;
+using llvm::Expected;
+using llvm::StringRef;
+
+namespace lldb_dap {
+
+Expected<int> OutputRedirector::GetWriteFileDescriptor() {
+  if (!m_pipe.CanWrite())
+    return createStringError(std::errc::bad_file_descriptor,
+                             "write handle is not open for writing");
+  return m_pipe.GetWriteFileDescriptor();
+}
+
+Error OutputRedirector::RedirectTo(std::function<void(StringRef)> callback) {
+  Status status = m_pipe.CreateNew(/*child_process_inherit=*/false);
+  if (status.Fail())
+    return status.takeError();
+
+  m_forwarder = std::thread([this, callback]() {
+    char buffer[OutputBufferSize];
+    while (m_pipe.CanRead() && !m_stopped) {
+      size_t bytes_read;
+      Status status = m_pipe.Read(&buffer, sizeof(buffer), bytes_read);
+      if (status.Fail())
+        continue;
+
+      // EOF detected
+      if (bytes_read == 0)
+        break;
+
+      callback(StringRef(buffer, bytes_read));
+    }
+  });
+
+  return Error::success();
+}
+
+void OutputRedirector::Stop() {
+  m_stopped = true;
+
+  if (m_pipe.CanWrite()) {
+    // If the fd is waiting for input and is closed it may not return from the
+    // current select/poll/kqueue/etc. asyncio wait operation. Write a null byte
+    // to ensure the read fd wakes to detect the closed FD.
+    char buf[] = "\0";
+    size_t bytes_written;
+    m_pipe.Write(buf, sizeof(buf), bytes_written);
+    m_pipe.CloseWriteFileDescriptor();
+    m_forwarder.join();
+  }
+}
+
+} // namespace lldb_dap

lldb/tools/lldb-dap/OutputRedirector.h

@@ -0,0 +1,47 @@
+//===-- OutputRedirector.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_TOOLS_LLDB_DAP_OUTPUT_REDIRECTOR_H
+#define LLDB_TOOLS_LLDB_DAP_OUTPUT_REDIRECTOR_H
+
+#include "lldb/Host/Pipe.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include <atomic>
+#include <functional>
+#include <thread>
+
+namespace lldb_dap {
+
+struct OutputRedirector {
+  /// Creates writable file descriptor that will invoke the given callback on
+  /// each write in a background thread.
+  ///
+  /// \return
+  ///     \a Error::success if the redirection was set up correctly, or an error
+  ///     otherwise.
+  llvm::Error RedirectTo(std::function<void(llvm::StringRef)> callback);
+
+  llvm::Expected<int> GetWriteFileDescriptor();
+  void Stop();
+
+  ~OutputRedirector() { Stop(); }
+
+  OutputRedirector() = default;
+  OutputRedirector(const OutputRedirector &) = delete;
+  OutputRedirector &operator=(const OutputRedirector &) = delete;
+
+private:
+  std::atomic<bool> m_stopped = false;
+  lldb_private::Pipe m_pipe;
+  std::thread m_forwarder;
+};
+
+} // namespace lldb_dap
+
+#endif // LLDB_TOOLS_LLDB_DAP_OUTPUT_REDIRECTOR_H