Skip initializing the C# runtime when generating glue bindings #76659
Conversation
There was a problem hiding this comment.
Also this causes these 2 errors on exit after the glue generation:
ERROR: Pages in use exist at exit in PagedAllocator: union Variant::Pools::BucketMedium
at: (C:\dir\godot\core/templates/paged_allocator.h:140)
ERROR: Pages in use exist at exit in PagedAllocator: union Variant::Pools::BucketSmall
at: (C:\dir\godot\core/templates/paged_allocator.h:140)
Not sure what exactly causes them
modules/mono/mono_gd/gd_mono.cpp
Outdated
| List<String> cmdline_args = OS::get_singleton()->get_cmdline_args(); | ||
| BindingsGenerator::process_cmdline(cmdline_args); |
There was a problem hiding this comment.
I would maybe move this to an earlier point in main, instead of here as it doesn't feel like it belongs here and more like a trap waiting to happen when additional command line changes are done
There was a problem hiding this comment.
Are you thinking right before the ScriptServer::init_languages(); call in main.cpp, a few lines above the generate_bindings call?
I can parse the command line arguments at that point, and just have the "check and early exit" in CSharpLanguage::init(), before the gdmono allocation, so we wouldn't even create gdmono at all in this codepath. That would keep most of these calls relatively close to each other.
There was a problem hiding this comment.
TBH I didn't really think about where to best put this, just that this isn't it.
above init is probably fine.
I tried just moving the command line handling above the init call and if I add a theme db initialize to the bindings generator this appears to work just fine as well (the comment stating that the script server has to be initialized appears to be wrong)
diff --git a/main/main.cpp b/main/main.cpp
index 5e0187cc7fd..758711f1ebd 100644
--- a/main/main.cpp
+++ b/main/main.cpp
@@ -2406,20 +2406,19 @@ Error Main::setup2() {
initialize_navigation_server();
register_server_singletons();
- // This loads global classes, so it must happen before custom loaders and savers are registered
- ScriptServer::init_languages();
-
- theme_db->initialize_theme();
- audio_server->load_default_bus_layout();
#if defined(MODULE_MONO_ENABLED) && defined(TOOLS_ENABLED)
// Hacky to have it here, but we don't have good facility yet to let modules
- // register command line options to call at the right time. This needs to happen
- // after init'ing the ScriptServer, but also after init'ing the ThemeDB,
- // for the C# docs generation in the bindings.
+ // register command line options to call at the right time.
List<String> cmdline_args = OS::get_singleton()->get_cmdline_args();
BindingsGenerator::handle_cmdline_args(cmdline_args);
#endif
+
+ // This loads global classes, so it must happen before custom loaders and savers are registered
+ ScriptServer::init_languages();
+ theme_db->initialize_theme();
+
+ audio_server->load_default_bus_layout();
if (use_debug_profiler && EngineDebugger::is_active()) {
// Start the "scripts" profiler, used in local debugging.
diff --git a/modules/mono/editor/bindings_generator.cpp b/modules/mono/editor/bindings_generator.cpp
index 8504fb2ac6a..2d77e5d2f76 100644
--- a/modules/mono/editor/bindings_generator.cpp
+++ b/modules/mono/editor/bindings_generator.cpp
@@ -39,6 +39,7 @@
#include "core/io/file_access.h"
#include "core/os/os.h"
#include "main/main.h"
+#include "scene/theme/theme_db.h"
#include "../godotsharp_defs.h"
#include "../utils/naming_utils.h"
@@ -3967,6 +3968,7 @@ void BindingsGenerator::handle_cmdline_args(const List<String> &p_cmdline_args)
}
if (glue_dir_path.length()) {
+ ThemeDB::get_singleton()->initialize_theme();
handle_cmdline_options(glue_dir_path);
// Exit once done
cleanup_and_exit_godot();The only diff that is causes in the generated code is the value of the NativeCalls::godot_api_hash of godotsharp but that files is actually unused anyways to this doesn't change anything.
There was a problem hiding this comment.
Indeed, the reason this was moved to main is to prevent a crash after some theme related changes were introduced: #74565. This PR effectively reverses that patch, so I would expect it to re-introduce that issue.
I can't review @RedworkDE's patch as I'm that is not my area, and it's easy to introduce a regression. I think it would be better to just skip C# initialization if the glue generation command line argument is detected:
void CSharpLanguage::init() {
+#ifdef TOOLS_ENABLED
+ if (OS::get_singleton()->get_cmdline_args().find("--generate-mono-glue")) {
+ return;
+ }
+#endifThere was a problem hiding this comment.
I moved things around a bit to try and make it cleaner without changing the existing initialization order (and hopefully not introduce regressions), see if it looks better. I would rather not hardcode the flag if I can help it, it makes the code more fragile - hopefully this latest refactor feels better.
There was a problem hiding this comment.
I feel like neikeq's suggestion would be enough and simpler than all the changes this PR is currently making. I don't think there's an issue with hardcoding the flag, we can keep it in a constant/static field if you prefer like in BindingsGenerator::generate_all_glue_option.
I'm not sure either, but I'm seeing them as well. I'll chase it down. |
Turns out it's the allocation of the BindingsGenerator instance, it needs to go completely out of scope to get cleaned up properly before exiting. I need to add back a static trampoline that got dropped with the refactor. |
shana
force-pushed
the
shana/75152-fix-crash-when-initializing-glue-generation
branch
from
a25dc29 to
b04ee35
Compare
That's more or less what I had originally, but it was pointed out that the command line flag loop would have to run twice - once to see if the flag is set, and another to extract the path to use. That's why I moved the parsing of the command line out, to avoid looping twice, and then was told that main.cpp would be a better place for it. I would personally prefer using the existing code that checks for the command line option instead of hardcoding it (just because that usually leads to problems later on, in my experience), but where that check is, I don't have a strong opinion. I'm happy to move the command line parsing and flag check bits back into csharp_script instead of in main.cpp, if that makes more sense to long term maintenance. I really have no preference, I was just following the suggestions. I would just really like to have this crash fixed. |
The bindings generator doesn't require the C# runtime in order to generate the glue, and when it the glue generation runs, it exits immediately afterwards, so we can skip this initialization when the `--generate-mono-glue` flag is passed in. Fixes issue 75152
shana
force-pushed
the
shana/75152-fix-crash-when-initializing-glue-generation
branch
from
aaac203 to
e56fdc8
Compare
|
Ok, I went back to the original one liner fix to skip the runtime initialization if we're generating glue, but moved to csharp_script, without any other changes. |
|
Thanks, and congrats on your first merged Godot PR! |
Fixes #75152
This splits out the glue generator initialization fix from #76542 into its own PR.
The bindings generator doesn't require the C# runtime in order to generate the glue, and when it the glue generation runs, it exits immediately afterwards, so we can skip this initialization when the
--generate-mono-glueflag is passed in.In some platforms, like on macos M1 arm, trying to run the godot+mono binary on an incomplete installation crashes during the hostfx initialization, so #75152 is a bit more serious than just an annoying popup.