Page MenuHomePhabricator

[lldb][lldb-vscode] Updated implementation of 'launch' and 'attach' requests to not create auxiliary target in case "launchCommands" and "attachCommands" are provided.
Changes PlannedPublic

Authored by serhiy.redko on Jan 19 2021, 12:51 PM.

Details

Summary

launch.json's fields have ambiguity with how final target is created: 

  1. There are dedicated fields to specify target like "program", "pid", "waitFor"
  2. There are  "launchCommands" and "attachCommands" fields that might or might not create a target.

Current implementation for handling of "launchCommands" and "attachCommands" implies
they are rather mutually exclusive with other target defining parameters like "program", "pid" etc
in case they create a new target. 

So user can provide in the launch.json fields "program", "pid", "core_file" and these parameters 
will be silently ignored by implementation in case "launchCommands" and "attachCommands" are provided
as well and create a new target. For attach "core_file" is ignored in case "attachCommands"
field is just provided, no matter whether commands create or not target.
At the same time, implementation always creates target by means of CreateTargetFromArguments()
API in expectance of target dedicated fields in launch.json like "program", "pid", even in case 
"launchCommands" and "attachCommands" (that do create a target) are provided.

Creation of this first, "auxilary" target, introduces issue with target settings specified for second
target and created by "launchCommands" or "attachCommands": target settings for the second target are 'hijacked'
by target created by CreateTargetFromArguments() call and not applied to target created 
by "launchCommands"/"attachCommands".

For example, for "launchCommands":

settings set target.exec-search-paths /some/path 
target create /some/file

target.exec-search-paths is not applied to the final target created in "launchCommands".  
The settings is applied to previously created "auxilary" target which doesn't become selected.
This breaks debugging of the target created by "launchCommands"/"attachCommands".

A possible workaround for the issue would be a strict requirement to set target settings
after target creation command, however it might not be feasible for some settings and 
inconsistent with cli lldb usage results where this issue is not reproduced.

This change avoids creation of "auxilary" target in case "launchCommands"/"attachCommands" 
are provided. It also uses proper SetTarget() setter to set final target and subscribe it
for notifications.

This change potentially can break the case when user provides "launchCommands"/"attachCommands"
that don't create a target, but rely on the "auxilary" target that they can tune further by commands
in "launchCommands", "attachCommands". This doesn't look like a correct usage intent to me
because it requires from users quite thorough inspection of 'launch'/'attach' requests implementation and
understanding which launch.json fields are ignored or not by implementation.

In case user needs to tune created target I would recommend to create new "postTargetCommands" launch.json
field with clear instructions to not create target and assume that target already exists.

Diff Detail

Event Timeline

serhiy.redko requested review of this revision.Jan 19 2021, 12:51 PM
serhiy.redko created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2021, 12:51 PM
clayborg requested changes to this revision.Jan 19 2021, 2:00 PM

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.

lldb/tools/lldb-vscode/lldb-vscode.cpp
582

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.

1558

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.

lldb/tools/lldb-vscode/package.json
190

No need to change this string, just clarify what will happen in the "attachCommands" description.

197

No need to change this string, just clarify what will happen in the "attachCommands" description.

201

No need to change this string, just clarify what will happen in the "attachCommands" description.

This revision now requires changes to proceed.Jan 19 2021, 2:00 PM

Another solution would be to not auto create a target if the "program" argument is missing from the launch config and document this in the "attachCommands" or "launchCommands"? Or is "program" a required launch config argument?

D92164 was intended for fixing the "settings set" issue, however, it revealed some deadlocks and data races, and had to be reverted temporarily. Currently, I'm working on those multithreading issues and will submit a patch as soon as possible.

serhiy.redko planned changes to this revision.Jan 20 2021, 9:54 AM

Thanks for review and your input, @clayborg

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.

Please correct me if I'm wrong, but my understanding is: from lldb-vscode launch.json configuration we generally should create a single target/process which user will debug.
The fact that 'debugger' instance has several targets after 'launch', 'attach' doesn't look correct to me, while things still work. It introduces ambiguity and conundrum in implementation what exactly user wants to debug.
Do we have use cases for several targets in single lldb-vscode session? If we need to debug several processes, my understanding is we need to launch several lldb-vscode instances for each process to debug, is it correct?
So ideally from launch.json we should construct single target and use it.

So I think it is important for user to have a clear understanding about the way the debug target is created.
Now it can be created with:

  1. 'convenience' fields like "program", "pid"..,
  2. 'attachCommands'/'launchCommands'
  3. potentially with any other fields that execute lldb commands e.g "initCommands"

I assume all we want is to get clear instructions from user about final target, but not a mix of all ways listed above to create target and use last to work with. Otherwise it will be confusion like why target I specified with my "program" field is ignored when I provided 'launchCommands' that also create target?

With this change I try to resolve ambiguity for user and implementation on the approach to create the debug target and don't break many existing use cases.

So I suggest to document and 'enforce' the following:

  1. 'convenience' fields like "program", "pid".. for which we create auto target because eventually we need a target.
  2. 'attachCommands'/'launchCommands' where user will be responsible to provide LLDB commands that will create a target. No auto target is needed.

#1 and #2 will be stated as mutually exclusive in documentation and made in implementation.

I'm not sure we can 'enforce' user to not create target in "initCommands" (unless we assert(debugger.GetTargetsNum() == 1)) and other similar launch.json fields but it should be easy to document.

Another solution would be to not auto create a target if the "program" argument is missing from the launch config and document this in the "attachCommands" or "launchCommands"? Or is "program" a required launch config argument?

While "program" is listed as required in launch.json it is not required in case 'launchCommands' create a target, so we can make it optional.
Since there are many target creating fields like "program" with described changes it will be easier to check whether 'attachCommands'/'launchCommands' are provided and avoid auto target creation.

I agree that we need to do sanitization of input parameters and warn about ambiguity. Since "program" is listed as required, I think some clients, that use 'launchCommands' are passing dummy value for "program" (exactly what our current tests are doing) and the "program" is ignored eventually. To not break such clients, probably, it would be better to log warnings vs stop with an error. What do you think?

aadsm added inline comments.Jan 20 2021, 10:45 AM
lldb/tools/lldb-vscode/lldb-vscode.cpp
1558

I vote for a warning here otherwise it might break people's current setups, assuming the error is an indication that the launch won't happen, but we should totally do it.

Thanks for review and your input, @clayborg

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.

Please correct me if I'm wrong, but my understanding is: from lldb-vscode launch.json configuration we generally should create a single target/process which user will debug.

It just needs to debug one of the targets. My comments below detail why it was created this way, for better or worse.

The fact that 'debugger' instance has several targets after 'launch', 'attach' doesn't look correct to me, while things still work. It introduces ambiguity and conundrum in implementation what exactly user wants to debug.
Do we have use cases for several targets in single lldb-vscode session?

We do not currently. The only reason the target was created when using "launchCommands" was to be able to have it just be one command:

"launchCommands": ["gdb-remote <host>:<port>"]

I created "launchCommands" for this very case. That being said, it doesn't need to stay this way, just an explanation of why the target is created currently. Then someone added "attachCommands" and copied what "launchCommands" was doing, including the creation of the target. You could easily use the pre-created target with "attachCommands":

"attachCommands": ["process attach --waitfor"]

Since the current stuff creates a target for you with the "program", it would know to use the program's path as the name of the process to attach to. Again, that is just how it is currently coded and we are here to fix this in this patch.

If we need to debug several processes, my understanding is we need to launch several lldb-vscode instances for each process to debug, is it correct?

We don't need them, it was just out I originally coded "launchCommands" because my primary use case was for attaching to a remote GDB server for android apps.

So ideally from launch.json we should construct single target and use it.

A single target will solve any "settings set target.XXXX" issues, so I am fine with any solution that can create one target.

So I think it is important for user to have a clear understanding about the way the debug target is created.
Now it can be created with:

  1. 'convenience' fields like "program", "pid"..,
  2. 'attachCommands'/'launchCommands'
  3. potentially with any other fields that execute lldb commands e.g "initCommands"

A target can be created with "initCommands" but should be discouraged. Maybe we need to add something to the description for "initCommands" in package.json

I assume all we want is to get clear instructions from user about final target, but not a mix of all ways listed above to create target and use last to work with. Otherwise it will be confusion like why target I specified with my "program" field is ignored when I provided 'launchCommands' that also create target?

Yes, we should figure out the best solution that makes sense as I agree that the way it is coded right now has issues.

With this change I try to resolve ambiguity for user and implementation on the approach to create the debug target and don't break many existing use cases.

So I suggest to document and 'enforce' the following:

  1. 'convenience' fields like "program", "pid".. for which we create auto target because eventually we need a target.
  2. 'attachCommands'/'launchCommands' where user will be responsible to provide LLDB commands that will create a target. No auto target is needed.

#1 and #2 will be stated as mutually exclusive in documentation and made in implementation.

I'm not sure we can 'enforce' user to not create target in "initCommands" (unless we assert(debugger.GetTargetsNum() == 1)) and other similar launch.json fields but it should be easy to document.

It might be nice to check and log a warning to the console if the does this, but yes, hard to enforce and we don't want to take away someone's ability to do what they need to to get something working.

Another solution would be to not auto create a target if the "program" argument is missing from the launch config and document this in the "attachCommands" or "launchCommands"? Or is "program" a required launch config argument?

While "program" is listed as required in launch.json it is not required in case 'launchCommands' create a target, so we can make it optional.
Since there are many target creating fields like "program" with described changes it will be easier to check whether 'attachCommands'/'launchCommands' are provided and avoid auto target creation.

I agree that we need to do sanitization of input parameters and warn about ambiguity. Since "program" is listed as required, I think some clients, that use 'launchCommands' are passing dummy value for "program" (exactly what our current tests are doing) and the "program" is ignored eventually. To not break such clients, probably, it would be better to log warnings vs stop with an error. What do you think?

Logging to the console is fine with me. As long as there is some feedback to the user that these arguments are not being used I am fine.

Anyone else have any opinions on this?

D92164 was intended for fixing the "settings set" issue, however, it revealed some deadlocks and data races, and had to be reverted temporarily. Currently, I'm working on those multithreading issues and will submit a patch as soon as possible.

This settings set issue is not related to your diff, it is just because a target is created, the auto created target gets the settings set on it, and any new target created by "launchCommands" or "attachCommands" would not due to the way settings work in general.