[Lldb-commits] [lldb] [llvm] [lldb][lldb-dap] Implement jump to cursor (PR #130503)

[Adrian Vogelsgesang via lldb-commits]

================
@@ -0,0 +1,118 @@
+//===-- GoToRequestHandler.cpp --------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "DAP.h"
+#include "EventHelper.h"
+#include "JSONUtils.h"
+
+namespace lldb_dap {
+
+/// Creates an \p StoppedEvent with the reason \a goto
+static void SendThreadGotoEvent(DAP &dap, lldb::tid_t thread_id) {
+  llvm::json::Object event(CreateEventObject("stopped"));
+  llvm::json::Object body;
+  body.try_emplace("reason", "goto");
+  body.try_emplace("description", "Paused on Jump To Cursor");
+  body.try_emplace("threadId", thread_id);
+  body.try_emplace("preserveFocusHint", false);
+  body.try_emplace("allThreadsStopped", true);
+
+  event.try_emplace("body", std::move(body));
+  dap.SendJSON(llvm::json::Value(std::move(event)));
+}
+
+// "GotoRequest": {
+//   "allOf": [ { "$ref": "#/definitions/Request" }, {
+//     "type": "object",
+//     "description": "The request sets the location where the debuggee will
+//     continue to run.\nThis makes it possible to skip the execution of code or
+//     to execute code again.\nThe code between the current location and the
+//     goto target is not executed but skipped.\nThe debug adapter first sends
+//     the response and then a `stopped` event with reason `goto`.\nClients
+//     should only call this request if the corresponding capability
+//     `supportsGotoTargetsRequest` is true (because only then goto targets
+//     exist that can be passed as arguments).",
+//.    "properties": {
+//       "command": {
+//         "type": "string",
+//         "enum": [ "goto" ]
+//       },
+//       "arguments": {
+//         "$ref": "#/definitions/GotoArguments"
+//       }
+//     },
+//     "required": [ "command", "arguments"  ]
+//   }]
+// }
+// "GotoArguments": {
+//   "type": "object",
+//   "description": "Arguments for `goto` request.",
+//   "properties": {
+//     "threadId": {
+//       "type": "integer",
+//       "description": "Set the goto target for this thread."
+//     },
+//     "targetId": {
+//       "type": "integer",
+//       "description": "The location where the debuggee will continue to run."
+//     }
+//   },
+//   "required": [ "threadId", "targetId" ]
+// }
+// "GotoResponse": {
+//   "allOf": [ { "$ref": "#/definitions/Response" }, {
+//     "type": "object",
+//     "description": "Response to `goto` request. This is just an
+//     acknowledgement, so no body field is required."
+//   }]
+// }
+void GoToRequestHandler::operator()(const llvm::json::Object &request) const {
+  llvm::json::Object response;
+  FillResponse(request, response);
+
+  auto SendError = [&](auto &&message) {
+    response["success"] = false;
+    response["message"] = message;
+    dap.SendJSON(llvm::json::Value(std::move(response)));
+  };
----------------
vogelsgesang wrote:

IMO, we should not factor out this helper, because:

1. We already have a better abstraction.
2. This helper is encouraging to violate the debug adapter protocol.

**Better abstraction**: In #129155, @ashgti already introduced the `protocol::Response` class, which also provides the `message` header. In #130090, those messages got integrated with our request handlers. IMO, we should change this PR to use the new mechanism introduced in #130090, instead of using the legacy mechanism.

**Violation of the debug adapter protocol specification**: This helper sets the message to an arbitrary string, such as `"Arguments is empty"`. However, this is not the correct usage of DAP.

The `message` field of the [Response](https://microsoft.github.io/debug-adapter-protocol/specification#Base_Protocol_Response) should be a machine-readable enum (`cancelled`, `notStopped`, ...). It should not contain the human-readable the error message and will not be displayed to users.

Instead, the user-readable message should use the [ErrorResponse](https://microsoft.github.io/debug-adapter-protocol/specification#Base_Protocol_ErrorResponse), i.e. the user-readable message should be in `body. format`, and `body.showUser` should probably be set to true.

This is currently done incorrectly by lldb-dap everywhere. This is also, why users currently get no feedback if, e.g., setting a variable failed. The new mechanism introduced in #130090 still does not handle this correctly. But if we consistently adopt the new mechanism, we at least only need to fix a single place to become standard-compliant

(That being said, I would not block this PR on using `RequestHandler` instead of `LegacyRequestHandler,` in case @da-viper does not want to invest the time to update his code to the newer `RequestHandler` mechanism)

https://github.com/llvm/llvm-project/pull/130503