libretro / gpsp

gpSP for libretro.
GNU General Public License v2.0
52 stars 52 forks source link

Freeze with Pokemon Gaia Version #180

Closed schmurtzm closed 1 year ago

schmurtzm commented 1 year ago

Pokemon Gaia Version (hack of FireRed) freeze when we open the letter at the beginning of the game. Image seems freezed, sound still OK, no controls.

To reproduce : open the letter

https://user-images.githubusercontent.com/7110113/211164545-dc86ae33-554b-4b9c-a31a-70694e2a8c2c.mp4

Plateform : armv7 +neon , same behavior on x86 Withtout dynarec the behavior is slightly different but it still freeze. Tested with last gPSP commit : 90170e33894ac8e3598a5660da62a18884d795f4 5 jan 2023

andymcca commented 1 year ago

Hi again! Just quick - especially when a dynarec is involved, please also specify which platform you're testing on (i.e. x86/ARM/MIPS) - although I note you say it happens in Interpreter mode as well.

schmurtzm commented 1 year ago

At the beginning of each video my settings are exposed ;) Description updated ;)

andymcca commented 1 year ago

Hi - for the Issue #190 that I've been troubleshooting, I've built gpsp from commit cc1a074 which is before the ROM buffer rewrite commit. I suspect it's this commit that breaks 32MB ROMS on low memory platforms. I previously tried Pokemon Gaia using the latest commit and it doesn't even load on my Leapster Explorer (64MB RAM).

Building from this commit cc1a074 and also adjusting the old ROM buffer code to start malloc attempts very low (4MB) means that 32MB ROMs like Mother 3 and Pokemon Gaia now work.

Specifically in this case, it also means that I can go past opening the letter!

@davidgfnet just fyi - I think there's a bug in the ROM buffer allocation code. Based on my anecdotal testing it doesn't seem to work correctly if the ROM being loaded (mainly 32MB ROMs) is bigger than available RAM (even if ROM_BUFFER_SIZE is set on compilation). As an aside - what is the benefit of doing mallocs on more than 1MB of a ROM? I've reduced the buffer down to 1MB (albeit on the old code), but not yet noticed any discernible performance hit, and it's actually helping to run these big ROM carts on low mem (32MB RAM) platforms??

davidgfnet commented 1 year ago

Hey there. I also suspect this could be an issue fixed lately. To be able to help with this I would need: a copy of the ROM (the exact one you are using, you can email it to me), a savestate/savefile and some instructions to reproduce. I can't play for an hour to reproduce a bug, so a savestate/savefile is a great way to send bugs. Thanks!

andymcca commented 1 year ago

Hi David, if I can reproduce the error on build 9017e03 as per the first message using the copy of the ROM I have, then I'll send you over the ROM and a Savestate (assuming the most recent build doesn't fix it, will test that first).

andymcca commented 1 year ago

@davidgfnet I've emailed you the ROM and a Savestate 👍

andymcca commented 1 year ago

Hey @schmurtzm, after investigating with @davidgfnet it appears that the problem is actually with a bug in this ROM hack rather than gpsp! Commit 8207775 contains fixes to bring gpsp more in line with real hardware, which then exposes a bug in this ROM hack. So this won't be 'fixed' here, it needs the developer of the ROM hack to fix it instead.

schmurtzm commented 1 year ago

OK thank you for all these investigation (that I follow with interest !). I will try to contact the author, which information can I give him about this bug ?

andymcca commented 1 year ago

According to David it is a null pointer dereference bug which then causes the crash - but it seemingly manages to be ok only via inaccurate emulation.

davidgfnet commented 1 year ago

Hey there! I just confirmed that the bug exists in the game using mgba's debugger. Since this ROM is actually a fork of Pokemon FireRed I was able to leverage the available reversed sources (at https://github.com/pret/pokefirered) to discover that the bug is related to the FreeSpriteTilesIfNotUsingSheet function being called using with "sprite->images" being NULL. In this case, the null ptr dereference causes some funny behaviour in GBA, since it doesnt have an MMU. It reads some fun value and miraculosuly "works" and the game can continue. However in gpsp the value doesn't match what a real device might return so it gets into a very long loop (that likely takes a few hours to get out so it appears as "frozen"). The bug is similar to the bug described here: https://mgba.io/2020/01/25/infinite-loop-holy-grail/ (and also a similar bug in Zelda Minish Cup where Link cant roll, see https://github.com/libretro/gpsp/issues/141) After that mgba improved their open-bus implementation "fixing" these games. Not sure if this game runs on real hardware. I can give it a shot on my NDS. Is "sphericalice" the developer? Maybe we can report it and they can fix it :) The description for the open bus behaviour lives here: https://www.ngemu.com/threads/gba-open-bus.170809/ I think it would be a good idea to add some test for this. Not long ago I created some test ROMs for GBA (https://github.com/davidgfnet/test-rom-suite/tree/master/gba) with the goal of testing all these weird corner cases (and catch regressions!) that gpsp is vulnerable to. Anyway, long answer :D I will improve the open bus behaviour since so many games seem to have bugs that depend on it.

davidgfnet commented 1 year ago

You might wanna check out the latest HEAD. Emulating these prohibited reads (more) correctly seems to work for this ROM hack.

andymcca commented 1 year ago

Excellent! Will check it out via new build later on.

andymcca commented 1 year ago

Yep confirmed, all working again as of the latest commit! Reckon this one can be closed now 👍