Page MenuHomePhabricator

[lldb-vscode] capture the debuggers file handles before lldbinit runs
Needs RevisionPublic

Authored by aadsm on Dec 1 2019, 8:47 PM.

Details

Summary

We need to do this before we create the debugger because the commands run in the lldbinit might do output to these handlers as well.

This is still an issue when lldb-vscode is run in stdout/stdin mode, not sure how to address this to be honest, we would need to both capture the stdout (that comes from the script interpreter) and wrap it using the stdout category. Maybe I should post this to the lldb-dev ML.

Event Timeline

aadsm created this revision.Dec 1 2019, 8:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2019, 8:47 PM
aadsm edited the summary of this revision. (Show Details)Dec 1 2019, 8:54 PM
clayborg requested changes to this revision.Dec 2 2019, 9:14 AM

Would be good to write a test for this where the init file does "script print('hello')" which would hose up the connection

This revision now requires changes to proceed.Dec 2 2019, 9:14 AM
clayborg added inline comments.Dec 2 2019, 9:24 AM
lldb/tools/lldb-vscode/lldb-vscode.cpp
1207–1210

Are we allows to add extra "initialize" arguments here? How and who will ever pass this to lldb-vscode? Is there a way to make Microsoft VS Code pass this? If not, then just use an environment variable instead?

aadsm added a comment.Dec 2 2019, 11:01 AM

The way phabricator shows the diff is tricky, this change has nothing to do with D70882 and stands by itself. The important part here is making the g_vsc.debugger.SetOutputFileHandle(out, true); g_vsc.debugger.SetErrorFileHandle(out, false); happen before the debugger creation. Not sure how to create a test for this though since there's no mechanism to give lldb-vscode an initial file to load...

labath added a subscriber: labath.Dec 3 2019, 12:53 AM

The way phabricator shows the diff is tricky, this change has nothing to do with D70882 and stands by itself. The important part here is making the g_vsc.debugger.SetOutputFileHandle(out, true); g_vsc.debugger.SetErrorFileHandle(out, false); happen before the debugger creation. Not sure how to create a test for this though since there's no mechanism to give lldb-vscode an initial file to load...

Maybe you could do something similar to LocalLLDBInit.test ?

aadsm added a comment.Dec 3 2019, 12:17 PM

Maybe you could do something similar to LocalLLDBInit.test ?

That test uses the lldb -S (and others) flags that lldb-vscode doesn't support :(. These flags should really be added to the initialize packet but they're very specific to lldb and the DAP doesn't support it.. We could implement something like what Driver::ProcessArgs does and pass options through envs but, the logic in ProcessArgs itself is sketchy (just like the comment mentions) and I l also find it odd to pass options through env vars...

We could just add a new flag to lldb-vscode like "--no-lldb-init" and always pass that when we run our test suite?

Maybe you could do something similar to LocalLLDBInit.test ?

That test uses the lldb -S (and others) flags that lldb-vscode doesn't support :(. These flags should really be added to the initialize packet but they're very specific to lldb and the DAP doesn't support it.. We could implement something like what Driver::ProcessArgs does and pass options through envs but, the logic in ProcessArgs itself is sketchy (just like the comment mentions) and I l also find it odd to pass options through env vars...

I was specifically referring to the part where the test sets up an .lldbinit in a random directory and then points the HOME environment variable there in order to "trick" lldb into executing it. It's true that the test also uses -S/--source-before-file, but nothing in there is really needed to test this functionality (i.e., making sure that lldb-vscode handles the lldbinit file reasonably -- making sure that it does not get confused by ambient lldbinit files is a different topic).

As for -no-lldbinit functionality, all of the proposed solutions (cmdline flag, environment variable, protocol flag...) seem reasonable to me. The environment variable is probably easiest to implement, and should be sufficient if we only want to use this for testing....