Page MenuHomePhabricator

[lldb-vscode] redirect stderr/stdout to the IDE's console
ClosedPublic

Authored by wallace on Apr 6 2021, 10:29 AM.

Details

Summary

In certain occasions times, like when LLDB is initializing and evaluating the .lldbinit files, it tries to print to stderr and stdout directly. This confuses the IDE with malformed data, as it talks to lldb-vscode using stdin and stdout following the JSON RPC protocol. This ends up terminating the debug session with the user unaware of what's going on. There might be other situations in which this can happen, and they will be harder to debug than the .lldbinit case.

After several discussions with @clayborg, @yinghuitan and @aadsm, we realized that the best course of action is to simply redirect stdout and stderr to the console, without modifying LLDB itself. This will prove to be resilient to future bugs or features.

I made the simplest possible redirection logic I could come up with. It only works for POSIX, and to make it work with Windows should be merely changing pipe and dup2 for the windows equivalents like _pipe and _dup2. Sadly I don't have a Windows machine, so I'll do it later once my office reopens, or maybe someone else can do it.

I'm intentionally not adding a stop-redirecting logic, as I don't see it useful for the lldb-vscode case (why would we want to do that, really?).

I added a test.

Note: this is a simpler version of D80659. I first tried to implement a RIIA version of it, but it was problematic to manage the state of the thread and reverting the redirection came with some non trivial complexities, like what to do with unflushed data after the debug session has finished on the IDE's side.

Diff Detail

Event Timeline

wallace created this revision.Apr 6 2021, 10:29 AM
wallace requested review of this revision.Apr 6 2021, 10:29 AM
wallace edited the summary of this revision. (Show Details)Apr 6 2021, 10:42 AM
wallace edited the summary of this revision. (Show Details)Apr 6 2021, 10:49 AM
wallace edited the summary of this revision. (Show Details)
clayborg requested changes to this revision.Apr 6 2021, 4:05 PM

Just need to fix the default python arguments that were: "foo={}". This doesn't do what you think it will do. See https://docs.quantifiedcode.com/python-anti-patterns/correctness/mutable_default_value_as_argument.html

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
12

You must use None instead of {} otherwise all callers to this function will share the default dictionary

21

You must use None instead of {} otherwise all callers to this function will share the default dictionary

346–347

You must use None instead of {} otherwise all callers to this function will share the default dictionary

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
931

You must use None instead of {} otherwise all callers to this function will share the default dictionary

This revision now requires changes to proceed.Apr 6 2021, 4:05 PM
wallace updated this revision to Diff 335854.Apr 7 2021, 10:00 AM

Address comments

clayborg requested changes to this revision.Apr 7 2021, 1:44 PM
clayborg added inline comments.
lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
936

Python dictionaries have an update method that does what you want here: it will replace value for any keys that exist in env, and create one that don't exist.

lldb/tools/lldb-vscode/lldb-vscode.cpp
3094–3101

Do we need this code in lldb-vscode? Can't we just run some initCommands that print something from python?

3125–3126

I'd rather not build code into lldb-vscode for testing if we don't have to. You can easily just run "target list" from "initCommands" and it should print out "No targets."

This revision now requires changes to proceed.Apr 7 2021, 1:44 PM
clayborg added inline comments.Apr 7 2021, 2:10 PM
lldb/tools/lldb-vscode/lldb-vscode.cpp
3126

Come to think of it, this is the easiest way to do this. We can't do anything with and "initCommands" because that is too late as the in/out/err has been correctly set for the debugger by that time. We also don't want to have the test move the user's ~/.lldbinit or ~/.lldbinit-lldb-vscode file out of the way and replace it as that is just gnarly. So this is probably the best solution I can think of.

wallace updated this revision to Diff 336946.Apr 12 2021, 1:29 PM

updated minor python detail

clayborg accepted this revision.Tue, Apr 20, 4:15 PM
This revision is now accepted and ready to land.Tue, Apr 20, 4:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Apr 21, 2:24 PM
wallace closed this revision.Wed, Apr 21, 3:06 PM
JDevlieghere added inline comments.Thu, Apr 22, 9:19 PM
lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
939

Should this be LLDB_VSCODE_LOG, similar to LLDB_VSCODE_TEST_STDOUT_STDERR_REDIRECTION? Most of our environment variables begin with LLDB_ so I think that would be most consistent.

wallace added inline comments.Thu, Apr 22, 9:30 PM
lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
939

I agree with you, but it might be hard to change it, as that might break some users' flow. The env var is also used inside of lldb-vscode.cpp itself