This is an archive of the discontinued LLVM Phabricator instance.

[lldb-vscode] Reopen stderr as /dev/null
AbandonedPublic

Authored by rmaz on May 26 2020, 10:42 AM.

Details

Reviewers
clayborg
labath
Summary

When initializing LLDB it can generate warnings which get written to stderr. As the stderr pipe is not being read this will block once the pipe buffer is full. Reopen stderr as /dev/null to prevent write blocking.

Diff Detail

Event Timeline

rmaz created this revision.May 26 2020, 10:42 AM
clayborg added a subscriber: aadsm.May 26 2020, 5:44 PM

It's not the end of the world, but it certainly seems like it would be better if this would be handled on the other end ( == having VSCode redirecting the stderr to /dev/null)

IIUC, you're saying that VSCode deliberately sets up a stderr pipe when launching lldb-vscode, but then never bothers to read from it. Is that correct? It seems like a pretty odd thing to do... Did you look at why it does that?

lldb/tools/lldb-vscode/lldb-vscode.cpp
2783–2784

The right way to do this is via freopen("/dev/null", "w", stderr). And on windows, I think, this should be "nul" instead of "/dev/null".

I don't think we should ignore stderr this at this level, we should either do what Pavel suggests or send the stderr back through the protocol. I actually have some code that does this, it outputs both stderr and stdout to the console message. I did this because I want to be able to see in the vscode's debugger console what I see when running lldb in the command line and also because any text sent by lldb (e.g.: python interpreter) to the stdout will ruin the protocol communication. Let me put my code up in a diff.

So this brings up the question of what to do with any unsolicited STDOUT/STDERR. One idea that we floated around before was to immediately dup the original STDIN/OUT/ERR to new file descriptors, adopt the duped stdin/stdout using g_vsc.input and g_vsc.output, then close stdin, stdout, stderr and create make new file handles for stdout/stderr and listen to them with a thread and pass any output from these to the "output" console stream. This way any warnings or errors are not just dropped on the floor, and yet if we dupe the file descriptors for stdin/out, then we won't run into any stdout/stderr ruining out packet traffic. Pavel, thoughts on what the best approach is?

lldb/tools/lldb-vscode/lldb-vscode.cpp
2780–2784

This should be moved to line 2830 in this patch, or 2824 in the original source. If we attach with a port, then we don't need to worry about this.

rmaz added a comment.May 27 2020, 2:07 PM

I don't think we should ignore stderr this at this level, we should either do what Pavel suggests or send the stderr back through the protocol.

This is definitely a nicer solution, although in our case would result in a large number of unhelpful warnings every time we attached the debugger. Happy to go with this approach instead.

So this brings up the question of what to do with any unsolicited STDOUT/STDERR. One idea that we floated around before was to immediately dup the original STDIN/OUT/ERR to new file descriptors, adopt the duped stdin/stdout using g_vsc.input and g_vsc.output, then close stdin, stdout, stderr and create make new file handles for stdout/stderr and listen to them with a thread and pass any output from these to the "output" console stream. This way any warnings or errors are not just dropped on the floor, and yet if we dupe the file descriptors for stdin/out, then we won't run into any stdout/stderr ruining out packet traffic. Pavel, thoughts on what the best approach is?

I think that Antonio's solution (with a fair amount of cleanup) would be fine. However, I'm still puzzled by the vscode behavior. I find it hard to believe that lldb is the only process which outputs random blurbs to stderr. Leaving applications which do that to hang is not very nice. If vscode does not intend to read from stderr, *it* should redirect it to /dev/null.

rmaz added a comment.EditedMay 28 2020, 9:12 AM

I think that Antonio's solution (with a fair amount of cleanup) would be fine. However, I'm still puzzled by the vscode behavior. I find it hard to believe that lldb is the only process which outputs random blurbs to stderr. Leaving applications which do that to hang is not very nice. If vscode does not intend to read from stderr, *it* should redirect it to /dev/null.

Managed to find the code responsible, its here: https://github.com/microsoft/vscode/blob/master/src/vs/workbench/contrib/debug/node/debugAdapter.ts#L207

From the docs:

By default, pipes for stdin, stdout, and stderr are established between the parent Node.js process and the spawned child. These pipes have limited (and platform-specific) capacity. If the child process writes to stdout in excess of that limit without the output being captured, the child process will block waiting for the pipe buffer to accept more data

However it does seem that there is a stderr callback added on https://github.com/microsoft/vscode/blob/master/src/vs/workbench/contrib/debug/node/debugAdapter.ts#L234, so now I'm confused as to why this blocking is occurring. I suppose this is only possible if the debug adapter has not outputService, but not sure what controls that.

It looks like outputService is always undefined, so that would explain it. Does seem like the output should be redirected in that case instead, i'll put up a PR for this instead.

rmaz abandoned this revision.May 28 2020, 9:35 AM