Skip to content

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Oct 8, 2025

We've had at least one report of someone crashing here after doing an incremental build after #110965 was merged:

// Version check
if (!VerifyJoltVersionIDInternal(inVersionID))
{
Trace("Version mismatch, make sure you compile the client code with the same Jolt version and compiler definitions!");
uint64 mismatch = JPH_VERSION_ID ^ inVersionID;
auto check_bit = [mismatch](int inBit, const char *inLabel) { if (mismatch & (uint64(1) << (inBit + 23))) Trace("Mismatching define %s.", inLabel); };
check_bit(1, "JPH_DOUBLE_PRECISION");
check_bit(2, "JPH_CROSS_PLATFORM_DETERMINISTIC");
check_bit(3, "JPH_FLOATING_POINT_EXCEPTIONS_ENABLED");
check_bit(4, "JPH_PROFILE_ENABLED");
check_bit(5, "JPH_EXTERNAL_PROFILE");
check_bit(6, "JPH_DEBUG_RENDERER");
check_bit(7, "JPH_DISABLE_TEMP_ALLOCATOR");
check_bit(8, "JPH_DISABLE_CUSTOM_ALLOCATOR");
check_bit(9, "JPH_OBJECT_LAYER_BITS");
check_bit(10, "JPH_ENABLE_ASSERTS");
check_bit(11, "JPH_OBJECT_STREAM");
std::abort();
}

The reason for this isn't confirmed, but is (as discussed in #110965) potentially as follows:

  1. #104893 broke the SCons dependencies between the source and header files in thirdparty/jolt_physics (and possibly other third-party vendorings as well).
  2. This was then reverted in #111331, but since the revert relies on changing CPPPATH, it likely clashes with us relying on fast_unsafe=yes by default when dev_build=yes, which enables the implicit_cache SCons option, which in its documentation states that it causes SCons to "ignore any changes that may have been made to search paths (like $CPPPATH or $LIBPATH)".
  3. #110965 was then merged, and while Jolt/RegisterTypes.cpp didn't have any changes to it, it should still have been recompiled, since its header dependencies did change, but these dependencies were likely not restored because of fast_unsafe=yes.
  4. Jolt then (rightfully) std::abort in its version check because the compilation environment of that particular file doesn't line up with the caller's compilation environment, leading to this crash.

While the fix here is as simple as doing a clean rebuild, this will likely keep happening to people in the future as they go to bisect with dev_build=yes, since incremental builds from any commit between 1f56d96 and 9052d31 will be subject to this.

As such, this PR cherrypicks jrouwe/JoltPhysics@9e48d59 in order to have some kind of change to Jolt/RegisterTypes.cpp, which should at least prevent the crash from happening going forward.

Note that the crash is arguably a red herring here, the real problem is ending up with incomplete incremental builds because of what is likely to be fast_unsafe=yes, potentially across several third-party vendorings.

@mihe mihe added this to the 4.6 milestone Oct 8, 2025
@mihe mihe requested a review from a team as a code owner October 8, 2025 12:17
@mihe mihe mentioned this pull request Oct 8, 2025
@bruvzg
Copy link
Member

bruvzg commented Oct 8, 2025

We've had at least one report of someone crashing here after doing an incremental build

I had the same issue this morning as well, clean build fixed it (editor build on macOS, for the reference).

@Repiteo Repiteo merged commit 4ad11b6 into godotengine:master Oct 8, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 8, 2025

Thanks!

@mihe mihe deleted the jolt/version-crash-fix branch October 8, 2025 13:15
akien-mga added a commit to akien-mga/godot that referenced this pull request Oct 8, 2025
We experienced first hand why it's called unsafe, and so we should leave it
as an explicit choice for contributors, informing themselves of the caveats.

See godotengine#111408.
@jrouwe
Copy link
Contributor

jrouwe commented Oct 8, 2025

Note that instead of doing a std::abort, the application will now run with a random mix of recompiled and non-recompiled files. If e.g. a single class changed memory layout then this will lead to really hard to debug memory corruption crashes. Or it could lead to an algorithm using partially old and partially new code with unpredictable results.

@mihe
Copy link
Contributor Author

mihe commented Oct 8, 2025

Note that instead of doing a std::abort, the application will now run with a random mix of recompiled and non-recompiled files.

#111411 will remove the implicit fast_unsafe for dev_build, so should hopefully resolve that problem for the most part. Anyone who still ends up explicitly passing fast_unsafe=yes (to get the shorter incremental build times) should ideally do a clean build I guess, but they will most likely end up running a build or two without fast_unsafe first, before they realize the incremental builds are now slower, which should fix the header dependencies.

However, I guess it's theoretically possible for someone to run into trouble if they bisect with fast_unsafe and do a clean build on the commits where CPPEXTPATH was still a thing, but short of rewriting Git history I'm not sure how you'd fix that. Maybe by adding some arbitrary compile definition to every thirdparty/*/SCsub or something, just to force a rebuild of everything that used CPPEXTPATH?

In any case, they have at least made the explicit choice of entering the "danger zone" by enabling fast_unsafe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants