Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make push_error and push_warning print name of calling function #93648

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
62 changes: 62 additions & 0 deletions core/error/error_macros.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#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"

Expand Down Expand Up @@ -125,6 +126,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);
}

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<ScriptLanguage::StackInfo> 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';
}
}

// Print C++ call stack.
Vector<OS::StackInfo> 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";
}

_err_print_error(__FUNCTION__, __FILE__, __LINE__, p_error + '\n' + callstack, p_editor_notify, 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<ScriptLanguage::StackInfo> 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;
}
}

// If there is not a script stack frame, use the C++ stack frame.
Vector<OS::StackInfo> cpp_stack = OS::get_singleton()->get_cpp_stack_info();

for (int i = 1; i < cpp_stack.size(); ++i) {
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.
_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;
}
}

// 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() {
fflush(stdout);
}
2 changes: 2 additions & 0 deletions core/error/error_macros.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ 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 *p_filter, const String &p_error, bool p_editor_notify = false, ErrorHandlerType p_type = ERR_HANDLER_ERROR);
void _err_flush_stdout();

#ifdef __GNUC__
Expand Down
10 changes: 10 additions & 0 deletions core/os/os.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<StackInfo> get_cpp_stack_info() const { return Vector<StackInfo>(); }
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.
virtual void load_platform_gdextensions() const {}
Expand Down
8 changes: 8 additions & 0 deletions core/templates/list.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
#include "core/os/memory.h"
#include "core/templates/sort_array.h"

#include <initializer_list>

/**
* Generic Templatized Linked List Implementation.
* The implementation differs from the STL one because
Expand Down Expand Up @@ -761,6 +763,12 @@ class List {
}
}

List(std::initializer_list<T> p_init) {
for (const T &i : p_init) {
push_back(i);
}
}

List() {}

~List() {
Expand Down
4 changes: 2 additions & 2 deletions core/variant/variant_utility.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand Down
48 changes: 48 additions & 0 deletions drivers/unix/os_unix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@
#include <uvm/uvm_extern.h>
#endif

#if !defined(__ANDROID__) && !defined(__EMSCRIPTEN__)
#include <execinfo.h>
#endif

#include <cxxabi.h>
#include <dlfcn.h>
#include <errno.h>
#include <poll.h>
Expand Down Expand Up @@ -1006,6 +1011,49 @@ void UnixTerminalLogger::log_error(const char *p_function, const char *p_file, i
}
}

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(dli_sname, nullptr, nullptr, &status);

if (status == 0) {
result.function = demangled;
} else {
result.function = dli_sname;
}
free(demangled);

// Get file info
result.file = dli_fname;
result.offset = static_cast<const char *>(address) - static_cast<const char *>(dli_fbase);
result.load_address = dli_fbase;
result.symbol_address = address;

return result;
}

#if !defined(__ANDROID__) && !defined(__EMSCRIPTEN__)
Vector<OS::StackInfo> OS_Unix::get_cpp_stack_info() const {
constexpr int kMaxBacktraceDepth = 25;
void *backtrace_addrs[kMaxBacktraceDepth];
int trace_size = backtrace(backtrace_addrs, kMaxBacktraceDepth);

Vector<StackInfo> 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.dli_fname, info.dli_fbase, info.dli_sname, info.dli_saddr, backtrace_addrs[i]);
}

return result;
}
#endif

UnixTerminalLogger::~UnixTerminalLogger() {}

OS_Unix::OS_Unix() {
Expand Down
6 changes: 6 additions & 0 deletions drivers/unix/os_unix.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ class OS_Unix : public OS {

virtual void finalize_core() override;

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();

Expand Down Expand Up @@ -101,6 +103,10 @@ 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<StackInfo> get_cpp_stack_info() const override;
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be in the respective OS implementations I'd say, i.e. Mac and Linux, or overridden as stubs in Android and Web, this makes it hard to track

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AThousandShips I think it makes the most sense to keep them in OS_Unix because execinfo.h is theoretically a part of any standards-compliant Unix system. The problem is that Android and Emscripten aren't fully POSIX compliant and don't provide the execinfo.h header. I was surprised to even see that they were children of OS_Unix because neither are really Unix systems. I can remove the #ifdefs from the header here, but I will need to keep them in os_unix.cpp (which already has a ton of these because of the differences in different Unix implementations) because the implementation there depends on being able to import execinfo.h, which is not present in either

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, but this makes it harder to find as it'd be expected to be in those platforms instead of drivers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's weird to me that OS_MacOS in platform inherits from OS_Unix in drivers, which inherits from OS in core, and each provides different portions of the platform class's implementation, but here we are.

};

class UnixTerminalLogger : public StdLogger {
Expand Down
2 changes: 1 addition & 1 deletion editor/editor_log.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 4 additions & 0 deletions platform/macos/os_macos.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 &p_info) override;
#endif

public:
virtual void set_cmdline_platform_args(const List<String> &p_args);
virtual List<String> get_cmdline_platform_args() const override;
Expand Down
13 changes: 13 additions & 0 deletions platform/macos/os_macos.mm
Original file line number Diff line number Diff line change
Expand Up @@ -757,6 +757,19 @@
return PREFERRED_TEXTURE_FORMAT_S3TC_BPTC;
}

#ifdef DEBUG_ENABLED
String OS_MacOS::get_debug_descriptor(const StackInfo &p_info) {
String pipe;
Error error = execute("atos", { "-o", p_info.file, "-l", String::num_uint64(reinterpret_cast<uint64_t>(p_info.load_address), 16), String::num_uint64(reinterpret_cast<uint64_t>(p_info.symbol_address), 16) }, &pipe);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

atos call probably should have -arch arm64 or -arch x86_64 arg added based on the running executable architecture, or in some conditions (like running under Rosetta) it might try reading wrong architecture from the fat binary.


if (error == OK) {
return pipe.strip_edges();
} else {
return String();
}
}
#endif

void OS_MacOS::run() {
if (!main_loop) {
return;
Expand Down
Loading