Implemented initial DAP support #50454

rsubtil · 2021-07-14T15:03:07Z

(for GSoC DAP project, and godot-proposals#2494)

This PR implements support for the debug adapter protocol (DAP), implementing the most essential commands:

initialize/disconnect for protocol (de)initialization
launch/terminate for launching and terminating debug instances
pause/continue for pausing and continuing
next/stepIn for stepping over and in
setBreakpoints for setting breakpoints
stackTrace/scopes/variables for stacktrace information and variables
... (other small miscellaneous requests implemented, the complete list can be quickly checked at DebugAdapterParser.cpp:_bind_methods())

DebugAdapterProtocol and DebugAdapterParser work in tandem to implement the protocol backend: while Protocol handles communication, translating debugging info, notifying client and connects to various signals from the editor, Parser is focused with building the response packets for each request/event that is needed, and changing editor state in response to requests.

Other changes that were made:

Added new editor settings into a "Debug Adapter" category, similarly to how LSP is implemented. The default port for DAP is 6009.
Changed some communication messages between editor and project (core/debugger/remote_debugger.cpp):
- debug_enter now has an additional boolean field, indicating if the debugee has any stack frames to send. This allows to know immediately whether stackframes are coming or not, which is needed to distinguish between a breakpoint and a pause command.
- stack_frame_vars now reports the amount of variables it will be sending, allowing for the protocol to properly wait until all variables have been received.
Added a few signals on editor/debugger/script_editor_debugger.cpp to send information nedded for the protocol. These are prefixed with dap: as they're tailored for the protocol (except the new output signal)

I've done all testing under VSCode, and I'm able to already debug projects normally without ever touching the editor 🙂. Other from that, I've inspected the packets through Wireshark and manually check if they're correct according to the specification. However, this definitely needs more testing under other text editors/IDEs.

rsubtil · 2021-07-14T15:09:01Z

I also have the original unsquashed commit history here, if it's helpful for reviewing.

Faless

Looks really good! 👀 🥇
See the comments about containment and signals name, beside that, I'll do some testing once I manage to properly setup VS Code 😕 .

Faless · 2021-07-16T12:47:11Z

editor/debugger/script_editor_debugger.cpp

 		breaked = true;
 		can_debug = can_continue;
 		_update_buttons_state();
 		_set_reason_text(error, MESSAGE_ERROR);
 		emit_signal("breaked", true, can_continue);
-		DisplayServer::get_singleton()->window_move_to_foreground();
+		emit_signal("dap:breaked", error, has_stackdump);


I think that instead of adding dap: prefixed signals, we should just update the current ones, and add any extra needed one as non-prefixed.
The debuggers are exposed to Godot Editor plugins, so they'll benefit from this too.

Faless · 2021-07-16T12:50:27Z

editor/debugger/script_editor_debugger.cpp

 		breaked = true;
 		can_debug = can_continue;
 		_update_buttons_state();
 		_set_reason_text(error, MESSAGE_ERROR);
 		emit_signal("breaked", true, can_continue);
-		DisplayServer::get_singleton()->window_move_to_foreground();
+		emit_signal("dap:breaked", error, has_stackdump);
+		if (!DebugAdapterProtocol::get_singleton()->is_active()) {


For self-containment I would rather add this as a variable move_to_foreground that can be set via:

EditorDebuggerNode::get_singleton()->get_default_debugger()->set_move_to_foreground(false);

when the debugger is started by the DebugAdapterProtocol (and reset internally when the debugger disconnects).

Faless · 2021-07-16T12:57:40Z

editor/editor_node.cpp

@@ -2284,7 +2285,12 @@ void EditorNode::_run(bool p_current, const String &p_custom) {
 	editor_data.get_editor_breakpoints(&breakpoints);

 	args = ProjectSettings::get_singleton()->get("editor/run/main_run_args");
-	skip_breakpoints = EditorDebuggerNode::get_singleton()->is_skip_breakpoints();
+	DebugAdapterProtocol *dap = DebugAdapterProtocol::get_singleton();


Okay, it took me a bit to realize this, but as suggested for the move_to_foreground option, we can use the internal is_skip_breakpoints directly.
Instead of looking for the DAP current args, have the DebugAdapterProtocol call:

ScriptEditorDebugger *dbg = EditorDebuggerNode::get_singleton()->get_default_debugger(); if (arg["noDebug"] != dbg->is_skip_breakpoints()) { dbg->debug_skip_breakpoints(); }

Before emitting "play".

Fixed; also removed the current_args from DebugAdapterProtocol, as it's not needed anymore

Faless

See my comment about path checking.
This is excellent work 🥇 , and also tested a bit in VS Code.
I think is is good to merge as a first iteration 🟢

Faless · 2021-07-31T12:54:56Z

editor/debugger/debug_adapter/debug_adapter_parser.cpp

+
+Dictionary DebugAdapterParser::req_launch(const Dictionary &p_params) {
+	Dictionary args = p_params["arguments"];
+	if (args.has("project") && !is_valid_path(args["project"])) {


This caused me troubles due to symbolic link (I opened VSCode in the symlinked folder, while Godot used the real path).

I think this is an edge case that we might have to deal at some point, but it's not super important in common workflows. A better error, telling you the received (if any) and the expected path, would have saved me some debugging 😅 .

Just realized the error was being incorrectly sent, so VSCode didn't show the full message. I've fixed it now, and I've also updated that one to show both paths like you suggested.

Calinou · 2021-07-31T20:56:45Z

Since the default DAP port is 6009, isn't that going to be an issue with the editor remote debugger attempting ports 6007, then 6008, then 6009, and so on? The automatic remote debugger port assignment was implemented in #37067. It hasn't been forward-ported to master yet, but it's planned in the future.

rsubtil · 2021-07-31T21:27:50Z

@Calinou yup, that's a problem. I can change it to something lower like 6006 then.

But LSP uses 6008, so that's an issue too 😕

Faless · 2021-07-31T21:29:31Z

Since the default DAP port is 6009, isn't that going to be an issue with the editor remote debugger attempting ports 6007, then 6008, then 6009, and so on?

It's the same as with the LSP (port 6008). It will fail 2 times (instead of 1 like it does now).

Btw, weren't they both supposed to be behind a flag? Seems like they are auto-started on editor start, which sounds like a security issue.

Calinou · 2021-07-31T23:02:17Z

Btw, weren't they both supposed to be behind a flag? Seems like they are auto-started on editor start, which sounds like a security issue.

If the servers only listen on 127.0.0.1 rather than 0.0.0.0, it's probably fine security-wise. The editor's remote debugger only listens on 127.0.0.1.

Faless · 2021-07-31T23:08:15Z

If the servers only listen on 127.0.0.1 rather than 0.0.0.0, it's probably fine security-wise. The editor's remote debugger only listens on 127.0.0.1.

Right 👍 , we should probably make that an option, and use 127.0.0.1 by default (like we do for the web server).
EDIT: Oh, the LSP already listen to "127.0.0.1" only, I missed that.

I can change it to something lower like 6006 then.

Yeah, I guess that's the best choice. We can change the LSP default port in another PR.

Implemented "output" event Refactored "seq" field generation Prevent debugging when editor and client are in different projects Removed unneeded references to peer on the parser Refactored way to detect project path Implemented "setBreakpoints" request Fix double events when terminating from client Refactored "stopped" event Implemented "stopped" with breakpoint event Implemented "stackTrace", "scopes" and "variables" request Report incoming number of stack dump variables Implemented proper reporting of scopes and variables from stack frames Prevent editor from grabbing focus when a DAP session is active Implemented "next" and "stepIn" requests Implemented "Source" checksum computing Switched expected errors from macros to silent guards Refactored message_id Respect client settings regarding lines/columns behavior Refactored nested DAP fields Implement reporting of "Members" and "Globals" scopes as well Fix error messages not being shown, and improved wrong path message

rsubtil · 2021-08-02T10:10:50Z

Changed the port to 6006 as default.

@shackra since Nathan mentioned they've "sponsored someone to fix these issues", I don't want to potentially step into someone's current work. However, if there's indeed no active development on that, I don't mind having a look and see where I can help, in a few weeks.

akien-mga · 2021-08-02T11:07:54Z

@shackra since Nathan mentioned they've "sponsored someone to fix these issues", I don't want to potentially step into someone's current work. However, if there's indeed no active development on that, I don't mind having a look and see where I can help, in a few weeks.

@Razoric480 has been doing a number of fixes for the LSP, at least in part on behalf of GDQuest. So the issues referenced in this older issue might be solved. If not, they would be best described in dedicated bug reports so that @Razoric480 (and possibly @ev1lbl0w which his DAP expertise) could have a look at them. We're getting close to having a "External editors" team :)

Razoric480 · 2021-08-02T13:20:57Z

@ev1lbl0w @akien-mga I am not working on any specific LSP issue at the moment. Occasionally I see something wrong or that I want to improve and so work on it. That said, I don't recognize any of those issues as linked by @shackra and my testbed is in VSCode. I am not an emac user (and in fact it frightens me) so I'm not sure what could be wrong.

By the way, nice work on the DAP. My initial implementation of the debugger extension in vscode was based off the TCP/IP protocol godot uses, so its quality, while it worked, felt a bit cobbled together in places.

Faless · 2021-08-03T15:13:10Z

Really nice work! Thanks! 🎊 🏆

rsubtil requested review from a team as code owners July 14, 2021 15:03

Calinou added enhancement topic:editor topic:gdscript labels Jul 14, 2021

Calinou added this to the 4.0 milestone Jul 14, 2021

Faless requested changes Jul 16, 2021

View reviewed changes

rsubtil force-pushed the gsoc21-dap branch 2 times, most recently from 94d5c9c to cb9e3fb Compare July 19, 2021 14:03

Faless approved these changes Jul 31, 2021

View reviewed changes

rsubtil force-pushed the gsoc21-dap branch from cb9e3fb to 6a77d2b Compare July 31, 2021 21:09

This comment has been minimized.

Sign in to view

rsubtil force-pushed the gsoc21-dap branch from 6a77d2b to 7bccd54 Compare August 2, 2021 09:45

shackra mentioned this pull request Aug 3, 2021

LSP bugs experienced with lsp-mode in Emacs #51202

Open

Faless merged commit 80fc90e into godotengine:master Aug 3, 2021

Faless mentioned this pull request Feb 17, 2022

Implement DAP-compliant middleware to interact with Editor Debugger godotengine/godot-proposals#2494

Closed

Faless mentioned this pull request Jul 25, 2022

Implement a threaded script debugger godotengine/godot-proposals#4952

Closed

rsubtil mentioned this pull request Sep 18, 2023

Add LSP/DAP configuration section on external editors godotengine/godot-docs#7948

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented initial DAP support #50454

Implemented initial DAP support #50454

rsubtil commented Jul 14, 2021

rsubtil commented Jul 14, 2021

Faless left a comment

Faless Jul 16, 2021

rsubtil Jul 19, 2021

Faless Jul 16, 2021

rsubtil Jul 19, 2021

Faless Jul 16, 2021

rsubtil Jul 19, 2021

Faless left a comment

Faless Jul 31, 2021

rsubtil Jul 31, 2021

Calinou commented Jul 31, 2021 •

edited

Loading

rsubtil commented Jul 31, 2021

Faless commented Jul 31, 2021 •

edited

Loading

Calinou commented Jul 31, 2021 •

edited

Loading

Faless commented Jul 31, 2021 •

edited

Loading

This comment has been minimized.

rsubtil commented Aug 2, 2021

akien-mga commented Aug 2, 2021

Razoric480 commented Aug 2, 2021

Faless commented Aug 3, 2021

Implemented initial DAP support #50454

Implemented initial DAP support #50454

Conversation

rsubtil commented Jul 14, 2021

rsubtil commented Jul 14, 2021

Faless left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Faless left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Calinou commented Jul 31, 2021 • edited Loading

rsubtil commented Jul 31, 2021

Faless commented Jul 31, 2021 • edited Loading

Calinou commented Jul 31, 2021 • edited Loading

Faless commented Jul 31, 2021 • edited Loading

This comment has been minimized.

rsubtil commented Aug 2, 2021

akien-mga commented Aug 2, 2021

Razoric480 commented Aug 2, 2021

Faless commented Aug 3, 2021

Calinou commented Jul 31, 2021 •

edited

Loading

Faless commented Jul 31, 2021 •

edited

Loading

Calinou commented Jul 31, 2021 •

edited

Loading

Faless commented Jul 31, 2021 •

edited

Loading