[Lldb-commits] [PATCH] D80659: [lldb-vscode] Redirect stderr and stdout to DAPs console message

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Sorry, I missed this patch. Modulo the inline comments and the missing windows support and tests, I think this patch is ok-ish. However, I am still not convinced of its usefulness. Why would we be doing something (particularly a thing which will be hard to do in a cross-platform manner, and will very likely border on, or downright cross into, undefined behavior territory), if we get that from vscode for free?



================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:519-520
+    void StopRedirecting() {
+      if (!m_thread.joinable())
+        return;
+      close(m_captured_fd[1]);
----------------
This is very fishy. If the thread is detached in `StartRedirecting`, `joinable` will never be true.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:521
+        return;
+      close(m_captured_fd[1]);
+      m_thread.join();
----------------
Who's closing `m_captured_fd[0]` ?


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2919-2922
+  // Can now safely stop redirecting since lldb will no longer emit stdout
+  // or stderr messages.
+  stdout_redirector.StopRedirecting();
+  stderr_redirector.StopRedirecting();
----------------
Rely on  the desctructor calling these? Or have the  destructor assert they have been called?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80659/new/

https://reviews.llvm.org/D80659