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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

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



================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:627-628
+               "incompatible with 'attachCommands'.\n", arg.str().c_str());
+      }
+    }
+    // Run any pre run LLDB commands the user specified in the launch.json
----------------
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());
```

 


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1696-1697
+               "incompatible with 'launchCommands'.\n", arg.str().c_str());
+      }
+    }
+    g_vsc.RunPreRunCommands();
----------------
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.



================
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
----------------
aadsm wrote:
> clayborg wrote:
> > 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.
> 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.
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?


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

https://reviews.llvm.org/D94997