[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D70886#1767654 , @aadsm wrote:

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70886



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

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70886



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

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

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

aadsm added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70886



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

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D70886#1765629 , @aadsm wrote:

> 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 

 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70886



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

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

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

aadsm added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70886



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

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Abandon this one and modify https://reviews.llvm.org/D70882?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70886



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

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1207-1210
+  auto arguments = request.getObject("arguments");
+  const auto skipInitFiles = GetBoolean(arguments, "skipInitFiles", false);
+  g_vsc.debugger =
+  lldb::SBDebugger::Create(!skipInitFiles /*source_init_files*/);

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70886



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

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70886



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

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

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

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

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70886

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
@@ -1194,14 +1194,6 @@
 //   }]
 // }
 void request_initialize(const llvm::json::Object &request) {
-  auto arguments = request.getObject("arguments");
-  const auto skipInitFiles = GetBoolean(arguments, "skipInitFiles", false);
-  g_vsc.debugger =
-  lldb::SBDebugger::Create(!skipInitFiles /*source_init_files*/);
-  // Create an empty target right away since we might get breakpoint requests
-  // before we are given an executable to launch in a "launch" request, or a
-  // executable when attaching to a process by process ID in a "attach"
-  // request.
   FILE *out = llvm::sys::RetryAfterSignal(nullptr, fopen, dev_null_path, "w");
   if (out) {
 // Set the output and error file handles to redirect into nothing otherwise
@@ -1212,6 +1204,14 @@
 g_vsc.debugger.SetErrorFileHandle(out, false);
   }
 
+  auto arguments = request.getObject("arguments");
+  const auto skipInitFiles = GetBoolean(arguments, "skipInitFiles", false);
+  g_vsc.debugger =
+  lldb::SBDebugger::Create(!skipInitFiles /*source_init_files*/);
+  // Create an empty target right away since we might get breakpoint requests
+  // before we are given an executable to launch in a "launch" request, or a
+  // executable when attaching to a process by process ID in a "attach"
+  // request.
   g_vsc.target = g_vsc.debugger.CreateTarget(nullptr);
   lldb::SBListener listener = g_vsc.debugger.GetListener();
   listener.StartListeningForEvents(


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -1194,14 +1194,6 @@
 //   }]
 // }
 void request_initialize(const llvm::json::Object &request) {
-  auto arguments = request.getObject("arguments");
-  const auto skipInitFiles = GetBoolean(arguments, "skipInitFiles", false);
-  g_vsc.debugger =
-  lldb::SBDebugger::Create(!skipInitFiles /*source_init_files*/);
-  // Create an empty target right away since we might get breakpoint requests
-  // before we are given an executable to launch in a "launch" request, or a
-  // executable when attaching to a process by process ID in a "attach"
-  // request.
   FILE *out = llvm::sys::RetryAfterSignal(nullptr, fopen, dev_null_path, "w");
   if (out) {
 // Set the output and error file handles to redirect into nothing otherwise
@@ -1212,6 +1204,14 @@
 g_vsc.debugger.SetErrorFileHandle(out, false);
   }
 
+  auto arguments = request.getObject("arguments");
+  const auto skipInitFiles = GetBoolean(arguments, "skipInitFiles", false);
+  g_vsc.debugger =
+  lldb::SBDebugger::Create(!skipInitFiles /*source_init_files*/);
+  // Create an empty target right away since we might get breakpoint requests
+  // before we are given an executable to launch in a "launch" request, or a
+  // executable when attaching to a process by process ID in a "attach"
+  // request.
   g_vsc.target = g_vsc.debugger.CreateTarget(nullptr);
   lldb::SBListener listener = g_vsc.debugger.GetListener();
   listener.StartListeningForEvents(
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits