Merge pull request #76582 from reduz/threaded-debugger

Support threads in the script debugger

core/debugger/debugger_marshalls.cpp

@@ -67,14 +67,15 @@ Array DebuggerMarshalls::ScriptStackVariable::serialize(int max_size) {
 	Array arr;
 	arr.push_back(name);
 	arr.push_back(type);
+	arr.push_back(value.get_type());
 
 	Variant var = value;
 	if (value.get_type() == Variant::OBJECT && value.get_validated_object() == nullptr) {
 		var = Variant();
 	}
 
 	int len = 0;
-	Error err = encode_variant(var, nullptr, len, true);
+	Error err = encode_variant(var, nullptr, len, false);
 	if (err != OK) {
 		ERR_PRINT("Failed to encode variant.");
 	}
@@ -88,11 +89,12 @@ Array DebuggerMarshalls::ScriptStackVariable::serialize(int max_size) {
 }
 
 bool DebuggerMarshalls::ScriptStackVariable::deserialize(const Array &p_arr) {
-	CHECK_SIZE(p_arr, 3, "ScriptStackVariable");
+	CHECK_SIZE(p_arr, 4, "ScriptStackVariable");
 	name = p_arr[0];
 	type = p_arr[1];
-	value = p_arr[2];
-	CHECK_END(p_arr, 3, "ScriptStackVariable");
+	var_type = p_arr[2];
+	value = p_arr[3];
+	CHECK_END(p_arr, 4, "ScriptStackVariable");
 	return true;
 }
 

core/debugger/debugger_marshalls.h

@@ -38,6 +38,7 @@ struct DebuggerMarshalls {
 		String name;
 		Variant value;
 		int type = -1;
+		int var_type = -1;
 
 		Array serialize(int max_size = 1 << 20); // 1 MiB default.
 		bool deserialize(const Array &p_arr);

core/debugger/engine_debugger.cpp

@@ -111,14 +111,6 @@ Error EngineDebugger::capture_parse(const StringName &p_name, const String &p_ms
 	return cap.capture(cap.data, p_msg, p_args, r_captured);
 }
 
-void EngineDebugger::line_poll() {
-	// The purpose of this is just processing events every now and then when the script might get too busy otherwise bugs like infinite loops can't be caught
-	if (poll_every % 2048 == 0) {
-		poll_events(false);
-	}
-	poll_every++;
-}
-
 void EngineDebugger::iteration(uint64_t p_frame_ticks, uint64_t p_process_ticks, uint64_t p_physics_ticks, double p_physics_frame_time) {
 	frame_time = USEC_TO_SEC(p_frame_ticks);
 	process_time = USEC_TO_SEC(p_process_ticks);

core/debugger/engine_debugger.h

@@ -126,7 +126,13 @@ class EngineDebugger {
 	void profiler_enable(const StringName &p_name, bool p_enabled, const Array &p_opts = Array());
 	Error capture_parse(const StringName &p_name, const String &p_msg, const Array &p_args, bool &r_captured);
 
-	void line_poll();
+	void line_poll() {
+		// The purpose of this is just processing events every now and then when the script might get too busy otherwise bugs like infinite loops can't be caught.
+		if (unlikely(poll_every % 2048) == 0) {
+			poll_events(false);
+		}
+		poll_every++;
+	}
 
 	virtual void poll_events(bool p_is_idle) {}
 	virtual void send_message(const String &p_msg, const Array &p_data) = 0;

core/debugger/remote_debugger.cpp

@@ -94,6 +94,7 @@ class RemoteDebugger::PerformanceProfiler : public EngineProfiler {
 Error RemoteDebugger::_put_msg(String p_message, Array p_data) {
 	Array msg;
 	msg.push_back(p_message);
+	msg.push_back(Thread::get_caller_id());
 	msg.push_back(p_data);
 	Error err = peer->put_message(msg);
 	if (err != OK) {
@@ -185,9 +186,9 @@ RemoteDebugger::ErrorMessage RemoteDebugger::_create_overflow_error(const String
 }
 
 void RemoteDebugger::flush_output() {
+	MutexLock lock(mutex);
 	flush_thread = Thread::get_caller_id();
 	flushing = true;
-	MutexLock lock(mutex);
 	if (!is_peer_connected()) {
 		return;
 	}
@@ -348,18 +349,65 @@ Error RemoteDebugger::_try_capture(const String &p_msg, const Array &p_data, boo
 	return capture_parse(cap, msg, p_data, r_captured);
 }
 
+void RemoteDebugger::_poll_messages() {
+	MutexLock mutex_lock(mutex);
+
+	peer->poll();
+	while (peer->has_message()) {
+		Array cmd = peer->get_message();
+		ERR_CONTINUE(cmd.size() != 3);
+		ERR_CONTINUE(cmd[0].get_type() != Variant::STRING);
+		ERR_CONTINUE(cmd[1].get_type() != Variant::INT);
+		ERR_CONTINUE(cmd[2].get_type() != Variant::ARRAY);
+
+		Thread::ID thread = cmd[1];
+
+		if (!messages.has(thread)) {
+			continue; // This thread is not around to receive the messages
+		}
+
+		Message msg;
+		msg.message = cmd[0];
+		msg.data = cmd[2];
+		messages[thread].push_back(msg);
+	}
+}
+
+bool RemoteDebugger::_has_messages() {
+	MutexLock mutex_lock(mutex);
+	return messages.has(Thread::get_caller_id()) && !messages[Thread::get_caller_id()].is_empty();
+}
+
+Array RemoteDebugger::_get_message() {
+	MutexLock mutex_lock(mutex);
+	ERR_FAIL_COND_V(!messages.has(Thread::get_caller_id()), Array());
+	List<Message> &message_list = messages[Thread::get_caller_id()];
+	ERR_FAIL_COND_V(message_list.is_empty(), Array());
+
+	Array msg;
+	msg.resize(2);
+	msg[0] = message_list.front()->get().message;
+	msg[1] = message_list.front()->get().data;
+	message_list.pop_front();
+	return msg;
+}
+
 void RemoteDebugger::debug(bool p_can_continue, bool p_is_error_breakpoint) {
 	//this function is called when there is a debugger break (bug on script)
 	//or when execution is paused from editor
 
-	if (script_debugger->is_skipping_breakpoints() && !p_is_error_breakpoint) {
-		return;
-	}
+	{
+		MutexLock lock(mutex);
+		// Tests that require mutex.
+		if (script_debugger->is_skipping_breakpoints() && !p_is_error_breakpoint) {
+			return;
+		}
 
-	ERR_FAIL_COND_MSG(!is_peer_connected(), "Script Debugger failed to connect, but being used anyway.");
+		ERR_FAIL_COND_MSG(!is_peer_connected(), "Script Debugger failed to connect, but being used anyway.");
 
-	if (!peer->can_block()) {
-		return; // Peer does not support blocking IO. We could at least send the error though.
+		if (!peer->can_block()) {
+			return; // Peer does not support blocking IO. We could at least send the error though.
+		}
 	}
 
 	ScriptLanguage *script_lang = script_debugger->get_break_language();
@@ -369,22 +417,33 @@ void RemoteDebugger::debug(bool p_can_continue, bool p_is_error_breakpoint) {
 	msg.push_back(error_str);
 	ERR_FAIL_COND(!script_lang);
 	msg.push_back(script_lang->debug_get_stack_level_count() > 0);
+	msg.push_back(Thread::get_caller_id() == Thread::get_main_id() ? String(RTR("Main Thread")) : itos(Thread::get_caller_id()));
 	if (allow_focus_steal_fn) {
 		allow_focus_steal_fn();
 	}
 	send_message("debug_enter", msg);
 
-	Input::MouseMode mouse_mode = Input::get_singleton()->get_mouse_mode();
-	if (mouse_mode != Input::MOUSE_MODE_VISIBLE) {
-		Input::get_singleton()->set_mouse_mode(Input::MOUSE_MODE_VISIBLE);
+	Input::MouseMode mouse_mode = Input::MOUSE_MODE_VISIBLE;
+
+	if (Thread::get_caller_id() == Thread::get_main_id()) {
+		mouse_mode = Input::get_singleton()->get_mouse_mode();
+		if (mouse_mode != Input::MOUSE_MODE_VISIBLE) {
+			Input::get_singleton()->set_mouse_mode(Input::MOUSE_MODE_VISIBLE);
+		}
+	} else {
+		MutexLock mutex_lock(mutex);
+		messages.insert(Thread::get_caller_id(), List<Message>());
 	}
 
+	mutex.lock();
 	while (is_peer_connected()) {
+		mutex.unlock();
 		flush_output();
-		peer->poll();
 
-		if (peer->has_message()) {
-			Array cmd = peer->get_message();
+		_poll_messages();
+
+		if (_has_messages()) {
+			Array cmd = _get_message();
 
 			ERR_CONTINUE(cmd.size() != 2);
 			ERR_CONTINUE(cmd[0].get_type() != Variant::STRING);
@@ -479,14 +538,22 @@ void RemoteDebugger::debug(bool p_can_continue, bool p_is_error_breakpoint) {
 			}
 		} else {
 			OS::get_singleton()->delay_usec(10000);
-			OS::get_singleton()->process_and_drop_events();
+			if (Thread::get_caller_id() == Thread::get_main_id()) {
+				// If this is a busy loop on the main thread, events still need to be processed.
+				OS::get_singleton()->process_and_drop_events();
+			}
 		}
 	}
 
 	send_message("debug_exit", Array());
 
-	if (mouse_mode != Input::MOUSE_MODE_VISIBLE) {
-		Input::get_singleton()->set_mouse_mode(mouse_mode);
+	if (Thread::get_caller_id() == Thread::get_main_id()) {
+		if (mouse_mode != Input::MOUSE_MODE_VISIBLE) {
+			Input::get_singleton()->set_mouse_mode(mouse_mode);
+		}
+	} else {
+		MutexLock mutex_lock(mutex);
+		messages.erase(Thread::get_caller_id());
 	}
 }
 
@@ -496,9 +563,11 @@ void RemoteDebugger::poll_events(bool p_is_idle) {
 	}
 
 	flush_output();
-	peer->poll();
-	while (peer->has_message()) {
-		Array arr = peer->get_message();
+
+	_poll_messages();
+
+	while (_has_messages()) {
+		Array arr = _get_message();
 
 		ERR_CONTINUE(arr.size() != 2);
 		ERR_CONTINUE(arr[0].get_type() != Variant::STRING);
@@ -604,6 +673,8 @@ RemoteDebugger::RemoteDebugger(Ref<RemoteDebuggerPeer> p_peer) {
 	eh.errfunc = _err_handler;
 	eh.userdata = this;
 	add_error_handler(&eh);
+
+	messages.insert(Thread::get_main_id(), List<Message>());
 }
 
 RemoteDebugger::~RemoteDebugger() {

core/debugger/remote_debugger.h

@@ -80,6 +80,17 @@ class RemoteDebugger : public EngineDebugger {
 	bool flushing = false;
 	Thread::ID flush_thread = 0;
 
+	struct Message {
+		String message;
+		Array data;
+	};
+
+	HashMap<Thread::ID, List<Message>> messages;
+
+	void _poll_messages();
+	bool _has_messages();
+	Array _get_message();
+
 	PrintHandlerList phl;
 	static void _print_handler(void *p_this, const String &p_string, bool p_error, bool p_rich);
 	ErrorHandlerList eh;

core/debugger/script_debugger.cpp

@@ -32,22 +32,19 @@
 
 #include "core/debugger/engine_debugger.h"
 
+thread_local int ScriptDebugger::lines_left = -1;
+thread_local int ScriptDebugger::depth = -1;
+thread_local ScriptLanguage *ScriptDebugger::break_lang = nullptr;
+thread_local Vector<ScriptDebugger::StackInfo> ScriptDebugger::error_stack_info;
+
 void ScriptDebugger::set_lines_left(int p_left) {
 	lines_left = p_left;
 }
 
-int ScriptDebugger::get_lines_left() const {
-	return lines_left;
-}
-
 void ScriptDebugger::set_depth(int p_depth) {
 	depth = p_depth;
 }
 
-int ScriptDebugger::get_depth() const {
-	return depth;
-}
-
 void ScriptDebugger::insert_breakpoint(int p_line, const StringName &p_source) {
 	if (!breakpoints.has(p_line)) {
 		breakpoints[p_line] = HashSet<StringName>();
@@ -66,13 +63,6 @@ void ScriptDebugger::remove_breakpoint(int p_line, const StringName &p_source) {
 	}
 }
 
-bool ScriptDebugger::is_breakpoint(int p_line, const StringName &p_source) const {
-	if (!breakpoints.has(p_line)) {
-		return false;
-	}
-	return breakpoints[p_line].has(p_source);
-}
-
 String ScriptDebugger::breakpoint_find_source(const String &p_source) const {
 	return p_source;
 }
@@ -100,7 +90,7 @@ void ScriptDebugger::send_error(const String &p_func, const String &p_file, int
 	// Store stack info, this is ugly, but allows us to separate EngineDebugger and ScriptDebugger. There might be a better way.
 	error_stack_info.append_array(p_stack_info);
 	EngineDebugger::get_singleton()->send_error(p_func, p_file, p_line, p_err, p_descr, p_editor_notify, p_type);
-	error_stack_info.clear();
+	error_stack_info.clear(); // Clear because this is thread local
 }
 
 Vector<ScriptLanguage::StackInfo> ScriptDebugger::get_error_stack_info() const {

core/debugger/script_debugger.h

@@ -40,21 +40,25 @@
 class ScriptDebugger {
 	typedef ScriptLanguage::StackInfo StackInfo;
 
-	int lines_left = -1;
-	int depth = -1;
 	bool skip_breakpoints = false;
 
 	HashMap<int, HashSet<StringName>> breakpoints;
 
-	ScriptLanguage *break_lang = nullptr;
-	Vector<StackInfo> error_stack_info;
+	static thread_local int lines_left;
+	static thread_local int depth;
+	static thread_local ScriptLanguage *break_lang;
+	static thread_local Vector<StackInfo> error_stack_info;
 
 public:
 	void set_lines_left(int p_left);
-	int get_lines_left() const;
+	_ALWAYS_INLINE_ int get_lines_left() const {
+		return lines_left;
+	}
 
 	void set_depth(int p_depth);
-	int get_depth() const;
+	_ALWAYS_INLINE_ int get_depth() const {
+		return depth;
+	}
 
 	String breakpoint_find_source(const String &p_source) const;
 	void set_break_language(ScriptLanguage *p_lang) { break_lang = p_lang; }
@@ -63,7 +67,12 @@ class ScriptDebugger {
 	bool is_skipping_breakpoints();
 	void insert_breakpoint(int p_line, const StringName &p_source);
 	void remove_breakpoint(int p_line, const StringName &p_source);
-	bool is_breakpoint(int p_line, const StringName &p_source) const;
+	_ALWAYS_INLINE_ bool is_breakpoint(int p_line, const StringName &p_source) const {
+		if (likely(!breakpoints.has(p_line))) {
+			return false;
+		}
+		return breakpoints[p_line].has(p_source);
+	}
 	void clear_breakpoints();
 	const HashMap<int, HashSet<StringName>> &get_breakpoints() const { return breakpoints; }
 

doc/classes/Thread.xml

@@ -5,7 +5,6 @@
 	</brief_description>
 	<description>
 		A unit of execution in a process. Can run methods on [Object]s simultaneously. The use of synchronization via [Mutex] or [Semaphore] is advised if working with shared objects.
-		[b]Note:[/b] Breakpoints won't break on code if it's running in a thread. This is a current limitation of the GDScript debugger.
 		[b]Warning:[/b]
 		To ensure proper cleanup without crashes or deadlocks, when a [Thread]'s reference count reaches zero and it is therefore destroyed, the following conditions must be met:
 		- It must not have any [Mutex] objects locked.

editor/debugger/editor_debugger_inspector.cpp

@@ -232,7 +232,7 @@ void EditorDebuggerInspector::add_stack_variable(const Array &p_array) {
 	PropertyHint h = PROPERTY_HINT_NONE;
 	String hs;
 
-	if (v.get_type() == Variant::OBJECT) {
+	if (var.var_type == Variant::OBJECT) {
 		v = Object::cast_to<EncodedObjectAsID>(v)->get_object_id();
 		h = PROPERTY_HINT_OBJECT_ID;
 		hs = "Object";