goddard: read DynList int operands via .ptr to fix 64-bit big-endian crash (#426) - #605
Open
Scottcjn wants to merge 1 commit into
Open
goddard: read DynList int operands via .ptr to fix 64-bit big-endian crash (#426)#605Scottcjn wants to merge 1 commit into
Scottcjn wants to merge 1 commit into
Conversation
…crash
DynList integer operands are stored through the union's pointer member --
the dynlist_macros.h commands cast their int argument to void *, e.g.
SetColourNum(x) -> { 13, {0}, {(void *)(x)}, ... }. But Dyn1AsInt /
Dyn2AsInt read them back through the narrower .word member. On 64-bit
big-endian the value sits in the high 4 bytes of the 8-byte pointer slot,
so .word reads 0 -- a garbage object type that aborts the Goddard intro
and hangs the game at boot (issue sm64pc#426).
Read the operand back through .ptr (matching the sibling Dyn*AsID
accessors) and narrow to s32. This is correct on every word-size x
endianness combination and changes no struct layout, so 32-bit
big-endian (e.g. 32-bit PowerPC) is unaffected -- unlike widening word
to s64, which would fix 64-bit BE but regress 32-bit BE.
Verified by cross-building the full game for ppc64 big-endian (compiles
clean) and by a standalone reproduction using the repo's own dynlist
macros, which reads back the correct value instead of 0 on 64-bit BE
with this change (and shows the s64 alternative reading 0 on 32-bit BE).
Scottcjn
force-pushed
the
fix/dynlist-bigendian-int-operands
branch
from
June 23, 2026 14:36
431c2be to
050afb8
Compare
Author
|
Friendly nudge. This fixes a real 64-bit read of Goddard's DynList operands via .ptr. Small and self-contained. Anything needed from my side to get a review? |
evgenijpofalityj-pixel
approved these changes
Jul 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #426 — on 64-bit big-endian builds (e.g. ppc64) the game crashes/hangs at the Goddard ("Mario head") intro and never reaches the title screen.
Root cause
DynListinteger operands are written through the union's pointer member — the construction macros indynlist_macros.hcast the int argument tovoid *:…but
Dyn1AsInt/Dyn2AsIntread them back through the narrower.wordmember:On little-endian the value's low bytes line up with
.word, so it works. On 64-bit big-endian the value sits in the high 4 bytes of the 8-byte pointer slot, so.wordreads0— a garbage object type that makesd_makeobjfail and the Goddard intro abort.Fix
Read the operand back the same way it was written — through
.ptr— then narrow tos32. This mirrors the siblingDyn*AsIDaccessors right next to it, which already read.ptr:No struct layout change.
Why not just widen
wordtos64?That's the tempting one-liner, but it regresses 32-bit big-endian (e.g. 32-bit PowerPC), because there the pointer is 4 bytes and the widened read picks up the wrong half. Worked out across word size × endianness:
DynUnion.words32(current)s64.ptrreader (this PR)Reading via
.ptris the only option correct everywhere, and it leavesDynList's layout untouched (so LE and 32-bit builds are entirely unaffected).Testing
dynlist_macros.hconfirmsSetColourNum(5)reads back5on both endians with this change, vs0on 64-bit BE before it; and that thes64alternative reads0on 32-bit BE.Original diagnosis by @KungFuJesus in #426 — thanks!
Note: building on PowerPC also needs a small
Makefiletweak (LDFLAGShardcodes-march=$(TARGET_ARCH), which ppc GCC rejects — it wants-mcpu). I kept that out of this PR to stay focused on the #426 crash; happy to send it separately if useful.