[Lldb-commits] [lldb] [lldb-dap] Ensure the IO forwarding threads are managed by the DAP object lifecycle. (PR #120457)

[Pavel Labath via lldb-commits]

================
@@ -17,47 +19,59 @@
 #include "OutputRedirector.h"
 #include "llvm/ADT/StringRef.h"
 
-using namespace llvm;
+using lldb_private::Pipe;
+using lldb_private::Status;
+using llvm::createStringError;
+using llvm::Error;
+using llvm::Expected;
+using llvm::StringRef;
 
 namespace lldb_dap {
 
-Error RedirectFd(int fd, std::function<void(llvm::StringRef)> callback) {
-  int new_fd[2];
-#if defined(_WIN32)
-  if (_pipe(new_fd, 4096, O_TEXT) == -1) {
-#else
-  if (pipe(new_fd) == -1) {
-#endif
-    int error = errno;
-    return createStringError(inconvertibleErrorCode(),
-                             "Couldn't create new pipe for fd %d. %s", fd,
-                             strerror(error));
-  }
+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();
+}
 
-  if (dup2(new_fd[1], fd) == -1) {
-    int error = errno;
-    return createStringError(inconvertibleErrorCode(),
-                             "Couldn't override the fd %d. %s", fd,
-                             strerror(error));
-  }
+Error OutputRedirector::RedirectTo(std::function<void(StringRef)> callback) {
+  Status status = m_pipe.CreateNew(/*child_process_inherit=*/false);
+  if (status.Fail())
+    return status.takeError();
 
-  int read_fd = new_fd[0];
-  std::thread t([read_fd, callback]() {
+  m_forwarder = std::thread([this, callback]() {
     char buffer[OutputBufferSize];
-    while (true) {
-      ssize_t bytes_count = read(read_fd, &buffer, sizeof(buffer));
-      if (bytes_count == 0)
-        return;
-      if (bytes_count == -1) {
-        if (errno == EAGAIN || errno == EINTR)
-          continue;
+    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)
----------------
labath wrote:

```suggestion
    while (m_pipe.CanRead()) {
      size_t bytes_read;
      Status status = m_pipe.Read(&buffer, sizeof(buffer), bytes_read);
      if (status.Fail())
        continue;

      // EOF detected
      if (bytes_read == 0 || m_stopped)
```

.. motivation being to avoid sending the empty null byte. It's true that this can prevent us from sending other data as well, but I don't think anything can rely on that, as the thread can still exit before it manages to read that data from the pipe. If anything wanted to rely on some output being sent, it would have to implement to form of out-of-band synchronization to make sure the data is received before shutting things down.

https://github.com/llvm/llvm-project/pull/120457