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

[Serhiy Redko via Phabricator via lldb-commits]

serhiy.redko planned changes to this revision.
serhiy.redko added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94997