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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
