From 6955f7bb988d21cca546dcf15c6462582c704ebd Mon Sep 17 00:00:00 2001 From: Joel Croteau Date: Wed, 26 Jun 2024 21:34:38 -0600 Subject: [PATCH 01/11] Allow constructing `List`s from `initializer_list`s QOL improvement to allow direct construction of `List`s. --- core/templates/list.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/core/templates/list.h b/core/templates/list.h index 6663f06c30960..a6fe0df314f0b 100644 --- a/core/templates/list.h +++ b/core/templates/list.h @@ -35,6 +35,8 @@ #include "core/os/memory.h" #include "core/templates/sort_array.h" +#include + /** * Generic Templatized Linked List Implementation. * The implementation differs from the STL one because @@ -761,6 +763,12 @@ class List { } } + List(std::initializer_list init) { + for (const auto& i : init) { + push_back(i); + } + } + List() {} ~List() { From a0a886c38d1b8a5206a3f8eb70f81be0bccf463b Mon Sep 17 00:00:00 2001 From: Joel Croteau Date: Wed, 26 Jun 2024 23:00:25 -0600 Subject: [PATCH 02/11] Make `push_error` and `push_warning` print name of calling function Currently, `push_error` and `push_warning` give themselve as the calling function, which is not useful for debugging. I have altered these to use the C stacktrace to get the name of the function that called them. This saves having to turn these functions into macros, which would require recompiling every single piece of code that uses them. I have also set it to, when debug symbols are available, use them with `atos` (available on macOS) to find the exact source file and line being called from. It should be possible to the same thing on Linux using `addr2line`, but I don't have a Linux box handy to test that. I am not sure how you would implement this in Windows. I have tested this on macOS Sonoma 14.5 (23F79). It should work on any *nix system. I am not sure about Windows. Fixes #76770 --- core/error/error_macros.cpp | 65 ++++++++++++++++++++++++++++++++ core/error/error_macros.h | 1 + core/variant/variant_utility.cpp | 4 +- editor/editor_log.cpp | 2 +- 4 files changed, 69 insertions(+), 3 deletions(-) diff --git a/core/error/error_macros.cpp b/core/error/error_macros.cpp index 8376c0aaf84c3..80052a0da979e 100644 --- a/core/error/error_macros.cpp +++ b/core/error/error_macros.cpp @@ -34,6 +34,10 @@ #include "core/os/os.h" #include "core/string/ustring.h" +#include +#include +#include + static ErrorHandlerList *error_handler_list = nullptr; void add_error_handler(ErrorHandlerList *p_handler) { @@ -125,6 +129,67 @@ void _err_print_index_error(const char *p_function, const char *p_file, int p_li _err_print_index_error(p_function, p_file, p_line, p_index, p_size, p_index_str, p_size_str, p_message.utf8().get_data(), p_editor_notify, p_fatal); } +struct FunctionInfo { + const char* function; + const char* file; + int line; + String descriptor; +}; + +FunctionInfo calling_function(const char* filter, int frames_to_skip = 0) { + constexpr int kBacktraceDepth = 15; + constexpr int kDemangledBufferSize = 100; + void* backtrace_addrs[kBacktraceDepth]; + static char s_demangled[kDemangledBufferSize]; + Dl_info info; + FunctionInfo result; + + int trace_size = backtrace(backtrace_addrs, kBacktraceDepth); + + int i = frames_to_skip; + do { + ++i; + dladdr(backtrace_addrs[i], &info); + } while (i < trace_size && strstr(info.dli_sname, filter)); + + int status; + char* demangled = abi::__cxa_demangle(info.dli_sname, nullptr, nullptr, &status); + + if (status == 0) { + // Have to do it this way to avoid a memory leak, as abi::__cxa_demangle returns a `malloc`ed c-string. + strncpy(s_demangled, demangled, kDemangledBufferSize); + s_demangled[kDemangledBufferSize - 1] = 0; + result.function = s_demangled; + } else { + result.function = info.dli_sname; + } + free(demangled); + + result.file = info.dli_fname; + result.line = static_cast(info.dli_saddr) - static_cast(info.dli_fbase); + +#ifdef DEBUG_ENABLED + if (OS::get_singleton()->get_name() == "macOS") { + String pipe; + Error error = OS::get_singleton()->execute("atos", { "-o", info.dli_fname, "-l", + String::num_uint64(reinterpret_cast(info.dli_fbase), 16), + String::num_uint64(reinterpret_cast(backtrace_addrs[i]), 16)}, + &pipe); + + if (error == OK) { + result.descriptor = pipe + " - "; + } + } +#endif + + return result; +} + +void _err_print_error_backtrace(const char *filter, const String& p_error, bool p_editor_notify, ErrorHandlerType p_type) { + FunctionInfo info = calling_function(filter, 1); + _err_print_error(info.function, info.file, info.line, "", info.descriptor + p_error, p_editor_notify, p_type); +} + void _err_flush_stdout() { fflush(stdout); } diff --git a/core/error/error_macros.h b/core/error/error_macros.h index ab7dbcbd44b7d..c80bb98dfb745 100644 --- a/core/error/error_macros.h +++ b/core/error/error_macros.h @@ -69,6 +69,7 @@ void _err_print_error(const char *p_function, const char *p_file, int p_line, co void _err_print_error(const char *p_function, const char *p_file, int p_line, const String &p_error, const String &p_message, bool p_editor_notify = false, ErrorHandlerType p_type = ERR_HANDLER_ERROR); void _err_print_index_error(const char *p_function, const char *p_file, int p_line, int64_t p_index, int64_t p_size, const char *p_index_str, const char *p_size_str, const char *p_message = "", bool p_editor_notify = false, bool fatal = false); void _err_print_index_error(const char *p_function, const char *p_file, int p_line, int64_t p_index, int64_t p_size, const char *p_index_str, const char *p_size_str, const String &p_message, bool p_editor_notify = false, bool fatal = false); +void _err_print_error_backtrace(const char* filter, const String &p_error, bool p_editor_notify = false, ErrorHandlerType p_type = ERR_HANDLER_ERROR); void _err_flush_stdout(); #ifdef __GNUC__ diff --git a/core/variant/variant_utility.cpp b/core/variant/variant_utility.cpp index 7534a154a1c3b..fd5004f2b389b 100644 --- a/core/variant/variant_utility.cpp +++ b/core/variant/variant_utility.cpp @@ -1089,7 +1089,7 @@ void VariantUtilityFunctions::push_error(const Variant **p_args, int p_arg_count } } - ERR_PRINT(s); + _err_print_error_backtrace(FUNCTION_STR, s); r_error.error = Callable::CallError::CALL_OK; } @@ -1109,7 +1109,7 @@ void VariantUtilityFunctions::push_warning(const Variant **p_args, int p_arg_cou } } - WARN_PRINT(s); + _err_print_error_backtrace(FUNCTION_STR, s, false, ERR_HANDLER_WARNING); r_error.error = Callable::CallError::CALL_OK; } diff --git a/editor/editor_log.cpp b/editor/editor_log.cpp index 6a016c217a867..2745f0ca04d3d 100644 --- a/editor/editor_log.cpp +++ b/editor/editor_log.cpp @@ -49,7 +49,7 @@ void EditorLog::_error_handler(void *p_self, const char *p_func, const char *p_f if (p_errorexp && p_errorexp[0]) { err_str = String::utf8(p_errorexp); } else { - err_str = String::utf8(p_file) + ":" + itos(p_line) + " - " + String::utf8(p_error); + err_str = String::utf8(p_func) + " (" + String::utf8(p_file) + ": " + itos(p_line) + ") - " + String::utf8(p_error); } if (p_editor_notify) { From 699c0eb9ce4de8b7d816757eb1d9c24cf4fca47a Mon Sep 17 00:00:00 2001 From: Joel Croteau Date: Thu, 27 Jun 2024 13:34:30 -0600 Subject: [PATCH 03/11] Fix code style issues --- core/error/error_macros.cpp | 20 +++++++++----------- core/error/error_macros.h | 2 +- core/templates/list.h | 2 +- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/core/error/error_macros.cpp b/core/error/error_macros.cpp index 80052a0da979e..c4f548bc82e9a 100644 --- a/core/error/error_macros.cpp +++ b/core/error/error_macros.cpp @@ -130,16 +130,16 @@ void _err_print_index_error(const char *p_function, const char *p_file, int p_li } struct FunctionInfo { - const char* function; - const char* file; + const char *function; + const char *file; int line; String descriptor; }; -FunctionInfo calling_function(const char* filter, int frames_to_skip = 0) { +FunctionInfo calling_function(const char *filter, int frames_to_skip = 0) { constexpr int kBacktraceDepth = 15; constexpr int kDemangledBufferSize = 100; - void* backtrace_addrs[kBacktraceDepth]; + void *backtrace_addrs[kBacktraceDepth]; static char s_demangled[kDemangledBufferSize]; Dl_info info; FunctionInfo result; @@ -153,7 +153,7 @@ FunctionInfo calling_function(const char* filter, int frames_to_skip = 0) { } while (i < trace_size && strstr(info.dli_sname, filter)); int status; - char* demangled = abi::__cxa_demangle(info.dli_sname, nullptr, nullptr, &status); + char *demangled = abi::__cxa_demangle(info.dli_sname, nullptr, nullptr, &status); if (status == 0) { // Have to do it this way to avoid a memory leak, as abi::__cxa_demangle returns a `malloc`ed c-string. @@ -166,15 +166,13 @@ FunctionInfo calling_function(const char* filter, int frames_to_skip = 0) { free(demangled); result.file = info.dli_fname; - result.line = static_cast(info.dli_saddr) - static_cast(info.dli_fbase); + result.line = static_cast(info.dli_saddr) - static_cast(info.dli_fbase); #ifdef DEBUG_ENABLED if (OS::get_singleton()->get_name() == "macOS") { String pipe; - Error error = OS::get_singleton()->execute("atos", { "-o", info.dli_fname, "-l", - String::num_uint64(reinterpret_cast(info.dli_fbase), 16), - String::num_uint64(reinterpret_cast(backtrace_addrs[i]), 16)}, - &pipe); + Error error = OS::get_singleton()->execute("atos", { "-o", info.dli_fname, "-l", String::num_uint64(reinterpret_cast(info.dli_fbase), 16), String::num_uint64(reinterpret_cast(backtrace_addrs[i]), 16) }, + &pipe); if (error == OK) { result.descriptor = pipe + " - "; @@ -185,7 +183,7 @@ FunctionInfo calling_function(const char* filter, int frames_to_skip = 0) { return result; } -void _err_print_error_backtrace(const char *filter, const String& p_error, bool p_editor_notify, ErrorHandlerType p_type) { +void _err_print_error_backtrace(const char *filter, const String &p_error, bool p_editor_notify, ErrorHandlerType p_type) { FunctionInfo info = calling_function(filter, 1); _err_print_error(info.function, info.file, info.line, "", info.descriptor + p_error, p_editor_notify, p_type); } diff --git a/core/error/error_macros.h b/core/error/error_macros.h index c80bb98dfb745..dfcd38dc1c93f 100644 --- a/core/error/error_macros.h +++ b/core/error/error_macros.h @@ -69,7 +69,7 @@ void _err_print_error(const char *p_function, const char *p_file, int p_line, co void _err_print_error(const char *p_function, const char *p_file, int p_line, const String &p_error, const String &p_message, bool p_editor_notify = false, ErrorHandlerType p_type = ERR_HANDLER_ERROR); void _err_print_index_error(const char *p_function, const char *p_file, int p_line, int64_t p_index, int64_t p_size, const char *p_index_str, const char *p_size_str, const char *p_message = "", bool p_editor_notify = false, bool fatal = false); void _err_print_index_error(const char *p_function, const char *p_file, int p_line, int64_t p_index, int64_t p_size, const char *p_index_str, const char *p_size_str, const String &p_message, bool p_editor_notify = false, bool fatal = false); -void _err_print_error_backtrace(const char* filter, const String &p_error, bool p_editor_notify = false, ErrorHandlerType p_type = ERR_HANDLER_ERROR); +void _err_print_error_backtrace(const char *filter, const String &p_error, bool p_editor_notify = false, ErrorHandlerType p_type = ERR_HANDLER_ERROR); void _err_flush_stdout(); #ifdef __GNUC__ diff --git a/core/templates/list.h b/core/templates/list.h index a6fe0df314f0b..9908565d1adef 100644 --- a/core/templates/list.h +++ b/core/templates/list.h @@ -764,7 +764,7 @@ class List { } List(std::initializer_list init) { - for (const auto& i : init) { + for (const T &i : init) { push_back(i); } } From 4c5e93d77f8b874e48458025af2b20921a8ddd3c Mon Sep 17 00:00:00 2001 From: Joel Croteau Date: Thu, 27 Jun 2024 16:51:15 -0600 Subject: [PATCH 04/11] Move C++ code line discovery to its own function Also add method to print whole callstack. --- core/error/error_macros.cpp | 49 +++++++++++++++++++++++++------------ core/error/error_macros.h | 1 + 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/core/error/error_macros.cpp b/core/error/error_macros.cpp index c4f548bc82e9a..3a4a7c7bfd053 100644 --- a/core/error/error_macros.cpp +++ b/core/error/error_macros.cpp @@ -136,22 +136,10 @@ struct FunctionInfo { String descriptor; }; -FunctionInfo calling_function(const char *filter, int frames_to_skip = 0) { - constexpr int kBacktraceDepth = 15; +FunctionInfo describe_function(const Dl_info &info, const void *address) { + FunctionInfo result; constexpr int kDemangledBufferSize = 100; - void *backtrace_addrs[kBacktraceDepth]; static char s_demangled[kDemangledBufferSize]; - Dl_info info; - FunctionInfo result; - - int trace_size = backtrace(backtrace_addrs, kBacktraceDepth); - - int i = frames_to_skip; - do { - ++i; - dladdr(backtrace_addrs[i], &info); - } while (i < trace_size && strstr(info.dli_sname, filter)); - int status; char *demangled = abi::__cxa_demangle(info.dli_sname, nullptr, nullptr, &status); @@ -171,7 +159,7 @@ FunctionInfo calling_function(const char *filter, int frames_to_skip = 0) { #ifdef DEBUG_ENABLED if (OS::get_singleton()->get_name() == "macOS") { String pipe; - Error error = OS::get_singleton()->execute("atos", { "-o", info.dli_fname, "-l", String::num_uint64(reinterpret_cast(info.dli_fbase), 16), String::num_uint64(reinterpret_cast(backtrace_addrs[i]), 16) }, + Error error = OS::get_singleton()->execute("atos", { "-o", info.dli_fname, "-l", String::num_uint64(reinterpret_cast(info.dli_fbase), 16), String::num_uint64(reinterpret_cast(address), 16) }, &pipe); if (error == OK) { @@ -183,6 +171,37 @@ FunctionInfo calling_function(const char *filter, int frames_to_skip = 0) { return result; } +FunctionInfo calling_function(const char *filter, int frames_to_skip = 0) { + constexpr int kBacktraceDepth = 15; + void *backtrace_addrs[kBacktraceDepth]; + Dl_info info; + + int trace_size = backtrace(backtrace_addrs, kBacktraceDepth); + + int i = frames_to_skip; + do { + ++i; + dladdr(backtrace_addrs[i], &info); + } while (i < trace_size && strstr(info.dli_sname, filter)); + + return describe_function(info, backtrace_addrs[i]); +} + +void _err_print_callstack(const String &p_error, bool p_editor_notify, ErrorHandlerType p_type) { + constexpr int kBacktraceDepth = 25; + void *backtrace_addrs[kBacktraceDepth]; + Dl_info info; + + int trace_size = backtrace(backtrace_addrs, kBacktraceDepth); + + for (int i = 0; i < trace_size; ++i) { + dladdr(backtrace_addrs[i], &info); + + FunctionInfo func_info = describe_function(info, backtrace_addrs[i]); + _err_print_error(func_info.function, func_info.file, func_info.line, "", func_info.descriptor + p_error, p_editor_notify, p_type); + } +} + void _err_print_error_backtrace(const char *filter, const String &p_error, bool p_editor_notify, ErrorHandlerType p_type) { FunctionInfo info = calling_function(filter, 1); _err_print_error(info.function, info.file, info.line, "", info.descriptor + p_error, p_editor_notify, p_type); diff --git a/core/error/error_macros.h b/core/error/error_macros.h index dfcd38dc1c93f..2708ce1b18245 100644 --- a/core/error/error_macros.h +++ b/core/error/error_macros.h @@ -69,6 +69,7 @@ void _err_print_error(const char *p_function, const char *p_file, int p_line, co void _err_print_error(const char *p_function, const char *p_file, int p_line, const String &p_error, const String &p_message, bool p_editor_notify = false, ErrorHandlerType p_type = ERR_HANDLER_ERROR); void _err_print_index_error(const char *p_function, const char *p_file, int p_line, int64_t p_index, int64_t p_size, const char *p_index_str, const char *p_size_str, const char *p_message = "", bool p_editor_notify = false, bool fatal = false); void _err_print_index_error(const char *p_function, const char *p_file, int p_line, int64_t p_index, int64_t p_size, const char *p_index_str, const char *p_size_str, const String &p_message, bool p_editor_notify = false, bool fatal = false); +void _err_print_callstack(const String &p_error, bool p_editor_notify = false, ErrorHandlerType p_type = ERR_HANDLER_ERROR); void _err_print_error_backtrace(const char *filter, const String &p_error, bool p_editor_notify = false, ErrorHandlerType p_type = ERR_HANDLER_ERROR); void _err_flush_stdout(); From cab9969d3521277790db8735bc8aa47738779830 Mon Sep 17 00:00:00 2001 From: Joel Croteau Date: Thu, 27 Jun 2024 22:50:08 -0600 Subject: [PATCH 05/11] Move platform dependent code into `OS` class and fix stacktrace from scripts --- core/error/error_macros.cpp | 120 +++++++++++++++--------------------- core/os/os.h | 10 +++ drivers/unix/os_unix.cpp | 43 +++++++++++++ drivers/unix/os_unix.h | 4 ++ platform/macos/os_macos.h | 4 ++ platform/macos/os_macos.mm | 13 ++++ 6 files changed, 124 insertions(+), 70 deletions(-) diff --git a/core/error/error_macros.cpp b/core/error/error_macros.cpp index 3a4a7c7bfd053..a5430a5693897 100644 --- a/core/error/error_macros.cpp +++ b/core/error/error_macros.cpp @@ -31,13 +31,10 @@ #include "error_macros.h" #include "core/io/logger.h" +#include "core/object/script_language.h" #include "core/os/os.h" #include "core/string/ustring.h" -#include -#include -#include - static ErrorHandlerList *error_handler_list = nullptr; void add_error_handler(ErrorHandlerList *p_handler) { @@ -129,82 +126,65 @@ void _err_print_index_error(const char *p_function, const char *p_file, int p_li _err_print_index_error(p_function, p_file, p_line, p_index, p_size, p_index_str, p_size_str, p_message.utf8().get_data(), p_editor_notify, p_fatal); } -struct FunctionInfo { - const char *function; - const char *file; - int line; - String descriptor; -}; - -FunctionInfo describe_function(const Dl_info &info, const void *address) { - FunctionInfo result; - constexpr int kDemangledBufferSize = 100; - static char s_demangled[kDemangledBufferSize]; - int status; - char *demangled = abi::__cxa_demangle(info.dli_sname, nullptr, nullptr, &status); - - if (status == 0) { - // Have to do it this way to avoid a memory leak, as abi::__cxa_demangle returns a `malloc`ed c-string. - strncpy(s_demangled, demangled, kDemangledBufferSize); - s_demangled[kDemangledBufferSize - 1] = 0; - result.function = s_demangled; - } else { - result.function = info.dli_sname; - } - free(demangled); - - result.file = info.dli_fname; - result.line = static_cast(info.dli_saddr) - static_cast(info.dli_fbase); - -#ifdef DEBUG_ENABLED - if (OS::get_singleton()->get_name() == "macOS") { - String pipe; - Error error = OS::get_singleton()->execute("atos", { "-o", info.dli_fname, "-l", String::num_uint64(reinterpret_cast(info.dli_fbase), 16), String::num_uint64(reinterpret_cast(address), 16) }, - &pipe); - - if (error == OK) { - result.descriptor = pipe + " - "; +void _err_print_callstack(const String &p_error, bool p_editor_notify, ErrorHandlerType p_type) { + // Print detailed call stack information from everywhere available. It is recommended to only + // use this for debugging, as it has a fairly high overhead. + String callstack; + + // Print script stack frames, if available. + Vector si; + for (int i = 0; i < ScriptServer::get_language_count(); i++) { + si = ScriptServer::get_language(i)->debug_get_current_stack_info(); + if (si.size()) { + callstack += "Callstack from " + ScriptServer::get_language(i)->get_name() + ":\n"; + for (int j = 0; j < si.size(); ++j) { + callstack += si[i].file + ':' + itos(si[i].line) + " @ " + si[i].func + '\n'; + } + callstack += '\n'; } } -#endif - - return result; -} - -FunctionInfo calling_function(const char *filter, int frames_to_skip = 0) { - constexpr int kBacktraceDepth = 15; - void *backtrace_addrs[kBacktraceDepth]; - Dl_info info; - - int trace_size = backtrace(backtrace_addrs, kBacktraceDepth); - int i = frames_to_skip; - do { - ++i; - dladdr(backtrace_addrs[i], &info); - } while (i < trace_size && strstr(info.dli_sname, filter)); + // Print C++ call stack. + Vector cpp_stack = OS::get_singleton()->get_cpp_stack_info(); + callstack += "C++ call stack:\n"; + for (int i = 0; i < cpp_stack.size(); ++i) { + String descriptor = OS::get_singleton()->get_debug_descriptor(cpp_stack[i]); + callstack += descriptor + " (" + cpp_stack[i].file + ":0x" + String::num_uint64(cpp_stack[i].offset, 16) + " @ " + cpp_stack[i].function + ")\n"; + } - return describe_function(info, backtrace_addrs[i]); + _err_print_error(__FUNCTION__, __FILE__, __LINE__, p_error + '\n' + callstack, p_editor_notify, p_type); } -void _err_print_callstack(const String &p_error, bool p_editor_notify, ErrorHandlerType p_type) { - constexpr int kBacktraceDepth = 25; - void *backtrace_addrs[kBacktraceDepth]; - Dl_info info; - - int trace_size = backtrace(backtrace_addrs, kBacktraceDepth); +void _err_print_error_backtrace(const char *filter, const String &p_error, bool p_editor_notify, ErrorHandlerType p_type) { + // Print script stack frame, if available. + Vector si; + for (int i = 0; i < ScriptServer::get_language_count(); i++) { + si = ScriptServer::get_language(i)->debug_get_current_stack_info(); + if (si.size()) { + _err_print_error(si[0].func.utf8(), si[0].file.utf8(), si[0].line, p_error, p_editor_notify, p_type); + return; + } + } - for (int i = 0; i < trace_size; ++i) { - dladdr(backtrace_addrs[i], &info); + // If there is not a script stack frame, use the C++ stack frame. + Vector cpp_stack = OS::get_singleton()->get_cpp_stack_info(); - FunctionInfo func_info = describe_function(info, backtrace_addrs[i]); - _err_print_error(func_info.function, func_info.file, func_info.line, "", func_info.descriptor + p_error, p_editor_notify, p_type); + for (int i = 1; i < cpp_stack.size(); ++i) { + if (!cpp_stack[i].function.contains(filter)) { + String descriptor = OS::get_singleton()->get_debug_descriptor(cpp_stack[i]); + if (descriptor.is_empty()) { + // If we can't get debug info, just print binary file name and address. + _err_print_error(cpp_stack[i].function.utf8(), cpp_stack[i].file.utf8(), cpp_stack[i].offset, p_error, p_editor_notify, p_type); + } else { + // Expect debug descriptor to replace file and line info. + _err_print_error(cpp_stack[i].function.utf8(), cpp_stack[i].file.utf8(), cpp_stack[i].offset, "", descriptor + ": " + p_error, p_editor_notify, p_type); + } + return; + } } -} -void _err_print_error_backtrace(const char *filter, const String &p_error, bool p_editor_notify, ErrorHandlerType p_type) { - FunctionInfo info = calling_function(filter, 1); - _err_print_error(info.function, info.file, info.line, "", info.descriptor + p_error, p_editor_notify, p_type); + // If there is no usable stack frame (this should basically never happen), fall back to using the current stack frame. + _err_print_error(__FUNCTION__, __FILE__, __LINE__, p_error, p_editor_notify, p_type); } void _err_flush_stdout() { diff --git a/core/os/os.h b/core/os/os.h index 63cc6ed50e632..86257ed092659 100644 --- a/core/os/os.h +++ b/core/os/os.h @@ -339,6 +339,16 @@ class OS { virtual PreferredTextureFormat get_preferred_texture_format() const; + struct StackInfo { + String function; + String file; + uint64_t offset; + const void *load_address; + const void *symbol_address; + }; + virtual Vector get_cpp_stack_info() const { return Vector(); } + virtual String get_debug_descriptor(const StackInfo &info) { return String(); } + // Load GDExtensions specific to this platform. // This is invoked by the GDExtensionManager after loading GDExtensions specified by the project. virtual void load_platform_gdextensions() const {} diff --git a/drivers/unix/os_unix.cpp b/drivers/unix/os_unix.cpp index ce2553456d91e..e13487a40da96 100644 --- a/drivers/unix/os_unix.cpp +++ b/drivers/unix/os_unix.cpp @@ -68,8 +68,10 @@ #include #endif +#include #include #include +#include #include #include #include @@ -1006,6 +1008,47 @@ void UnixTerminalLogger::log_error(const char *p_function, const char *p_file, i } } +OS::StackInfo OS_Unix::describe_function(const Dl_info &info, const void *address) const { + StackInfo result; + + // Demangle C++ symbols + int status; + char *demangled = abi::__cxa_demangle(info.dli_sname, nullptr, nullptr, &status); + + if (status == 0) { + result.function = demangled; + } else { + result.function = info.dli_sname; + } + free(demangled); + + // Get file info + result.file = info.dli_fname; + result.offset = static_cast(address) - static_cast(info.dli_fbase); + result.load_address = info.dli_fbase; + result.symbol_address = address; + + return result; +} + +Vector OS_Unix::get_cpp_stack_info() const { + constexpr int kMaxBacktraceDepth = 25; + void *backtrace_addrs[kMaxBacktraceDepth]; + int trace_size = backtrace(backtrace_addrs, kMaxBacktraceDepth); + + Vector result; + result.resize(trace_size - 1); + + // Skip the current stack frame and return the stack frame of the calling function + for (int i = 1; i < trace_size; ++i) { + Dl_info info; + dladdr(backtrace_addrs[i], &info); + result.write[i - 1] = describe_function(info, backtrace_addrs[i]); + } + + return result; +} + UnixTerminalLogger::~UnixTerminalLogger() {} OS_Unix::OS_Unix() { diff --git a/drivers/unix/os_unix.h b/drivers/unix/os_unix.h index df269a59d32a5..afc4bceaf5b2c 100644 --- a/drivers/unix/os_unix.h +++ b/drivers/unix/os_unix.h @@ -53,6 +53,8 @@ class OS_Unix : public OS { virtual void finalize_core() override; + virtual StackInfo describe_function(const struct dl_info &info, const void *address) const; + public: OS_Unix(); @@ -101,6 +103,8 @@ class OS_Unix : public OS { virtual String get_executable_path() const override; virtual String get_user_data_dir() const override; + + virtual Vector get_cpp_stack_info() const override; }; class UnixTerminalLogger : public StdLogger { diff --git a/platform/macos/os_macos.h b/platform/macos/os_macos.h index 912a682a6bbc6..1dee461492be9 100644 --- a/platform/macos/os_macos.h +++ b/platform/macos/os_macos.h @@ -75,6 +75,10 @@ class OS_MacOS : public OS_Unix { virtual void set_main_loop(MainLoop *p_main_loop) override; virtual void delete_main_loop() override; +#ifdef DEBUG_ENABLED + virtual String get_debug_descriptor(const StackInfo &info) override; +#endif + public: virtual void set_cmdline_platform_args(const List &p_args); virtual List get_cmdline_platform_args() const override; diff --git a/platform/macos/os_macos.mm b/platform/macos/os_macos.mm index 9f0bea5951c35..6dac070f89311 100644 --- a/platform/macos/os_macos.mm +++ b/platform/macos/os_macos.mm @@ -757,6 +757,19 @@ return PREFERRED_TEXTURE_FORMAT_S3TC_BPTC; } +#ifdef DEBUG_ENABLED +String OS_MacOS::get_debug_descriptor(const StackInfo &info) { + String pipe; + Error error = execute("atos", { "-o", info.file, "-l", String::num_uint64(reinterpret_cast(info.load_address), 16), String::num_uint64(reinterpret_cast(info.symbol_address), 16) }, &pipe); + + if (error == OK) { + return pipe.strip_edges(); + } else { + return String(); + } +} +#endif + void OS_MacOS::run() { if (!main_loop) { return; From 7d42962e589f365bce67dd9f4ef2d805aaefdb1c Mon Sep 17 00:00:00 2001 From: Joel Croteau Date: Fri, 28 Jun 2024 00:14:51 -0600 Subject: [PATCH 06/11] Try fix for Linux builds The Linux build breaks on `OS_Unix` because `Dl_info` is not `typedef`ed as `struct dl_info`, so the forward declaration of that struct breaks. Changing the forward declaration to `struct Dl_info` breaks the Mac build though, so that leaves us with some `#ifdef`s. This may or may not work, I'll have to see. --- drivers/unix/os_unix.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/unix/os_unix.h b/drivers/unix/os_unix.h index afc4bceaf5b2c..0fed939e997af 100644 --- a/drivers/unix/os_unix.h +++ b/drivers/unix/os_unix.h @@ -53,7 +53,11 @@ class OS_Unix : public OS { virtual void finalize_core() override; +#ifdef __APPLE__ virtual StackInfo describe_function(const struct dl_info &info, const void *address) const; +#else + virtual StackInfo describe_function(const struct Dl_info &info, const void *address) const; +#endif public: OS_Unix(); From 07e29f76f0ed6f37626f0583899ea23821e2c145 Mon Sep 17 00:00:00 2001 From: Joel Croteau Date: Fri, 28 Jun 2024 00:42:09 -0600 Subject: [PATCH 07/11] Actual fix for Linux builds Okay, there doesn't actually seem to be a way to forward declare a `typedef` to an anonymous `struct`. So, rather than include an unnecessary system header in `os_unix.h`, I am now just passing the members of `Dl_info` individually. This will be slightly less efficient, but this code is generally not called in performance-critical paths, and this should be clean, maintainable, and platform-independent. --- drivers/unix/os_unix.cpp | 14 +++++++------- drivers/unix/os_unix.h | 6 +----- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/drivers/unix/os_unix.cpp b/drivers/unix/os_unix.cpp index e13487a40da96..97702eabdb843 100644 --- a/drivers/unix/os_unix.cpp +++ b/drivers/unix/os_unix.cpp @@ -1008,24 +1008,24 @@ void UnixTerminalLogger::log_error(const char *p_function, const char *p_file, i } } -OS::StackInfo OS_Unix::describe_function(const Dl_info &info, const void *address) const { +OS::StackInfo OS_Unix::describe_function(const char *dli_fname, const void *dli_fbase, const char *dli_sname, const void *dli_saddr, const void *address) const { StackInfo result; // Demangle C++ symbols int status; - char *demangled = abi::__cxa_demangle(info.dli_sname, nullptr, nullptr, &status); + char *demangled = abi::__cxa_demangle(dli_sname, nullptr, nullptr, &status); if (status == 0) { result.function = demangled; } else { - result.function = info.dli_sname; + result.function = dli_sname; } free(demangled); // Get file info - result.file = info.dli_fname; - result.offset = static_cast(address) - static_cast(info.dli_fbase); - result.load_address = info.dli_fbase; + result.file = dli_fname; + result.offset = static_cast(address) - static_cast(dli_fbase); + result.load_address = dli_fbase; result.symbol_address = address; return result; @@ -1043,7 +1043,7 @@ Vector OS_Unix::get_cpp_stack_info() const { for (int i = 1; i < trace_size; ++i) { Dl_info info; dladdr(backtrace_addrs[i], &info); - result.write[i - 1] = describe_function(info, backtrace_addrs[i]); + result.write[i - 1] = describe_function(info.dli_fname, info.dli_fbase, info.dli_sname, info.dli_saddr, backtrace_addrs[i]); } return result; diff --git a/drivers/unix/os_unix.h b/drivers/unix/os_unix.h index 0fed939e997af..aafac09eb9dad 100644 --- a/drivers/unix/os_unix.h +++ b/drivers/unix/os_unix.h @@ -53,11 +53,7 @@ class OS_Unix : public OS { virtual void finalize_core() override; -#ifdef __APPLE__ - virtual StackInfo describe_function(const struct dl_info &info, const void *address) const; -#else - virtual StackInfo describe_function(const struct Dl_info &info, const void *address) const; -#endif + virtual StackInfo describe_function(const char *dli_fname, const void *dli_fbase, const char *dli_sname, const void *dli_saddr, const void *address) const; public: OS_Unix(); From 8ac389d4d677ce41a3ad629b2cbe0469797c4c59 Mon Sep 17 00:00:00 2001 From: Joel Croteau Date: Fri, 28 Jun 2024 01:17:37 -0600 Subject: [PATCH 08/11] Fix Android and Web builds Neither Android's NDK nor Emscripten provide `backtrace` through `execinfo.h`. There are other ways to get a stacktrace on these platforms, but I am not expert enough to implement them, so leave them unimplemented for now. --- drivers/unix/os_unix.cpp | 7 ++++++- drivers/unix/os_unix.h | 2 ++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/unix/os_unix.cpp b/drivers/unix/os_unix.cpp index 97702eabdb843..2c49e116e310f 100644 --- a/drivers/unix/os_unix.cpp +++ b/drivers/unix/os_unix.cpp @@ -68,10 +68,13 @@ #include #endif +#if !defined(__ANDROID__) && !defined(__EMSCRIPTEN__) +#include +#endif + #include #include #include -#include #include #include #include @@ -1031,6 +1034,7 @@ OS::StackInfo OS_Unix::describe_function(const char *dli_fname, const void *dli_ return result; } +#if !defined(__ANDROID__) && !defined(__EMSCRIPTEN__) Vector OS_Unix::get_cpp_stack_info() const { constexpr int kMaxBacktraceDepth = 25; void *backtrace_addrs[kMaxBacktraceDepth]; @@ -1048,6 +1052,7 @@ Vector OS_Unix::get_cpp_stack_info() const { return result; } +#endif UnixTerminalLogger::~UnixTerminalLogger() {} diff --git a/drivers/unix/os_unix.h b/drivers/unix/os_unix.h index aafac09eb9dad..a695a64f5c633 100644 --- a/drivers/unix/os_unix.h +++ b/drivers/unix/os_unix.h @@ -104,7 +104,9 @@ class OS_Unix : public OS { virtual String get_executable_path() const override; virtual String get_user_data_dir() const override; +#if !defined(__ANDROID__) && !defined(__EMSCRIPTEN__) virtual Vector get_cpp_stack_info() const override; +#endif }; class UnixTerminalLogger : public StdLogger { From 029246d8b6645a5f705f2ccd6beb5518f39aae21 Mon Sep 17 00:00:00 2001 From: Joel Croteau Date: Tue, 2 Jul 2024 22:43:57 -0600 Subject: [PATCH 09/11] Remove unnecessary `i == 0` checks from string building loops `String` is auto-initialized to be empty and `String::operator+=` [already checks](https://github.com/godotengine/godot/blob/f0d15bbfdfde1c1076903afb7a7db373580d5534/core/string/ustring.cpp#L520) if it is empty before appending, so this check is pointless and just needlessly clutters the codebase. --- core/variant/variant_utility.cpp | 56 +++++--------------------------- 1 file changed, 8 insertions(+), 48 deletions(-) diff --git a/core/variant/variant_utility.cpp b/core/variant/variant_utility.cpp index fd5004f2b389b..f3291efad881f 100644 --- a/core/variant/variant_utility.cpp +++ b/core/variant/variant_utility.cpp @@ -933,12 +933,7 @@ String VariantUtilityFunctions::str(const Variant **p_args, int p_arg_count, Cal String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - - if (i == 0) { - s = os; - } else { - s += os; - } + s += os; } r_error.error = Callable::CallError::CALL_OK; @@ -963,12 +958,7 @@ void VariantUtilityFunctions::print(const Variant **p_args, int p_arg_count, Cal String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - - if (i == 0) { - s = os; - } else { - s += os; - } + s += os; } print_line(s); @@ -979,12 +969,7 @@ void VariantUtilityFunctions::print_rich(const Variant **p_args, int p_arg_count String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - - if (i == 0) { - s = os; - } else { - s += os; - } + s += os; } print_line_rich(s); @@ -998,12 +983,7 @@ void VariantUtilityFunctions::print_verbose(const Variant **p_args, int p_arg_co String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - - if (i == 0) { - s = os; - } else { - s += os; - } + s += os; } // No need to use `print_verbose()` as this call already only happens @@ -1019,12 +999,7 @@ void VariantUtilityFunctions::printerr(const Variant **p_args, int p_arg_count, String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - - if (i == 0) { - s = os; - } else { - s += os; - } + s += os; } print_error(s); @@ -1061,12 +1036,7 @@ void VariantUtilityFunctions::printraw(const Variant **p_args, int p_arg_count, String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - - if (i == 0) { - s = os; - } else { - s += os; - } + s += os; } OS::get_singleton()->print("%s", s.utf8().get_data()); @@ -1081,12 +1051,7 @@ void VariantUtilityFunctions::push_error(const Variant **p_args, int p_arg_count String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - - if (i == 0) { - s = os; - } else { - s += os; - } + s += os; } _err_print_error_backtrace(FUNCTION_STR, s); @@ -1101,12 +1066,7 @@ void VariantUtilityFunctions::push_warning(const Variant **p_args, int p_arg_cou String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - - if (i == 0) { - s = os; - } else { - s += os; - } + s += os; } _err_print_error_backtrace(FUNCTION_STR, s, false, ERR_HANDLER_WARNING); From 062a9248111688721a95249db2eef0e479682e4e Mon Sep 17 00:00:00 2001 From: Joel Croteau Date: Wed, 3 Jul 2024 13:45:42 -0600 Subject: [PATCH 10/11] Revert "Remove unnecessary `i == 0` checks from string building loops" This reverts commit 029246d8b6645a5f705f2ccd6beb5518f39aae21. --- core/variant/variant_utility.cpp | 56 +++++++++++++++++++++++++++----- 1 file changed, 48 insertions(+), 8 deletions(-) diff --git a/core/variant/variant_utility.cpp b/core/variant/variant_utility.cpp index f3291efad881f..fd5004f2b389b 100644 --- a/core/variant/variant_utility.cpp +++ b/core/variant/variant_utility.cpp @@ -933,7 +933,12 @@ String VariantUtilityFunctions::str(const Variant **p_args, int p_arg_count, Cal String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - s += os; + + if (i == 0) { + s = os; + } else { + s += os; + } } r_error.error = Callable::CallError::CALL_OK; @@ -958,7 +963,12 @@ void VariantUtilityFunctions::print(const Variant **p_args, int p_arg_count, Cal String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - s += os; + + if (i == 0) { + s = os; + } else { + s += os; + } } print_line(s); @@ -969,7 +979,12 @@ void VariantUtilityFunctions::print_rich(const Variant **p_args, int p_arg_count String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - s += os; + + if (i == 0) { + s = os; + } else { + s += os; + } } print_line_rich(s); @@ -983,7 +998,12 @@ void VariantUtilityFunctions::print_verbose(const Variant **p_args, int p_arg_co String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - s += os; + + if (i == 0) { + s = os; + } else { + s += os; + } } // No need to use `print_verbose()` as this call already only happens @@ -999,7 +1019,12 @@ void VariantUtilityFunctions::printerr(const Variant **p_args, int p_arg_count, String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - s += os; + + if (i == 0) { + s = os; + } else { + s += os; + } } print_error(s); @@ -1036,7 +1061,12 @@ void VariantUtilityFunctions::printraw(const Variant **p_args, int p_arg_count, String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - s += os; + + if (i == 0) { + s = os; + } else { + s += os; + } } OS::get_singleton()->print("%s", s.utf8().get_data()); @@ -1051,7 +1081,12 @@ void VariantUtilityFunctions::push_error(const Variant **p_args, int p_arg_count String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - s += os; + + if (i == 0) { + s = os; + } else { + s += os; + } } _err_print_error_backtrace(FUNCTION_STR, s); @@ -1066,7 +1101,12 @@ void VariantUtilityFunctions::push_warning(const Variant **p_args, int p_arg_cou String s; for (int i = 0; i < p_arg_count; i++) { String os = p_args[i]->operator String(); - s += os; + + if (i == 0) { + s = os; + } else { + s += os; + } } _err_print_error_backtrace(FUNCTION_STR, s, false, ERR_HANDLER_WARNING); From 0d5a775cff37e16888f9564e585aecafe19c2ca8 Mon Sep 17 00:00:00 2001 From: Joel Croteau Date: Wed, 3 Jul 2024 14:04:06 -0600 Subject: [PATCH 11/11] Prefix arguments with `p_` Incorporate suggestions from @AThousandShips and necessary code changes to make those work. --- core/error/error_macros.cpp | 4 ++-- core/error/error_macros.h | 2 +- core/os/os.h | 2 +- core/templates/list.h | 4 ++-- platform/macos/os_macos.h | 2 +- platform/macos/os_macos.mm | 4 ++-- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/core/error/error_macros.cpp b/core/error/error_macros.cpp index a5430a5693897..1b7160b83b288 100644 --- a/core/error/error_macros.cpp +++ b/core/error/error_macros.cpp @@ -155,7 +155,7 @@ void _err_print_callstack(const String &p_error, bool p_editor_notify, ErrorHand _err_print_error(__FUNCTION__, __FILE__, __LINE__, p_error + '\n' + callstack, p_editor_notify, p_type); } -void _err_print_error_backtrace(const char *filter, const String &p_error, bool p_editor_notify, ErrorHandlerType p_type) { +void _err_print_error_backtrace(const char *p_filter, const String &p_error, bool p_editor_notify, ErrorHandlerType p_type) { // Print script stack frame, if available. Vector si; for (int i = 0; i < ScriptServer::get_language_count(); i++) { @@ -170,7 +170,7 @@ void _err_print_error_backtrace(const char *filter, const String &p_error, bool Vector cpp_stack = OS::get_singleton()->get_cpp_stack_info(); for (int i = 1; i < cpp_stack.size(); ++i) { - if (!cpp_stack[i].function.contains(filter)) { + if (!cpp_stack[i].function.contains(p_filter)) { String descriptor = OS::get_singleton()->get_debug_descriptor(cpp_stack[i]); if (descriptor.is_empty()) { // If we can't get debug info, just print binary file name and address. diff --git a/core/error/error_macros.h b/core/error/error_macros.h index 2708ce1b18245..ec9740af00977 100644 --- a/core/error/error_macros.h +++ b/core/error/error_macros.h @@ -70,7 +70,7 @@ void _err_print_error(const char *p_function, const char *p_file, int p_line, co void _err_print_index_error(const char *p_function, const char *p_file, int p_line, int64_t p_index, int64_t p_size, const char *p_index_str, const char *p_size_str, const char *p_message = "", bool p_editor_notify = false, bool fatal = false); void _err_print_index_error(const char *p_function, const char *p_file, int p_line, int64_t p_index, int64_t p_size, const char *p_index_str, const char *p_size_str, const String &p_message, bool p_editor_notify = false, bool fatal = false); void _err_print_callstack(const String &p_error, bool p_editor_notify = false, ErrorHandlerType p_type = ERR_HANDLER_ERROR); -void _err_print_error_backtrace(const char *filter, const String &p_error, bool p_editor_notify = false, ErrorHandlerType p_type = ERR_HANDLER_ERROR); +void _err_print_error_backtrace(const char *p_filter, const String &p_error, bool p_editor_notify = false, ErrorHandlerType p_type = ERR_HANDLER_ERROR); void _err_flush_stdout(); #ifdef __GNUC__ diff --git a/core/os/os.h b/core/os/os.h index 86257ed092659..798ecffe73dba 100644 --- a/core/os/os.h +++ b/core/os/os.h @@ -347,7 +347,7 @@ class OS { const void *symbol_address; }; virtual Vector get_cpp_stack_info() const { return Vector(); } - virtual String get_debug_descriptor(const StackInfo &info) { return String(); } + virtual String get_debug_descriptor(const StackInfo &p_info) { return String(); } // Load GDExtensions specific to this platform. // This is invoked by the GDExtensionManager after loading GDExtensions specified by the project. diff --git a/core/templates/list.h b/core/templates/list.h index 9908565d1adef..c8cdc574209e8 100644 --- a/core/templates/list.h +++ b/core/templates/list.h @@ -763,8 +763,8 @@ class List { } } - List(std::initializer_list init) { - for (const T &i : init) { + List(std::initializer_list p_init) { + for (const T &i : p_init) { push_back(i); } } diff --git a/platform/macos/os_macos.h b/platform/macos/os_macos.h index 1dee461492be9..4d1cd1325e9b9 100644 --- a/platform/macos/os_macos.h +++ b/platform/macos/os_macos.h @@ -76,7 +76,7 @@ class OS_MacOS : public OS_Unix { virtual void delete_main_loop() override; #ifdef DEBUG_ENABLED - virtual String get_debug_descriptor(const StackInfo &info) override; + virtual String get_debug_descriptor(const StackInfo &p_info) override; #endif public: diff --git a/platform/macos/os_macos.mm b/platform/macos/os_macos.mm index 6dac070f89311..8631bd85f3acc 100644 --- a/platform/macos/os_macos.mm +++ b/platform/macos/os_macos.mm @@ -758,9 +758,9 @@ } #ifdef DEBUG_ENABLED -String OS_MacOS::get_debug_descriptor(const StackInfo &info) { +String OS_MacOS::get_debug_descriptor(const StackInfo &p_info) { String pipe; - Error error = execute("atos", { "-o", info.file, "-l", String::num_uint64(reinterpret_cast(info.load_address), 16), String::num_uint64(reinterpret_cast(info.symbol_address), 16) }, &pipe); + Error error = execute("atos", { "-o", p_info.file, "-l", String::num_uint64(reinterpret_cast(p_info.load_address), 16), String::num_uint64(reinterpret_cast(p_info.symbol_address), 16) }, &pipe); if (error == OK) { return pipe.strip_edges();