Fix DAP bugs: crash when removing breakpoints, duplicated breakpoint data, source checksums not updating

Author: rsubtil

editor/debugger/debug_adapter/debug_adapter_parser.cpp

@@ -322,8 +322,7 @@ Dictionary DebugAdapterParser::req_stackTrace(const Dictionary &p_params) const
 
 	Array arr;
 	DebugAdapterProtocol *dap = DebugAdapterProtocol::get_singleton();
-	for (const KeyValue<DAP::StackFrame, List<int>> &E : dap->stackframe_list) {
-		DAP::StackFrame sf = E.key;
+	for (DAP::StackFrame sf : dap->stackframe_list) {
 		if (!lines_at_one) {
 			sf.line--;
 		}
@@ -369,6 +368,8 @@ Dictionary DebugAdapterParser::req_setBreakpoints(const Dictionary &p_params) co
 		lines.push_back(breakpoint.line + !lines_at_one);
 	}
 
+	// Always update the source checksum for the requested path, as it might have been modified externally.
+	DebugAdapterProtocol::get_singleton()->update_source(source.path);
 	Array updated_breakpoints = DebugAdapterProtocol::get_singleton()->update_breakpoints(source.path, lines);
 	body["breakpoints"] = updated_breakpoints;
 
@@ -399,15 +400,13 @@ Dictionary DebugAdapterParser::req_scopes(const Dictionary &p_params) const {
 	int frame_id = args["frameId"];
 	Array scope_list;
 
-	DAP::StackFrame frame;
-	frame.id = frame_id;
-	HashMap<DAP::StackFrame, List<int>, DAP::StackFrame>::Iterator E = DebugAdapterProtocol::get_singleton()->stackframe_list.find(frame);
+	HashMap<DebugAdapterProtocol::DAPStackFrameID, Vector<int>>::Iterator E = DebugAdapterProtocol::get_singleton()->scope_list.find(frame_id);
 	if (E) {
-		ERR_FAIL_COND_V(E->value.size() != 3, prepare_error_response(p_params, DAP::ErrorType::UNKNOWN));
-		List<int>::ConstIterator itr = E->value.begin();
-		for (int i = 0; i < 3; ++itr, ++i) {
+		const Vector<int> &scope_ids = E->value;
+		ERR_FAIL_COND_V(scope_ids.size() != 3, prepare_error_response(p_params, DAP::ErrorType::UNKNOWN));
+		for (int i = 0; i < 3; ++i) {
 			DAP::Scope scope;
-			scope.variablesReference = *itr;
+			scope.variablesReference = scope_ids[i];
 			switch (i) {
 				case 0:
 					scope.name = "Locals";

editor/debugger/debug_adapter/debug_adapter_protocol.cpp

@@ -176,6 +176,7 @@ void DebugAdapterProtocol::reset_current_info() {
 void DebugAdapterProtocol::reset_ids() {
 	breakpoint_id = 0;
 	breakpoint_list.clear();
+	breakpoint_source_list.clear();
 
 	reset_stack_info();
 }
@@ -185,6 +186,7 @@ void DebugAdapterProtocol::reset_stack_info() {
 	variable_id = 1;
 
 	stackframe_list.clear();
+	scope_list.clear();
 	variable_list.clear();
 	object_list.clear();
 	object_pending_set.clear();
@@ -824,6 +826,30 @@ bool DebugAdapterProtocol::request_remote_evaluate(const String &p_eval, int p_s
 	return true;
 }
 
+const DAP::Source &DebugAdapterProtocol::fetch_source(const String &p_path) {
+	const String &global_path = ProjectSettings::get_singleton()->globalize_path(p_path);
+
+	HashMap<String, DAP::Source>::Iterator E = breakpoint_source_list.find(global_path);
+	if (E != breakpoint_source_list.end()) {
+		return E->value;
+	}
+	DAP::Source &added_source = breakpoint_source_list.insert(global_path, DAP::Source())->value;
+	added_source.name = global_path.get_file();
+	added_source.path = global_path;
+	added_source.compute_checksums();
+
+	return added_source;
+}
+
+void DebugAdapterProtocol::update_source(const String &p_path) {
+	const String &global_path = ProjectSettings::get_singleton()->globalize_path(p_path);
+
+	HashMap<String, DAP::Source>::Iterator E = breakpoint_source_list.find(global_path);
+	if (E != breakpoint_source_list.end()) {
+		E->value.compute_checksums();
+	}
+}
+
 bool DebugAdapterProtocol::process_message(const String &p_text) {
 	JSON json;
 	ERR_FAIL_COND_V_MSG(json.parse(p_text) != OK, true, "Malformed message!");
@@ -962,22 +988,40 @@ Array DebugAdapterProtocol::update_breakpoints(const String &p_path, const Array
 
 	// Add breakpoints
 	for (int i = 0; i < p_lines.size(); i++) {
-		EditorDebuggerNode::get_singleton()->get_default_debugger()->_set_breakpoint(p_path, p_lines[i], true);
-		DAP::Breakpoint breakpoint;
+		DAP::Breakpoint breakpoint(fetch_source(p_path));
 		breakpoint.line = p_lines[i];
-		breakpoint.source.path = p_path;
 
-		ERR_FAIL_COND_V(!breakpoint_list.find(breakpoint), Array());
-		updated_breakpoints.push_back(breakpoint_list.find(breakpoint)->get().to_json());
+		// Avoid duplicated entries.
+		List<DAP::Breakpoint>::Element *E = breakpoint_list.find(breakpoint);
+		if (E) {
+			updated_breakpoints.push_back(E->get().to_json());
+			continue;
+		}
+
+		EditorDebuggerNode::get_singleton()->get_default_debugger()->_set_breakpoint(p_path, p_lines[i], true);
+
+		// Breakpoints are inserted at the end of the breakpoint list.
+		List<DAP::Breakpoint>::Element *added_breakpoint = breakpoint_list.back();
+		ERR_FAIL_NULL_V(added_breakpoint, Array());
+		ERR_FAIL_COND_V(!(added_breakpoint->get() == breakpoint), Array());
+		updated_breakpoints.push_back(added_breakpoint->get().to_json());
 	}
 
 	// Remove breakpoints
+	// Must be deferred because we are iterating the breakpoint list.
+	Vector<int> to_remove;
+
 	for (const DAP::Breakpoint &b : breakpoint_list) {
-		if (b.source.path == p_path && !p_lines.has(b.line)) {
-			EditorDebuggerNode::get_singleton()->get_default_debugger()->_set_breakpoint(p_path, b.line, false);
+		if (b.source->path == p_path && !p_lines.has(b.line)) {
+			to_remove.push_back(b.line);
 		}
 	}
 
+	// Safe to remove queued data now.
+	for (const int &line : to_remove) {
+		EditorDebuggerNode::get_singleton()->get_default_debugger()->_set_breakpoint(p_path, line, false);
+	}
+
 	return updated_breakpoints;
 }
 
@@ -1020,10 +1064,8 @@ void DebugAdapterProtocol::on_debug_breaked(const bool &p_reallydid, const bool
 }
 
 void DebugAdapterProtocol::on_debug_breakpoint_toggled(const String &p_path, const int &p_line, const bool &p_enabled) {
-	DAP::Breakpoint breakpoint;
+	DAP::Breakpoint breakpoint(fetch_source(p_path));
 	breakpoint.verified = true;
-	breakpoint.source.path = ProjectSettings::get_singleton()->globalize_path(p_path);
-	breakpoint.source.compute_checksums();
 	breakpoint.line = p_line;
 
 	if (p_enabled) {
@@ -1046,8 +1088,7 @@ void DebugAdapterProtocol::on_debug_stack_dump(const Array &p_stack_dump) {
 	if (_processing_breakpoint && !p_stack_dump.is_empty()) {
 		// Find existing breakpoint
 		Dictionary d = p_stack_dump[0];
-		DAP::Breakpoint breakpoint;
-		breakpoint.source.path = ProjectSettings::get_singleton()->globalize_path(d["file"]);
+		DAP::Breakpoint breakpoint(fetch_source(d["file"]));
 		breakpoint.line = d["line"];
 
 		List<DAP::Breakpoint>::Element *E = breakpoint_list.find(breakpoint);
@@ -1060,25 +1101,26 @@ void DebugAdapterProtocol::on_debug_stack_dump(const Array &p_stack_dump) {
 
 	stackframe_id = 0;
 	stackframe_list.clear();
+	scope_list.clear();
 
 	// Fill in stacktrace information
 	for (int i = 0; i < p_stack_dump.size(); i++) {
 		Dictionary stack_info = p_stack_dump[i];
-		DAP::StackFrame stackframe;
+
+		DAP::StackFrame stackframe(fetch_source(stack_info["file"]));
 		stackframe.id = stackframe_id++;
 		stackframe.name = stack_info["function"];
 		stackframe.line = stack_info["line"];
 		stackframe.column = 0;
-		stackframe.source.path = ProjectSettings::get_singleton()->globalize_path(stack_info["file"]);
-		stackframe.source.compute_checksums();
 
 		// Information for "Locals", "Members" and "Globals" variables respectively
-		List<int> scope_ids;
+		Vector<int> scope_ids;
 		for (int j = 0; j < 3; j++) {
 			scope_ids.push_back(variable_id++);
 		}
 
-		stackframe_list.insert(stackframe, scope_ids);
+		stackframe_list.push_back(stackframe);
+		scope_list.insert(stackframe.id, scope_ids);
 	}
 
 	_current_frame = 0;
@@ -1087,11 +1129,9 @@ void DebugAdapterProtocol::on_debug_stack_dump(const Array &p_stack_dump) {
 
 void DebugAdapterProtocol::on_debug_stack_frame_vars(const int &p_size) {
 	_remaining_vars = p_size;
-	DAP::StackFrame frame;
-	frame.id = _current_frame;
-	ERR_FAIL_COND(!stackframe_list.has(frame));
-	List<int> scope_ids = stackframe_list.find(frame)->value;
-	for (const int var_id : scope_ids) {
+	ERR_FAIL_COND(!scope_list.has(_current_frame));
+	Vector<int> scope_ids = scope_list.find(_current_frame)->value;
+	for (const int &var_id : scope_ids) {
 		if (variable_list.has(var_id)) {
 			variable_list.find(var_id)->value.clear();
 		} else {
@@ -1104,11 +1144,9 @@ void DebugAdapterProtocol::on_debug_stack_frame_var(const Array &p_data) {
 	DebuggerMarshalls::ScriptStackVariable stack_var;
 	stack_var.deserialize(p_data);
 
-	ERR_FAIL_COND(stackframe_list.is_empty());
-	DAP::StackFrame frame;
-	frame.id = _current_frame;
+	ERR_FAIL_COND(!scope_list.has(_current_frame));
+	Vector<int> scope_ids = scope_list.find(_current_frame)->value;
 
-	List<int> scope_ids = stackframe_list.find(frame)->value;
 	ERR_FAIL_COND(scope_ids.size() != 3);
 	ERR_FAIL_INDEX(stack_var.type, 4);
 	int var_id = scope_ids.get(stack_var.type);

editor/debugger/debug_adapter/debug_adapter_protocol.h

@@ -76,6 +76,7 @@ class DebugAdapterProtocol : public Object {
 	friend class DebugAdapterParser;
 
 	using DAPVarID = int;
+	using DAPStackFrameID = int;
 
 private:
 	static DebugAdapterProtocol *singleton;
@@ -109,6 +110,9 @@ class DebugAdapterProtocol : public Object {
 	bool request_remote_object(const ObjectID &p_object_id);
 	bool request_remote_evaluate(const String &p_eval, int p_stack_frame);
 
+	const DAP::Source &fetch_source(const String &p_path);
+	void update_source(const String &p_path);
+
 	bool _initialized = false;
 	bool _processing_breakpoint = false;
 	bool _stepping = false;
@@ -125,7 +129,9 @@ class DebugAdapterProtocol : public Object {
 	int stackframe_id = 0;
 	DAPVarID variable_id = 0;
 	List<DAP::Breakpoint> breakpoint_list;
-	HashMap<DAP::StackFrame, List<int>, DAP::StackFrame> stackframe_list;
+	HashMap<String, DAP::Source> breakpoint_source_list;
+	List<DAP::StackFrame> stackframe_list;
+	HashMap<DAPStackFrameID, Vector<int>> scope_list;
 	HashMap<DAPVarID, Array> variable_list;
 
 	HashMap<ObjectID, DAPVarID> object_list;

editor/debugger/debug_adapter/debug_adapter_types.h

@@ -30,8 +30,7 @@
 
 #pragma once
 
-#include "core/io/json.h"
-#include "core/variant/dictionary.h"
+#include "core/io/file_access.h"
 
 namespace DAP {
 
@@ -68,6 +67,8 @@ struct Source {
 	void compute_checksums() {
 		ERR_FAIL_COND(path.is_empty());
 
+		_checksums.clear();
+
 		// MD5
 		Checksum md5;
 		md5.algorithm = "MD5";
@@ -101,18 +102,24 @@ struct Source {
 struct Breakpoint {
 	int id = 0;
 	bool verified = false;
-	Source source;
+	const Source *source = nullptr;
 	int line = 0;
 
+	Breakpoint() = default; // Empty constructor is invalid, but is necessary because Godot's collections don't support rvalues.
+	Breakpoint(const Source &p_source) :
+			source(&p_source) {}
+
 	bool operator==(const Breakpoint &p_other) const {
-		return source.path == p_other.source.path && line == p_other.line;
+		return source == p_other.source && line == p_other.line;
 	}
 
 	_FORCE_INLINE_ Dictionary to_json() const {
 		Dictionary dict;
 		dict["id"] = id;
 		dict["verified"] = verified;
-		dict["source"] = source.to_json();
+		if (source) {
+			dict["source"] = source->to_json();
+		}
 		dict["line"] = line;
 
 		return dict;
@@ -212,30 +219,25 @@ struct SourceBreakpoint {
 struct StackFrame {
 	int id = 0;
 	String name;
-	Source source;
+	const Source *source = nullptr;
 	int line = 0;
 	int column = 0;
 
+	StackFrame() = default; // Empty constructor is invalid, but is necessary because Godot's collections don't support rvalues.
+	StackFrame(const Source &p_source) :
+			source(&p_source) {}
+
 	static uint32_t hash(const StackFrame &p_frame) {
 		return hash_murmur3_one_32(p_frame.id);
 	}
-	bool operator==(const StackFrame &p_other) const {
-		return id == p_other.id;
-	}
-
-	_FORCE_INLINE_ void from_json(const Dictionary &p_params) {
-		id = p_params["id"];
-		name = p_params["name"];
-		source.from_json(p_params["source"]);
-		line = p_params["line"];
-		column = p_params["column"];
-	}
 
 	_FORCE_INLINE_ Dictionary to_json() const {
 		Dictionary dict;
 		dict["id"] = id;
 		dict["name"] = name;
-		dict["source"] = source.to_json();
+		if (source) {
+			dict["source"] = source->to_json();
+		}
 		dict["line"] = line;
 		dict["column"] = column;