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

[walter erquinigo via Phabricator via lldb-commits]

wallace abandoned this revision.
wallace added a comment.

not needed anymore because of D99974 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80659

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80659

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[António Afonso via Phabricator via lldb-commits]

aadsm added a comment.

> 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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80659

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[António Afonso via Phabricator via lldb-commits]

aadsm updated this revision to Diff 276894.
aadsm added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80659

Files:
  lldb/tools/lldb-vscode/IOStream.cpp
  lldb/tools/lldb-vscode/IOStream.h
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -486,6 +486,43 @@
   }
 }
 
+class FileDescriptorToConsoleOutputRedirector {
+  private:
+int m_captured_fd[2];
+std::thread m_thread;
+
+  public:
+FileDescriptorToConsoleOutputRedirector(int fd) {
+  pipe(m_captured_fd);
+  dup2(m_captured_fd[1], fd);
+};
+~FileDescriptorToConsoleOutputRedirector() {
+  StopRedirecting();
+}
+// Only call this once.
+void StartRedirecting() {
+  int read_fd = m_captured_fd[0];
+
+  m_thread = std::thread([read_fd] {
+char buffer[4096];
+while (true) {
+  ssize_t bytes_count = read(read_fd, , sizeof(buffer));
+  if (bytes_count == 0)
+return;
+  g_vsc.SendOutput(OutputType::Console, buffer);
+}
+  });
+  m_thread.detach();
+}
+
+void StopRedirecting() {
+  if (!m_thread.joinable())
+return;
+  close(m_captured_fd[1]);
+  m_thread.join();
+}
+};
+
 // "AttachRequest": {
 //   "allOf": [ { "$ref": "#/definitions/Request" }, {
 // "type": "object",
@@ -2781,7 +2818,9 @@
 }
 
 int main(int argc, char *argv[]) {
-
+  int stdout_fd = dup(fileno(stdout));
+  FileDescriptorToConsoleOutputRedirector stdout_redirector(fileno(stdout));
+  FileDescriptorToConsoleOutputRedirector stderr_redirector(fileno(stderr));
   // Initialize LLDB first before we do anything.
   lldb::SBDebugger::Initialize();
 
@@ -2824,9 +2863,11 @@
 }
   } else {
 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(stdout_fd, false);
   }
+  // Can start redirecting now that the output description has been properly set.
+  stdout_redirector.StartRedirecting();
+  stderr_redirector.StartRedirecting();
   auto request_handlers = GetRequestHandlers();
   uint32_t packet_idx = 0;
   while (!g_vsc.sent_terminated_event) {
@@ -2875,5 +2916,9 @@
   // We must terminate the debugger in a thread before the C++ destructor
   // chain messes everything up.
   lldb::SBDebugger::Terminate();
+  // Can now safely stop redirecting since lldb will no longer emit stdout
+  // or stderr messages.
+  stdout_redirector.StopRedirecting();
+  stderr_redirector.StopRedirecting();
   return 0;
 }
Index: lldb/tools/lldb-vscode/IOStream.h
===
--- lldb/tools/lldb-vscode/IOStream.h
+++ lldb/tools/lldb-vscode/IOStream.h
@@ -63,6 +63,9 @@
   StreamDescriptor descriptor;
 
   bool write_full(llvm::StringRef str);
+
+  private:
+  std::mutex m_mutex;
 };
 } // namespace lldb_vscode
 
Index: lldb/tools/lldb-vscode/IOStream.cpp

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

[Richard Howell via Phabricator via lldb-commits]

rmaz added a comment.



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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80659



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[António Afonso via Phabricator via lldb-commits]

aadsm added a comment.

Absolutely, I'll look into that first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80659



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80659



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[António Afonso via Phabricator via lldb-commits]

aadsm added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80659



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80659



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[António Afonso via Phabricator via lldb-commits]

aadsm created this revision.
aadsm added reviewers: clayborg, labath, rmaz.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80659

Files:
  lldb/tools/lldb-vscode/lldb-vscode.cpp


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -485,6 +485,31 @@
   }
 }
 
+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();
+  FD_SET(read_fd, );
+  select(read_fd + 1, , nullptr, nullptr, nullptr);
+  if (FD_ISSET(read_fd, )) {
+read(read_fd, , sizeof(buffer));
+g_vsc.SendOutput(OutputType::Console, buffer);
+  }
+}
+  }).detach();
+
+  return new_fd;
+}
+
 // "AttachRequest": {
 //   "allOf": [ { "$ref": "#/definitions/Request" }, {
 // "type": "object",
@@ -2818,9 +2843,10 @@
   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;


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -485,6 +485,31 @@
   }
 }
 
+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();
+  FD_SET(read_fd, );
+  select(read_fd + 1, , nullptr, nullptr, nullptr);
+  if (FD_ISSET(read_fd, )) {
+read(read_fd, , sizeof(buffer));
+g_vsc.SendOutput(OutputType::Console, buffer);
+  }
+}
+  }).detach();
+
+  return new_fd;
+}
+
 // "AttachRequest": {
 //   "allOf": [ { "$ref": "#/definitions/Request" }, {
 // "type": "object",
@@ -2818,9 +2843,10 @@
   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;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits