[lldb-vscode] Redirect stderr and stdout to DAPs console message

[wallace]

Summary

Doing this for 2 main reasons:

• Avoid stdout from corrupting DAP messages
• Make sure we can see in VSCode (or any other DAP client) the same output we see when debugging from the command line

[labath]

I don't actually use vscode, so I don't really know whether this is a good idea, but if you say it is then I can go with that.

However, I think the implementation needs a lot of work:

• it is very windows-hostile: dup, pipe, select, etc. don't exist or don't work on windows (btw, doing a blocking select before a blocking read is completely useless -- you might as well just do the read alone)
• the lack of synchronization between the (detached) forwarding threads makes it very prone to crashes if something happens to write to stderr during shutdown or early in initialization

The way I would imagine doing this is:

• redirect stdout/err as early as possible to avoid something writing to it, but don't forward anything yet
• when things are sufficiently initialized, start up the forwarding machinery
• at, an appropriate point during shutdown, stop forwarding

The stop forwarding is the trickiest part I think the most portable solution would be to just close the write end of the pipe, and join the forwarding thread (which will exit once read returns EOF).

We should also think about testing all of this. Do we have a reliable mechanism to produce stdout/err output from lldb? I guess it the worst case we could try debugging a module which produces one of these warnings.

Or, if this output is going to the same place as the SBDebuggers notion of stdout/err, then we could just stop calling SetOutputFileHandle, and have everything flow through this. That would reduce the overall complexity and would ensure this path gets better coverage, as it would be used in the common case instead of just for printing random errors.

[aadsm]

thanks for the feedback, I'll read it more carefully later. As for the tests it should be easy to set up, didn't want to spend time on that before we agreed on doing it this way.

[labath]

Btw, given that Richard has found some vscode code which already tries to send stderr to the console window https://github.com/microsoft/vscode/blob/master/src/vs/workbench/contrib/debug/node/debugAdapter.ts#L234, it would be good to first understand why doesn't that kick in. Maybe there's a vscode bug that could be fixed instead.

[aadsm]

Absolutely, I'll look into that first.

[rmaz]

In D80659#2062304, @labath wrote:

Btw, given that Richard has found some vscode code which already tries to send stderr to the console window https://github.com/microsoft/vscode/blob/master/src/vs/workbench/contrib/debug/node/debugAdapter.ts#L234, it would be good to first understand why doesn't that kick in. Maybe there's a vscode bug that could be fixed instead.

The issue is fairly clear, that listener is only added if the optional constructor arg outputService is defined, but it never is. I was planning on putting up a diff this week some time to always read from stderr regardless.

[aadsm]

This update still doesn't add windows support but I just want to get a feel that I'm going on the right direction.
It addresses the following:

• redirect stdout/err as early as possible to avoid something writing to it, and only starts or ends redirection it once it's safe.
• adds a mutex to OutputStream::write_full to make sure multiple threads don't try to write to the output fd at the same time.

We should also think about testing all of this. Do we have a reliable mechanism to produce stdout/err output from lldb?

this turned out to be harder than I thought it would be, my plan was to print to stdout in python from a thread but I never see that print anywhere (not sure why though). A sure way to test this (what I did manually) is to put a script print('foo') command in lldninit. Unfortunately I can't use a local lldbinit since that's a concept of the lldb binary and not of liblldb (unless we want to add that option to lldb-vscode as well), so not really sure about this as well.
I also tried to use a breakpoint command add -o 'script print("foo")' but same behaviour, I never saw that print. Still don't know how to tackle this, need to think more about it.

Or, if this output is going to the same place as the SBDebuggers notion of stdout/err,

There no guarantee of this, people can just use sys.stdout.write in python.

given that Richard has found some vscode code which already tries to send stderr to the console window

I would still prefer to wrap liblldb's stderr into a console message so people can see it in the client(e.g.: VSCode) console rather than outputting it through the lldb-vscode stderr which will then be up to the client to decide what to do with it.

[labath]

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?

lldb/tools/lldb-vscode/lldb-vscode.cpp

518–519

This is very fishy. If the thread is detached in StartRedirecting, joinable will never be true.

520

Who's closing m_captured_fd[0] ?

2899–2902

Rely on the desctructor calling these? Or have the destructor assert they have been called?

[aadsm]

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?

I'm not sure what you mean with get this for free on vscode... Right now lldb-vscode will just crash if there's any command in the lldbinit that outputs to stdout because it pollutes the protocol, I don't think think there's anything vscode can do about this. But even if it did vscode is not the only client of lldb-vscode, there are multiple others: https://microsoft.github.io/debug-adapter-protocol/implementors/tools/.

I haven't answered this yet because I was trying to figure out why this only happens when parsing the lldbinit. I was looking into the CommandInterpreter{Python} code but to be honest it's not easy to figure out because there are multiple levels of indirection when it comes to processing the IO of the python interpreter, but it seems that the stdout while executing the lldbinit script is being flushed right away to the ImmediateOutputStream but haven't figure out when this is set in the case of CommandInterpreter::IOHandlerInputComplete.

[labath]

Sorry, I confused this with the patch that was wrapping stderr into DAP messages.

Having python output break the protocol is definitely not good. Fixing it would be nice, but the solution should work on windows as well.

One possibility I'd consider is making this fix at the python level (as this is a python problem). We already are trying to do some stdin/out/err redirection there, but it seems it's not good enough. It seems it should be easy enough (though hacky, but all of this is going to be hacky anyway) to replace sys.stdin/out/err with a custom wrapper object.

[wallace]

not needed anymore because of D99974

lldb/tools/lldb-vscode/lldb-vscode.cpp

 //===-- lldb-vscode.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
 //
 //===----------------------------------------------------------------------===//
 
if (sourceMap) {
     strm << "\".\" \"" << sourcePath << "\"";
   }
   strm.flush();
   if (!sourceMapCommand.empty()) {
     g_vsc.RunLLDBCommands("Setting source map:", {sourceMapCommand});
   }
 }
 
+int RedirectFileDescriptorToConsoleOutput(int fd) {
+  int new_fd = dup(fd);
+  int captured_fd[2];
+
+  pipe(captured_fd);
+  dup2(captured_fd[1], fd);
+  int read_fd = captured_fd[0];
+
+  std::thread([read_fd] {
+    char buffer[4096];
+    fd_set readfds;
+    while (true) {
+      FD_ZERO(&readfds);
+      FD_SET(read_fd, &readfds);
+      select(read_fd + 1, &readfds, nullptr, nullptr, nullptr);
+      if (FD_ISSET(read_fd, &readfds)) {
+        read(read_fd, &buffer, sizeof(buffer));
+        g_vsc.SendOutput(OutputType::Console, buffer);
+      }
+    }
+  }).detach();
+
+  return new_fd;
+}
+
 // "AttachRequest": {
 //   "allOf": [ { "$ref": "#/definitions/Request" }, {
 //     "type": "object",
 //     "description": "Attach request; value of command field is 'attach'.",
 //     "properties": {
 //       "command": {
 //         "type": "string",
 //         "enum": [ "attach" ]
 //       },
 //       "arguments": {
 //         "$ref": "#/definitions/AttachRequestArguments"
 //       }
 //     },
 //     "required": [ "command", "arguments" ]
 //   }]
 // },
if (portno != -1) {
     SOCKET socket_fd = AcceptConnection(portno);
     if (socket_fd >= 0) {
       g_vsc.input.descriptor = StreamDescriptor::from_socket(socket_fd, true);
       g_vsc.output.descriptor = StreamDescriptor::from_socket(socket_fd, false);
     } else {
       exit(1);
     }
   } else {
+    int new_stdout_fd = RedirectFileDescriptorToConsoleOutput(fileno(stdout));
+    RedirectFileDescriptorToConsoleOutput(fileno(stderr));
     g_vsc.input.descriptor = StreamDescriptor::from_file(fileno(stdin), false);
-    g_vsc.output.descriptor =
-        StreamDescriptor::from_file(fileno(stdout), false);
+    g_vsc.output.descriptor = StreamDescriptor::from_file(new_stdout_fd, false);
   }
   auto request_handlers = GetRequestHandlers();
   uint32_t packet_idx = 0;
   while (!g_vsc.sent_terminated_event) {
     std::string json = g_vsc.ReadJSON();
     if (json.empty())
       break;
 
if (packet_type == "request") {
       }
     }
     ++packet_idx;
   }
 
   // We must terminate the debugger in a thread before the C++ destructor
   // chain messes everything up.
   lldb::SBDebugger::Terminate();
   return 0;
 }