Fix Epona spawn with Shuffle Individual Ocarina Notes - Again

[GSKirox]

The value of EPONAS_SONG_NOTES that was read by the C code didn't match the value sent in Patches.py
I don't really get why, since how it was done before looks correct to me, and both ShadowShine and me tested the fix pushed in #2066 and it worked back then. 😓
However, i noticed i used a halfword for absolutely no good reason since the song information only needs 5 bits, so a byte is plenty enough.
So I switched to sending a single byte/receiving an int8, and that does seem to fix the value. After that, the rest of the initial fix does the work correctly.

Fixes #2065

[MajikVroom]

I don't really get why, since how it was done before looks correct to me

As mentioned in the Issue, I think the problem is alignment-related.
What I directly observed when debugging:

• For reading EPONAS_SONG_NOTES, the compiler generated an lh from fixed address 0x80401323, which is where EPONAS_SONG_NOTES should be per the symbols.
• A memory inspector confirmed that the 16 bits at address 0x80401323 contained the correct value: 0x1A
• The result of the lh read was just 0x1

I can't find a definitive source for the memory alignment requirements of the N64, but I've seen a handful of sources on MIPS in general mentioning that data reads must be aligned to the data size. So a halfword (2-byte) read needs to read from addresses that are a multiple of 2, which 0x80401323 isn't.
I'm not sure if the consequences of a misaligned read on N64 are well-defined; for all I know the behavior could be dependent on the specific emulator's implementation. But at the very least on Bizhawk it seems to just lead to bad data.

So I think you could put in a ".align 2" before the .halfword in config.asm, and it would solve the problem. Or as you said, switching to a single byte works because it gets rid of the alignment requirement entirely.

both ShadowShine and me tested the fix pushed in #2066 and it worked back then. 😓

Yep, that's consistent with the alignment theory. If EPONAS_SONG_NOTES happens to be placed in an address that's appropriately aligned, the code will behave correctly. It was probably working fine when you tested it, it was just subtly unstable and broke when a subsequent change shifted the layout.

Edit: apparently ".align n" produces alignment on 2^n bytes. So specifying 2-byte alignment would actually be ".align 1", rather than ".align 2".
Edit 2: the above edit is incorrect; that's how a different MIPS assembler behaves, but for armips ".align n" aligns on n bytes (and n itself must be a power of 2).

[GSKirox]

I see, it makes a lot more sense now !
Thanks for your analysis, i'm super shaky on these memory subtleties, so i feel a lot more confident about this fix after reading that.

[flagrama]

@MajikVroom

I'm not sure if the consequences of a misaligned read on N64 are well-defined; for all I know the behavior could be dependent on the specific emulator's implementation. But at the very least on Bizhawk it seems to just lead to bad data.

Nothing in an emulator can be trusted to follow the N64s behaviour "well-defined" or not unless the emulator has been tested against the results that the real system itself does. I don't think mupen64plus-core (used in bizhawk and retroarch) nor Project 64 3.0.1 and earlier are heavily tested against something like https://github.com/lemmy-64/n64-systemtest/ because tests like these didn't exist in the past because running software on actual hardware was much more difficult than it is now.

Unfortunately LH has not been tested in this suite yet (https://github.com/lemmy-64/n64-systemtest/blob/main/src/tests/address_error_exception/mod.rs) but based on how LW is supposed to generate an exception, I would expect that actually accurate emulators and hardware should crash on this unaligned read. I expect ares-emu and Project64 4.0-dev with everything in interpreter mode should be accurate to those tests, however without the tests actually testing LH it is unlikely there is currently an "accurate emulator" to trust unless there is another test suite I'm not aware of.

I could check to see if it crashes on console if given a patch file and instructions on how to trigger it to see which is the actual expected behaviour. Obviously not necessary for the fix though.

[MajikVroom]

@flagrama

Yeah, I guess it makes sense that perfectly emulating the behavior of N64 hardware under circumstances that aren't normally supposed to occur wouldn't be a priority for most emulators/emulation modes; "make sure it crashes exactly like it would on a real N64" is a non-critical requirement.

I could check to see if it crashes on console if given a patch file and instructions on how to trigger it to see which is the actual expected behaviour. Obviously not necessary for the fix though.

Agreed that it's not necessary, but if you want to try I believe the following conditions are sufficient:

• "Shuffle Individual Ocarina Notes" is enabled
• You know Epona's Song (I just started with it)
• Epona is rescued (I used Skip Epona Race)

I think that should be enough for can_spawn_epona to be called and reach the comparison against EPONAS_SONG_NOTES. I was triggering the code by just by entering Hyrule Field as an adult (from Market).

[flagrama]

8.0 crashes on hardware as expected with the given settings and entering hyrule field as adult due to the unaligned read. I definitely support #2141 being added to prevent that in the future if only because nobody will remember/be able to test on hardware and it'll get missed and cause problems for a release again.