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

CRASH_COND should interrupt execution #521

Open
Zylann opened this issue Feb 27, 2021 · 4 comments
Open

CRASH_COND should interrupt execution #521

Zylann opened this issue Feb 27, 2021 · 4 comments
Labels
bug This has been identified as a bug

Comments

@Zylann
Copy link
Collaborator

Zylann commented Feb 27, 2021

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

@Calinou Calinou added the bug This has been identified as a bug label Feb 27, 2021
@Zylann Zylann mentioned this issue May 9, 2021
@kb173
Copy link

kb173 commented Nov 3, 2022

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
Copy link
Collaborator Author

Zylann commented Nov 3, 2022

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
Copy link

kb173 commented Nov 3, 2022

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
Copy link
Collaborator Author

Zylann commented Nov 4, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug
Projects
None yet
Development

No branches or pull requests

3 participants