[Lldb-commits] [PATCH] D94997: [lldb][lldb-vscode] Updated implementation of 'launch' and 'attach' requests to not create auxiliary target in case "launchCommands" and "attachCommands" are provided.

[Greg Clayton via Phabricator via lldb-commits]

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

We have iterated on this patch and talked about the ramifications prior to posting this public patch. I will start with a little history:

I added "launchCommands" a long time ago to allow people to do custom commands to start a debug session. Back in those days, we would always create a target using all of the settings from the "launch" request arguments (program, args, environment, many other settings) and then the user could chose to use this target, or create their own in "launchCommands". The target that was selected after running "launchCommands" would be the one that would end up being used for the debug sessions in VS code. This allowed users to use the auto created target to run their debug session. This worked well for cases like GDB remote where you create a local target using local copies of the executable files and shared libraries, then you just run "gdb-remote <host>:<port>" and you could end up having LLDB launch the session for you. The user is now required to create a target manually and set all launch options correctly in the "process launch". So many options in the "launch" request args might be silently ignored now. We should be returning an error of any such options are set, but some of these options are required by all debug adaptors, so it won't be possible to not supply some things like "program", but any options like "args", "env", "stopOnEntry", "disableASLR", "disableSTDIO", "shellExpandArguments", "detachOnError" should produce an error or a warning to let the user know those args shouldn't be specified _if_ "launchCommands" is used since they will be ignored.

I do agree that creating this target automatically is causing problems with the target settings. To understand why this is happening, we must know how target settings work:

- If no targets exist yet, any "settings set target.XXX" calls will be set in the global target settings. Any targets that are created always get a copy of the global target settings
- if a target exists, the currently selected target will get its settings modified by any "settings set target.XXX" calls.

So if you do this in LLDB:

  (lldb) settings set target.foo bar
  (lldb) target create a.out

the "a.out" target will get the settings that were set before it because the settings were set in the global target settings when there was no target, and when the "a.out" target is created, it will copy the global settings.

If we have a launch configuration like found below and we run this in the current lldb-vscode:

  {
    "type": "lldb-vscode",
    "request": "launch",
    "name": "a.out custom launch",
    "program": "/tmp/a.out",
    "launchCommands": ["settings set target.foo bar", "target create /tmp/b.out"]
  }

we get lldb-vscode doing the equivalent of:

  (lldb) target create /tmp/a.out
  (lldb) settings set target.foo bar
  (lldb) target create /tmp/b.out

And the "settings set" applies to the "/tmp/a.out" target since there is a target and it is selected and the "/tmp/b.out" target gets a copy of the global target settings (which are all the default settings). So I can see the benefit of not creating a target automatically due to this "settings set" issue that was created by having lldb-vscode auto create a target all the time. But I also liked the ability to have a target created for you so you can just have your "launchCommands" be something simple like ["gdb-remote <host>:<port>"]

"attachCommands" was added later by open source contributors, and I don't have any objections to not creating a target for this one.

My original suggestion to fixing the "settings set target.XXX" issue was to have any such settings be specified in the "initCommands" array of commands as these commands get run _before_ any targets are created. To fix the issue above the launch configuration can change to:

  {
    "type": "lldb-vscode",
    "request": "launch",
    "name": "a.out custom launch",
    "program": "/tmp/a.out",
    "initCommands": ["settings set target.foo bar"]
    "launchCommands": ["target create /tmp/b.out"]
  }

This would be equivalent to:

  (lldb) settings set target.foo bar
  (lldb) target create /tmp/a.out
  (lldb) target create /tmp/b.out

Since the "settings set target.foo bar" gets run before any targets get created, the global target settings are modified. Any new target, like the "/tmp/a.out" target (auto created from "program" arg) and "/tmp/b.out" target (from "launchCommands") get a copy of the global settings.

So I agree we need to fix the "settings set" issue as it surfaces a quirk in the order and number of targets we create. The main questions is if we care that we don't auto create a target when "launchCommands" are used and what the right solution is for lldb-vscode going forward.

So if we "launchCommands" or "attachCommands" cause any launch config arguments to be ignored, then we should return an error to the "launch" or "attach" requests, or emit an warning to the debug console at the very least.



================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:582
+  if (!attachCommands.empty()) {
+    // We have "attachCommands" that are a set of commands that are expected
+    // to execute the commands after which a process should be created. If there
----------------
We need to check if any arguments are set in the launch config that "attachCommands" will ignore and return an error if any of them are set. Or we need to emit an error or warning to the debug console so the users can know that these settings are now ignored because "attachCommands" will cause them to be.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1558
+  if (!launchCommands.empty()) {
+    // if "launchCommands" are provided, then they are expected to make the
+    // launch happen for launch requests and they replace the normal logic that
----------------
We need to check if any arguments are set in the launch config that "launchCommands" will ignore and return an error if any of them are set. Or we need to emit an error or warning to the debug console so the users can know that these settings are now ignored because "launchCommands" will cause them to be.


================
Comment at: lldb/tools/lldb-vscode/package.json:190
 								"type": "string",
-								"description": "Path to the program to attach to."
+								"description": "Path to the program to attach to. Ignored if \"attachCommands\" is provided."
 							},
----------------
No need to change this string, just clarify what will happen in the "attachCommands" description.


================
Comment at: lldb/tools/lldb-vscode/package.json:197
 								],
-								"description": "System process ID to attach to."
+								"description": "System process ID to attach to. Ignored if \"attachCommands\" is provided."
 							},
----------------
No need to change this string, just clarify what will happen in the "attachCommands" description.


================
Comment at: lldb/tools/lldb-vscode/package.json:201
 								"type": "boolean",
-								"description": "If set to true, then wait for the process to launch by looking for a process with a basename that matches `program`. No process ID needs to be specified when using this flag.",
+								"description": "If set to true, then wait for the process to launch by looking for a process with a basename that matches `program`. No process ID needs to be specified when using this flag. Ignored if \"attachCommands\" is provided.",
 								"default": true
----------------
No need to change this string, just clarify what will happen in the "attachCommands" description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94997