coop-deluxe / sm64coopdx

An official continuation of https://github.com/djoslin0/sm64ex-coop on sm64coopdx for the enhancements and progress it already has.
https://sm64coopdx.com
316 stars 55 forks source link

Crash on macOS (arm64) when touching a flame #275

Closed rweichler closed 6 days ago

rweichler commented 4 weeks ago

v1.0.1 off the releases page. macOS arm64.

There was also a piranha plant nearby. This was on the star road romhack

Crashed as soon as the flame touched me, and the piranha plant was awake. I can't consistently repro.

Crash log: https://gist.github.com/rweichler/70793f71cc758bafd28a279391b1b4a6

image
EmeraldLoc commented 4 weeks ago

This is a lot easier to get in whomps. I’ve encountered this error before, unsure on how to reproduce. It’s unrelated to the flame, but has to do with the piranha plant

robertkirkman commented 3 weeks ago

Did you guys already try the solution invented in 2021 by VDavid003, the creator of the first port of singleplayer sm64ex to Android ARM?

--- a/src/game/behaviors/piranha_bubbles.inc.c
+++ b/src/game/behaviors/piranha_bubbles.inc.c
@@ -32,6 +32,9 @@ void bhv_piranha_plant_bubble_loop(void) {
     f32 scale = 0;
     s32 i;
     s32 frame = parent->header.gfx.animInfo.animFrame;
+    if (frame < 0) {
+        frame = 0;
+    }
     // TODO: rename lastFrame if it is inaccurate
     if (parent->header.gfx.animInfo.curAnim == NULL) { return; }
     s32 lastFrame = parent->header.gfx.animInfo.curAnim->loopEnd - 2;

It prevents the crash that also happens for me after compiling and running an unreleased fork of this repo (port of sm64coopdx 1.0.1 to Arch Linux ARM etc. - "sm64ex-coop" branding) with the clang/clang++ compiler on my Arch Linux ARM true aarch64 emulator when interacting with piranha plants.

sm64ex-coop-371-arch-linux-arm-piranha-plant-crash-fixes-comparison.webm

Many users of one of my repos have used it for over a year without ever encountering any piranha plant crash.

When I conduct my own analysis of this crash today, I can see that the root cause of the problem is the same that you encountered before and read about in a different thread @EmeraldLoc , the undefined behavior of passing a negative floating point number to the coss() macro which casts it to unsigned integer.

https://github.com/coop-deluxe/sm64coopdx/blob/e4bdb1fb17c6096ccee16e820a21b1d60212bf57/src/engine/math_util.h#L27-L28

Knowing that, this alternative solution also becomes possible.

--- a/src/game/behaviors/piranha_bubbles.inc.c
+++ b/src/game/behaviors/piranha_bubbles.inc.c
@@ -71,7 +71,7 @@ void bhv_piranha_plant_bubble_loop(void) {
                     // Note that the bubble always starts this loop at its largest.
                     if (frame < doneShrinkingFrame) {
                         // Shrink from 5.0f to 1.0f.
-                        scale = coss(frame / doneShrinkingFrame * 0x4000) * 4.0f + 1.0;
+                        scale = coss((s32)(frame / doneShrinkingFrame * 0x4000)) * 4.0f + 1.0;
                     } else if (frame > beginGrowingFrame) {
                         // Grow from 1.0f to 5.0f.
                         scale = sins((

That new solution also works for me to avoid the crash when interacting with the piranha plant, but I'm not completely sure whether it's the best solution or not. I still don't know how widespread bugs following this same pattern of problematic use of the sins() and coss() macros might be. If they are any more common beyond these two cases (this issue and the other issue that was deleted), the macros themselves might need to be replaced with something more robust.

EmeraldLoc commented 3 weeks ago

Won’t be at my computer today, will check this out when I can, thanks!

robertkirkman commented 3 weeks ago

Oh and since I wasn't really clear about this part of the explanation, in case this helps you understand it, the cause of the argument to coss() ending up being floating point to begin with is because doneShrinkingFrame is an f32 and in C if you multiply and divide floating point numbers by other numbers, the expression evaluates into another floating point number, so in general there are a few different angles to the problem and there are multiple possible solutions.

https://github.com/coop-deluxe/sm64coopdx/blob/e4bdb1fb17c6096ccee16e820a21b1d60212bf57/src/game/behaviors/piranha_bubbles.inc.c#L39