[Lldb-commits] [PATCH] D84974: [WIP] Enable Launching the Debugee in VSCode Terminal

[Greg Clayton via Phabricator via lldb-commits]

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

So there is a lot of noise in this patch that is just reformatting on code that hasn't changed. It would be nice to get rid of any changes that are whitespace/indentation/formatting only. Also see inlined comments for needed changes.



================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:410
+
+VSCode::PacketStatus VSCode::SendReverseRequest(llvm::json::Object &request,
+                                                llvm::json::Object &response) {
----------------
add "const" to before "llvm::json::Object &request"


================
Comment at: lldb/tools/lldb-vscode/VSCode.h:166-173
+  const std::map<std::string, RequestCallback> &GetRequestHandlers();
+
+  PacketStatus GetObject(llvm::json::Object &object);
+  bool HandleObject(const llvm::json::Object &object);
+  PacketStatus SendReverseRequest(llvm::json::Object &request,
+                                  llvm::json::Object &response);
+
----------------
Revert all changes to this function. Seems like there are some forward declarations to functions in here for some reason that aren't needed.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1441
+                           llvm::json::Object &launch_response) {
+  // Fill out attach info
+  g_vsc.is_attach = true;
----------------
Add more comment here to explain how we are attaching. Maybe something like:
```
// We have already created a target that has a valid "program" path to the executable.
// We will attach to the next process whose basename matches that of the target's 
// executable by waiting for the next process to launch that matches. This help LLDB
// ignore any existing processes that are already running that match this executable
// name and wait for the next one to launch.
```


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1448-1449
+  g_vsc.stop_at_entry = true;
+  auto attach_func = [&]() { g_vsc.target.Attach(attach_info, error); };
+  std::thread attacher(attach_func);
+
----------------
Inline the lambda, add a comment, and enable async mode to ensure we don't have a race condition:

```
// Here we launch a thread to do the attach. We disable async events so
// that when we call g_vsc.target.Attach(...) it will not return unless the attach
// succeeds. In the event the attach never happens, the user will be able to hit
// the square stop button in the debug session and it will kill lldb-vscode. If
// this ever changes in the IDE we will need to do more work to ensure we can
// nicely timeout from the attach after a set period of time.
std::thread attacher([&]() { 
   g_vsc.debugger.SetAsync(false);
   g_vsc.target.Attach(attach_info, error); 
   g_vsc.debugger.SetAsync(true);
});
```


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1486-1506
+  if (error.Success()) {
+    // IDE doesn't respond back the pid of the target, so lldb can only attach
+    // by process name, which is already set up in request_launch().
+    auto attached_pid = g_vsc.target.GetProcess().GetProcessID();
+    if (attached_pid == LLDB_INVALID_PROCESS_ID) {
+      error.SetErrorString("Failed to attach to a process");
+    }
----------------
Need to clean up this logic a bit. We have two "if (error.Success())" statements and the flow of the code is hard to follow.

```
if (error.Success()) {
  // IDE doesn't respond back the pid of the target from the runInTerminal 
  // response, so we have lldb attach by name and wait, so grab the process
  // ID from the target.
  auto attached_pid = g_vsc.target.GetProcess().GetProcessID();
  if (attached_pid == LLDB_INVALID_PROCESS_ID)
    error.SetErrorString("Failed to attach to a process");
  else
    SendProcessEvent(Attach);
}

// Check error again as it might have been modified with a new error above.
if (error.Fail()) {
  launch_response["success"] = llvm::json::Value(false);
  EmplaceSafeString(launch_response, "message", std::string(error.GetCString()));
} else {
  launch_response["success"] = llvm::json::Value(true);
}

```


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1582-1585
+  if (GetBoolean(arguments, "launchInTerminal", false)) {
+    request_runInTerminal(request, response);
+    g_vsc.SendJSON(llvm::json::Value(std::move(response)));
   } else {
----------------
Remove all new indentation that was added for the else scope and use early return:

```
if (GetBoolean(arguments, "launchInTerminal", false)) {
  request_runInTerminal(request, response);
  g_vsc.SendJSON(llvm::json::Value(std::move(response)));
  return;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84974