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

Wed Jan 20 14:31:56 PST 2021

clayborg added a comment.

In D94997#2510144 <https://reviews.llvm.org/D94997#2510144>, @serhiy.redko wrote:

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94997