[Lldb-commits] [PATCH] D84974: [WIP] Enable Launching the Debugee in VSCode Terminal

[walter erquinigo via Phabricator via lldb-commits]

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

This is very good for a first implementation. You need to write a test for this as well. Once this works, you have to work on the launcher helper Greg and I mentioned to you that will help you guarantee you can always attach to the target.

Regarding the test, you have to build the python code that will mimic the runInTerminal reverse request, along with its response, which will just launch the debuggee inside a terminal.

For launching a process inside a shell from python, you can use the subprocess.run function and pass the shell=True attribute.

For adding the runInTerminal reverse request in python, take a look at the file packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py. You can see examples like request_launch, where it sends data and waits for a response using the send_recv method. You should add support for reverse requests.



================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1361
   body.try_emplace("exceptionBreakpointFilters", std::move(filters));
+  // The debug adapter supports launching a debugee in intergrated VScode terminal.
+  body.try_emplace("supportsRunInTerminalRequest", true);
----------------
s/VScode/VSCode


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1494
 
-  // Instantiate a launch info instance for the target.
-  auto launch_info = g_vsc.target.GetLaunchInfo();
-
-  // Grab the current working directory if there is one and set it in the
-  // launch info.
-  const auto cwd = GetString(arguments, "cwd");
-  if (!cwd.empty())
-    launch_info.SetWorkingDirectory(cwd.data());
-
-  // Extract any extra arguments and append them to our program arguments for
-  // when we launch
-  auto args = GetStrings(arguments, "args");
-  if (!args.empty())
-    launch_info.SetArguments(MakeArgv(args).data(), true);
-
-  // Pass any environment variables along that the user specified.
-  auto envs = GetStrings(arguments, "env");
-  if (!envs.empty())
-    launch_info.SetEnvironmentEntries(MakeArgv(envs).data(), true);
-
-  auto flags = launch_info.GetLaunchFlags();
-
-  if (GetBoolean(arguments, "disableASLR", true))
-    flags |= lldb::eLaunchFlagDisableASLR;
-  if (GetBoolean(arguments, "disableSTDIO", false))
-    flags |= lldb::eLaunchFlagDisableSTDIO;
-  if (GetBoolean(arguments, "shellExpandArguments", false))
-    flags |= lldb::eLaunchFlagShellExpandArguments;
-  const bool detatchOnError = GetBoolean(arguments, "detachOnError", false);
-  launch_info.SetDetachOnError(detatchOnError);
-  launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug |
-                             lldb::eLaunchFlagStopAtEntry);
-
-  // Run any pre run LLDB commands the user specified in the launch.json
-  g_vsc.RunPreRunCommands();
-  if (launchCommands.empty()) {
-    // Disable async events so the launch will be successful when we return from
-    // the launch call and the launch will happen synchronously
-    g_vsc.debugger.SetAsync(false);
-    g_vsc.target.Launch(launch_info, error);
-    g_vsc.debugger.SetAsync(true);
+  if (GetBoolean(arguments, "launchInTerminal", false)) {
+    llvm::json::Object reverseRequest;
----------------
move the entire contents of this if to another function for readability


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1498
+    reverseRequest.try_emplace("command", "runInTerminal");
+    reverseRequest.try_emplace("seq", 100);
+    llvm::json::Object runInTerminalArgs;
----------------
does it work if you don't specify the "seq" number? If it works without it, I would leave it empty


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1501
+    runInTerminalArgs.try_emplace("kind", "integrated");
+    runInTerminalArgs.try_emplace("cwd", GetString(arguments, "cwd"));
+    std::vector<std::string> commands = GetStrings(arguments, "args");
----------------
what if cwd is not specified in the arguments? I imagine this might break or at least the IDE wouldn't be able to launch the target properly. 
If "cwd" is not defined, then use `llvm::sys::fs::current_path()` instead. Does this make sense @clayborg ?


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1507-1510
+    for(std::string envVar : envVars) {
+      size_t ind = envVar.find("=");
+      environment.try_emplace(envVar.substr(0, ind), envVar.substr(ind+1));
+    }
----------------
run clang-format


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1514
+    g_vsc.SendJSON(llvm::json::Value(std::move(reverseRequest)));
+    // g_vsc.SendJSON(llvm::json::Value(std::move(response)));
   } else {
----------------
remove this comment


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1514
+    g_vsc.SendJSON(llvm::json::Value(std::move(reverseRequest)));
+    // g_vsc.SendJSON(llvm::json::Value(std::move(response)));
   } else {
----------------
wallace wrote:
> remove this comment
shouldn't you return at this point and not execute anything that happens after this?
The lines 
    g_vsc.SendJSON(llvm::json::Value(std::move(response)));
    SendProcessEvent(Launch); // Maybe change this for Attach as you'll end up actually doing an attach
    g_vsc.SendJSON(llvm::json::Value(CreateEventObject("initialized")));
probably should only be sent after you were able to attach.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1576
+}
+void response_runInTerminal(const llvm::json::Object &reverseRequest) {
+  g_vsc.is_attach = true;
----------------
mention here that VSCode doesn't respond back the pid of the target, so we can only attach by process name, which is already set up in the request_launch method


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1592
     response["success"] = llvm::json::Value(false);
     EmplaceSafeString(response, "message", std::string(error.GetCString()));
   }
----------------
what happens if it fails to attach? Try running a program that dies right away and that lldb can't attach. Does the debug session terminate correctly?


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1598
+    g_vsc.SendJSON(CreateEventObject("initialized"));
+    // SendThreadStoppedEvent();
+  }
----------------
remove this


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3028
+      const auto command = GetString(object, "command");
+      auto handler_pos = response_handlers.find("runInTerminal");
+      if (handler_pos != response_handlers.end()) {
----------------
you should use the `command` here, instead of the hardcoded string "runInTerminal"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84974