godotengine / godot-cpp

C++ bindings for the Godot script API
MIT License
1.67k stars 503 forks source link

CRASH_COND should interrupt execution #521

Open Zylann opened 3 years ago

Zylann commented 3 years ago

The CRASH_COND macro is defined like this:

#define CRASH_COND(cond)                     \
    do {                                     \
        if (unlikely(cond)) {                \
            FATAL_PRINT(ERR_MSG_COND(cond)); \
            return;                          \
        }                                    \
    } while (0)

But this does not behave like an assertion like in Godot, and resumes execution. It should instead stop execution without even expecting to put a return value, like so: https://github.com/godotengine/godot/blob/7b685a1558aaa7e014d358c2db8be83aef5d8b8f/core/error/error_macros.h#L450

kb173 commented 1 year ago

Seems like the CRASH_COND macro is now defined as in the link @Zylann posted (calls GENERATE_TRAP()), but it still doesn't quite work as I would expect: it crashes the application with a signal 4 and produces a backtrace after printing the error to the console:

ERROR: Error Text
   at: load_from_file (src/geodata.cpp:358)

================================================================
handle_crash: Program crashed with signal 4
Engine version: Godot Engine v4.0.beta3.official (01ae26d31befb6679ecd92cd3c73aa5a76162e95)
Dumping the backtrace. Please include this when reporting the bug on: https://github.com/godotengine/godot/issues
[1] /usr/lib/libc.so.6(+0x38a00) [0x7f8c4fc1da00] (??:0)
...

But because it simply crashes with a signal 4, this does not actually show any information in the editor (no crash UI, no error in the editor's debugger, etc.) - just --- Debugging process stopped ---. That kind of defeats the purpose of this macro for me since that's not much better than crashing with a segfault due to invalid state later on. Am I missing something, or is there a bug in this current implementation?

Zylann commented 1 year ago

CRASH_COND is unfortunately supposed to work like that, it does a crash. The same thing happens with regular crashes in Godot, the application exits right away. If we want somehow to report the printed data to the editor (if ran from editor) then even the implementation in core Godot would have to be changed I guess.

kb173 commented 1 year ago

Hmm that's a shame. Do you know if there's at least a way to flush the log output before crashing in order to make sure that the error reaches the editor's debugger, rather than only getting output into the command line?

Zylann commented 1 year ago

Whatever happens, the application must quit at the CRASH_COND. I don't know much about what can be done, I wonder if the editor can intercept the logs you'd get in a standalone execution? (since you said you get a backtrace). But at this point it needs to be reported in Godot's repository maybe? Also the implementation still differs from core actually, flushing log output is missing https://github.com/godotengine/godot/blob/c98d6142d0c8cf4ac284a595ad1156a4b74736ad/core/error/error_macros.h#L551

Another note: CRASH_COND should normally not happen in something you ship, and is actually useful when using a C++ debugger (which you should be using if you do C++ dev and have to fix a crash), because the debugger will stop there and show you the crash site with all the locals and stack trace. Expecting a call stack after-the-fact by running without a debugger is useful for users to report it, but is apparently not necessarily granted, and debugging all crashes this way isn't efficient IMO.