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

[serhiy.redko]

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 setting 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.
An error is reported in case debug configuration contains target related keys
("program", "coreFile" etc) along with "launchCommands" or "attachCommands".

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.

Test plan:

./bin/lldb-dotest ../llvm-project/lldb/test/API/tools/lldb-vscode/
...
Ran 53 tests in 99.606s

RESULT: PASSED (48 passes, 0 failures, 0 errors, 4 skipped, 0 expected failures, 0 unexpected successes)

[clayborg]

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.

[clayborg]

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?

[tatyana-krasnukha]

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]

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]

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.

[clayborg]

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

[clayborg]

In D94997#2509175, @tatyana-krasnukha wrote:

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.

[serhiy.redko]

• added printing of unused target related fields from launch.json in case launchCommands/attachCommands fields are provided.
• updated help info in for launch/attach fields according to review request.

[clayborg]

I am fine overall with this change because of goofy things that can happen when we always create a target. We might think about returning an error if the user specifies "launchCommands" or "attachCommands" if the user also includes any of the arguments that will be ignored instead of printing to the console, see inlined comments. I would vote for returning an error as long the the full error string is displayed to the user and the entire string can be read and isn't obfuscated in the launch/attach error dialog. I am not set on this and would be interested to hear what others thing of the error vs showing something in the debugger console

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

627–628

We can't print to stderr or stdout since this is where the VS code DAP packets get delivered.

We have two options here IMHO:

• deliver the warning/error stirng to the debugger console
• return an error with this string as the reason and fail the attach as long as the error string get displayed to the user in the IDE

We can deliver this to the "Debugger Console" using:

std::string str;
llvm::raw_string_ostream strm(str);
strm << ...;
g_vsc.SendOutput(OutputType::Console, strm.str());

1558

I kind of disagree after thinking about this some more. Right now we have many things that can be specified in the launch config that will just get ignored and if the user doesn't check the debugger console, they won't know. It is probably ok to have the launch fail as long as the error string shows up and is readable to the user and is complete enough for the user to read. Thoughts?

1696–1697

use g_vsc.SendOutput(OutputType::Console, ...) as mentioned above or return an error. We will discuss the merits of message vs error in this comments.

[wallace]

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

627–628

do as Greg says and besides that terminate the debug session. This might be an indication of an erroneous configuration

1696–1697

same here, just terminate the session

[serhiy.redko]

• return error in case debug configuration contains incompatible fields with 'launchCommands'/'attachCommands'

[serhiy.redko]

As requested DAP now reports an error in case debug configuration contains unused target related keys
("program", "coreFile" etc) along with "launchCommands" or "attachCommands" that must create target.

[clayborg]

Revert the whitespace only changes to the .py file and good to go as long as all vscode tests are passing.

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py

3

revert whitespace only change

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py

                             "verify '%s' found in console output for '%s'" % (
                                 cmd, flavor))
 
+    def verify_contains_text(self, flavor, output, text):
+        self.assertTrue(output and len(output) > 0, "expect console output")
+        found = text in output
+        self.assertTrue(found,
+                        "verify '%s' found in console output for '%s'" % (
+                            text, flavor))
+
     def get_dict_value(self, d, key_path):
         '''Verify each key in the key_path array is in contained in each
            dictionary within "d". Assert if any key isn't in the

lldb/test/API/tools/lldb-vscode/launch/TestVSCode_launch.py

         '''
         self.build_and_create_debug_adaptor()
         program = self.getBuildArtifact("a.out")
+        execSearchPaths = '/unique/path/to/symbols/launch/test'
 
         source = 'main.c'
         first_line = line_number(source, '// breakpoint 1')

         # also we can verify that "stopCommands" get run as the
         # breakpoints get hit
         launchCommands = [
+            'settings set target.exec-search-paths "%s"' % (execSearchPaths),
             'target create "%s"' % (program),
             'breakpoint s -f main.c -l %d' % first_line,
             'breakpoint s -f main.c -l %d' % second_line,

         initCommands = ['target list', 'platform list']
         preRunCommands = ['image list a.out', 'image dump sections a.out']
         stopCommands = ['frame variable', 'bt']
-        exitCommands = ['expr 2+3', 'expr 3+4']
+        exitCommands = ['expr 2+3', 'expr 3+4', 'settings show target.exec-search-paths']
         self.launch(program,
                     initCommands=initCommands,
                     preRunCommands=preRunCommands,

         # "exitCommands" that were run after the second breakpoint was hit
         output = self.get_console(timeout=1.0)
         self.verify_commands('exitCommands', output, exitCommands)
+        # confirm that output contains correct target.exec-search-paths value
+        self.verify_contains_text('exitCommands', output, execSearchPaths)
 
     @skipIfWindows
     @skipIfNetBSD # Hangs on NetBSD as well

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

   // Run any initialize LLDB commands the user specified in the launch.json
   g_vsc.RunInitCommands();
 
-  lldb::SBError status;
-  g_vsc.SetTarget(g_vsc.CreateTargetFromArguments(*arguments, status));
-  if (status.Fail()) {
-    response["success"] = llvm::json::Value(false);
-    EmplaceSafeString(response, "message", status.GetCString());
-    g_vsc.SendJSON(llvm::json::Value(std::move(response)));
-    return;
-  }
+  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
+    // is no valid process after running these commands, we have failed.
 
-  // Run any pre run LLDB commands the user specified in the launch.json
-  g_vsc.RunPreRunCommands();
+    std::vector<llvm::StringRef> incompatible_fields;
+    for (llvm::StringRef arg : {"program", "pid", "waitFor", "coreFile",
+                                "targetTriple", "platformName"}) {
+      if (arguments->get(arg) != nullptr) {
+        incompatible_fields.push_back(arg);
+      }
+    }
 
-  if (pid == LLDB_INVALID_PROCESS_ID && wait_for) {
-    char attach_msg[256];
-    auto attach_msg_len = snprintf(attach_msg, sizeof(attach_msg),
-                                   "Waiting to attach to \"%s\"...",
-                                   g_vsc.target.GetExecutable().GetFilename());
-    g_vsc.SendOutput(OutputType::Console,
-                     llvm::StringRef(attach_msg, attach_msg_len));
+    if (!incompatible_fields.empty()) {
+      std::string str;
+      llvm::raw_string_ostream strm(str);
+      strm << "Incorrect debug configuration: " <<
+        "'attachCommands' is not compatible with ";
+      for (llvm::StringRef field : incompatible_fields) {
+        strm << " '" << field << "' ";
+      }
+      response["success"] = llvm::json::Value(false);
+      EmplaceSafeString(response, "message", strm.str());
+      g_vsc.SendJSON(llvm::json::Value(std::move(response)));
+      return;
+    }
+
+    // Run any pre run LLDB commands the user specified in the launch.json
+    g_vsc.RunPreRunCommands();
+    g_vsc.RunLLDBCommands("Running attachCommands:", attachCommands);
+    // The custom commands are expected to create a new target
+    if (g_vsc.debugger.GetNumTargets() > 0) {
+      g_vsc.SetTarget(g_vsc.debugger.GetSelectedTarget());
+    }
+    else {
+      error.SetErrorString("attachCommands failed to create target");
+    }
   }
-  if (attachCommands.empty()) {
+  else {
     // No "attachCommands", just attach normally.
+    lldb::SBError status;
+    g_vsc.SetTarget(g_vsc.CreateTargetFromArguments(*arguments, status));
+    if (status.Fail()) {
+      response["success"] = llvm::json::Value(false);
+      EmplaceSafeString(response, "message", status.GetCString());
+      g_vsc.SendJSON(llvm::json::Value(std::move(response)));
+      return;
+    }
+
+    // Run any pre run LLDB commands the user specified in the launch.json
+    g_vsc.RunPreRunCommands();
+
+    if (pid == LLDB_INVALID_PROCESS_ID && wait_for) {
+      char attach_msg[256];
+      auto attach_msg_len = snprintf(attach_msg, sizeof(attach_msg),
+                                    "Waiting to attach to \"%s\"...",
+                                    g_vsc.target.GetExecutable().GetFilename());
+      g_vsc.SendOutput(OutputType::Console,
+                      llvm::StringRef(attach_msg, attach_msg_len));
+    }
+
     // Disable async events so the attach will be successful when we return from
     // the launch call and the launch will happen synchronously
     g_vsc.debugger.SetAsync(false);

       g_vsc.target.LoadCore(core_file.data(), error);
     // Reenable async events
     g_vsc.debugger.SetAsync(true);
-  } else {
-    // 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
-    // is no valid process after running these commands, we have failed.
-    g_vsc.RunLLDBCommands("Running attachCommands:", attachCommands);
-    // The custom commands might have created a new target so we should use the
-    // selected target after these commands are run.
-    g_vsc.target = g_vsc.debugger.GetSelectedTarget();
   }
 
   SetSourceMapFromArguments(*arguments);

 
   SetSourceMapFromArguments(*arguments);
 
-  lldb::SBError status;
-  g_vsc.SetTarget(g_vsc.CreateTargetFromArguments(*arguments, status));
-  if (status.Fail()) {
-    response["success"] = llvm::json::Value(false);
-    EmplaceSafeString(response, "message", status.GetCString());
-    g_vsc.SendJSON(llvm::json::Value(std::move(response)));
-    return;
-  }
+  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 would implement the launch.
+    // Run any pre run LLDB commands the user specified in the launch.json
+
+    std::vector<llvm::StringRef> incompatible_fields;
+    for (llvm::StringRef arg : {"program", "cwd", "args", "env", "disableASLR", "disableSTDIO",
+                                "shellExpandArguments", "detachOnError", "runInTerminal", "targetTriple",
+                                "platformName"}) {
+      if (arguments->get(arg) != nullptr) {
+        incompatible_fields.push_back(arg);
+      }
+    }
 
-  // Instantiate a launch info instance for the target.
-  auto launch_info = g_vsc.target.GetLaunchInfo();
-
-  // Grab the current working directory if there is one and set it in the
-  // launch info.
-  const auto cwd = GetString(arguments, "cwd");
-  if (!cwd.empty())
-    launch_info.SetWorkingDirectory(cwd.data());
-
-  // Extract any extra arguments and append them to our program arguments for
-  // when we launch
-  auto args = GetStrings(arguments, "args");
-  if (!args.empty())
-    launch_info.SetArguments(MakeArgv(args).data(), true);
-
-  // Pass any environment variables along that the user specified.
-  auto envs = GetStrings(arguments, "env");
-  if (!envs.empty())
-    launch_info.SetEnvironmentEntries(MakeArgv(envs).data(), true);
-
-  auto flags = launch_info.GetLaunchFlags();
-
-  if (GetBoolean(arguments, "disableASLR", true))
-    flags |= lldb::eLaunchFlagDisableASLR;
-  if (GetBoolean(arguments, "disableSTDIO", false))
-    flags |= lldb::eLaunchFlagDisableSTDIO;
-  if (GetBoolean(arguments, "shellExpandArguments", false))
-    flags |= lldb::eLaunchFlagShellExpandArguments;
-  const bool detatchOnError = GetBoolean(arguments, "detachOnError", false);
-  launch_info.SetDetachOnError(detatchOnError);
-  launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug |
-                             lldb::eLaunchFlagStopAtEntry);
-
-  // Run any pre run LLDB commands the user specified in the launch.json
-  g_vsc.RunPreRunCommands();
-
-  if (GetBoolean(arguments, "runInTerminal", false)) {
-    if (llvm::Error err = request_runInTerminal(request))
-      error.SetErrorString(llvm::toString(std::move(err)).c_str());
-  } else if (launchCommands.empty()) {
-    // Disable async events so the launch will be successful when we return from
-    // the launch call and the launch will happen synchronously
-    g_vsc.debugger.SetAsync(false);
-    g_vsc.target.Launch(launch_info, error);
-    g_vsc.debugger.SetAsync(true);
-  } else {
+    if (!incompatible_fields.empty()) {
+      std::string str;
+      llvm::raw_string_ostream strm(str);
+      strm << "Incorrect debug configuration: " <<
+        "'launchCommands' is not compatible with ";
+      for (llvm::StringRef field : incompatible_fields) {
+        strm << " '" << field << "' ";
+      }
+      response["success"] = llvm::json::Value(false);
+      EmplaceSafeString(response, "message", strm.str());
+      g_vsc.SendJSON(llvm::json::Value(std::move(response)));
+      return;
+    }
+
+    g_vsc.RunPreRunCommands();
     g_vsc.RunLLDBCommands("Running launchCommands:", launchCommands);
-    // The custom commands might have created a new target so we should use the
-    // selected target after these commands are run.
-    g_vsc.target = g_vsc.debugger.GetSelectedTarget();
+    // The custom commands are expected to create a new target
+    if (g_vsc.debugger.GetNumTargets() > 0) {
+      g_vsc.SetTarget(g_vsc.debugger.GetSelectedTarget());
+    }
+    else {
+      error.SetErrorString("launchCommands failed to create target");
+    }
+  }
+  else {
+    // the normal logic that would implement the launch
+    lldb::SBError status;
+    g_vsc.SetTarget(g_vsc.CreateTargetFromArguments(*arguments, status));
+    if (status.Fail()) {
+      response["success"] = llvm::json::Value(false);
+      EmplaceSafeString(response, "message", status.GetCString());
+      g_vsc.SendJSON(llvm::json::Value(std::move(response)));
+      return;
+    }
+
+    // Instantiate a launch info instance for the target.
+    auto launch_info = g_vsc.target.GetLaunchInfo();
+
+    // Grab the current working directory if there is one and set it in the
+    // launch info.
+    const auto cwd = GetString(arguments, "cwd");
+    if (!cwd.empty())
+      launch_info.SetWorkingDirectory(cwd.data());
+
+    // Extract any extra arguments and append them to our program arguments for
+    // when we launch
+    auto args = GetStrings(arguments, "args");
+    if (!args.empty())
+      launch_info.SetArguments(MakeArgv(args).data(), true);
+
+    // Pass any environment variables along that the user specified.
+    auto envs = GetStrings(arguments, "env");
+    if (!envs.empty())
+      launch_info.SetEnvironmentEntries(MakeArgv(envs).data(), true);
+
+    auto flags = launch_info.GetLaunchFlags();
+
+    if (GetBoolean(arguments, "disableASLR", true))
+      flags |= lldb::eLaunchFlagDisableASLR;
+    if (GetBoolean(arguments, "disableSTDIO", false))
+      flags |= lldb::eLaunchFlagDisableSTDIO;
+    if (GetBoolean(arguments, "shellExpandArguments", false))
+      flags |= lldb::eLaunchFlagShellExpandArguments;
+    const bool detatchOnError = GetBoolean(arguments, "detachOnError", false);
+    launch_info.SetDetachOnError(detatchOnError);
+    launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug |
+                              lldb::eLaunchFlagStopAtEntry);
+
+    // Run any pre run LLDB commands the user specified in the launch.json
+    g_vsc.RunPreRunCommands();
+
+    if (GetBoolean(arguments, "runInTerminal", false)) {
+      if (llvm::Error err = request_runInTerminal(request))
+        error.SetErrorString(llvm::toString(std::move(err)).c_str());
+    } else {
+      // Disable async events so the launch will be successful when we return from
+      // the launch call and the launch will happen synchronously
+      g_vsc.debugger.SetAsync(false);
+      g_vsc.target.Launch(launch_info, error);
+      g_vsc.debugger.SetAsync(true);
+    }
   }
 
   if (error.Fail()) {

lldb/tools/lldb-vscode/package.json

               },
               "launchCommands": {
                 "type": "array",
-                "description": "Custom commands that are executed instead of launching a process. A target will be created with the launch arguments prior to executing these commands. The commands may optionally create a new target and must perform a launch. A valid process must exist after these commands complete or the \"launch\" will fail.",
+                "description": "Custom commands that are executed instead of launching a process. The commands must create a target and must perform a launch. A valid process must exist after these commands complete or the \"launch\" will fail. The following settings will be ignored if provided: \"program\", \"cwd\", \"args\", \"env\", \"disableASLR\", \"disableSTDIO\", \"shellExpandArguments\", \"detachOnError\", \"runInTerminal\", \"targetTriple\", \"platformName\"",
                 "default": []
               },
               "stopCommands": {

               },
               "attachCommands": {
                 "type": "array",
-                "description": "Custom commands that are executed instead of attaching to a process ID or to a process by name. These commands may optionally create a new target and must perform an attach. A valid process must exist after these commands complete or the \"attach\" will fail.",
+                "description": "Custom commands that are executed instead of attaching to a process ID or to a process by name. These commands must create a target and must perform an attach. A valid process must exist after these commands complete or the \"attach\" will fail. The following settings will be ignored if provided: \"program\", \"pid\", \"waitFor\", \"coreFile\", \"targetTriple\", \"platformName\"",
                 "default": []
               },
               "initCommands": {