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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
