retro_reset may change memory pointer without calling RETRO_ENVIRONMENT_SET_MEMORY_MAPS

[Jamiras]

RetroAchievements uses the memory exposed by RETRO_ENVIRONMENT_SET_MEMORY_MAPS to determine when achievements should trigger. We've noticed in SameBoy (and SameDuck) that resetting sometimes causes the exposed memory to change, causing achievements to trigger at the wrong time or not at all.

I've added a bunch of logging to RAlibretro (our libretro frontend), and captured the following (abridged):

132928.768 [INFO ] [COR] RETRO_ENVIRONMENT_SET_MEMORY_MAPS
132928.768 [INFO ] [COR]     3 M1A1bc 000001D643686750 00000000 0000C000 00000000 00000000 00001000 
132929.378 [INFO ] [MEM] desc[3]: $00c000 (1000): 000001D643686750: 
132929.378 [INFO ] [MEM] Registered 0x1000 bytes of SYSTEM RAM at $00C000 (descriptor 3, offset 0x000000)

132946.437 [INFO ] [APP] before reset: get_memory_data=000001D643686750
132946.440 [INFO ] [APP] after reset: get_memory_data=000001D643684740
132946.453 [INFO ] [MEM] desc[3]: $00c000 (1000): 000001D643686750: 
132946.453 [INFO ] [MEM] Registered 0x1000 bytes of SYSTEM RAM at $00C000 (descriptor 3, offset 0x000000)
132946.453 [INFO ] [MEM] no change detected, using existing memory map

You can see the pointer returned by retro_get_memory_data changes from 000001D643686750 to 000001D643684740, but the mapped memory is still pointing at 000001D643686750. This causes our logic to use the old pointer and read memory that isn't what we're expecting.

I should note that this doesn't happen every time the reset occurs. In fact, I let the emulator self-reset every 15 seconds for 8+ hours without encountering the problem. With some insight from other users, I've found that closing the game (unloading the core) and reopening it (reloading the core) several times first makes it more likely that the reset will cause the pointer to change.

I've attached the log associated with the captured lines above. If you scan through the entire thing you can see I load the core, reset 10 times, reload the game, reset 10 times, reload the game, reset 10 times, reload the game, and the experience the problem on the first reset after that.

log.txt

retro_reset calls init_for_current_model: https://github.com/LIJI32/SameBoy/blob/a7f7530eed32dff53dabf5f1bb1010123a6cb04f/libretro/libretro.c#L1146-L1156 which calls either GB_switch_model_and_reset (which does a realloc) or GB_Init (which does a malloc). https://github.com/LIJI32/SameBoy/blob/a7f7530eed32dff53dabf5f1bb1010123a6cb04f/libretro/libretro.c#L541-L546 In either case, the pointer could change (and logging seems to support that - I'm actually surprised how often it doesn't change).

I suspect that calling retro_set_memory_maps at the end of retro_reset (or maybe in init_for_current_model) would fix the issue.

Or maybe something can be done to avoid the reallocation when just resetting the game.

[LIJI32]

realloc shouldn't return a different value if the size hasn't changed, but other than this, this is indeed a bug which obviously affects this long standing issue. Thanks for investigating it and figuring it out!

[LIJI32]

Let me know if that last commit solved this issue.

[Jamiras]

Let me know if that last commit solved this issue.

It did not. I'm still seeing the same behavior:

082140.247 [INFO ] [APP] before reset: get_memory_data=000002A76AEC5280
082140.250 [INFO ] [APP] after reset: get_memory_data=000002A76AEC92A0
082140.263 [INFO ] [MEM] desc[3]: $00c000 (1000): 000002A76AEC5280: 

The last call to RETRO_ENVIRONMENT_SET_MEMORY_MAPS was when the game was loaded:

082122.965 [INFO ] [COR] retro_memory_map
082122.965 [INFO ] [COR] Content "E:\Games\RetroAchievements\ROMs\GameBoy\DuckTales (USA).zip#DuckTales (USA).gb" loaded

log.txt

Tested with the 2022-04-11 build of sameboy from here: https://buildbot.libretro.com/nightly/windows/x86_64/latest/ FWIW, the core is reporting itself as version "0.14.7 b154b7d". I'm not sure how that's generated, but it's the same version reported by the core that I originally reproduced on (which was from about two weeks ago).

[LIJI32]

Nightly builds are coming from libretro's fork, which is not always up to date. That commit is from December 2021 so it's likely just the same build over again. You're probably going to have to build the core yourself, as I don't have any environment to build the libretro core outside of macOS.

[LIJI32]

(I'm leaving this still open until verified)

[Jamiras]

Verified.

144007.369 [INFO ] [APP] before reset: get_memory_data=0000026A1AD5AB80
144007.426 [INFO ] [COR] retro_memory_map
144007.429 [INFO ] [APP] after reset: get_memory_data=0000026A1AD5CB90
144007.435 [INFO ] [MEM] desc[3]: $00c000 (1000): 0000026A1AD5CB90: 

I let it run for two hours resetting every 15 seconds and reloading every three minutes. It did not trigger my "unexpected values in memory" error once.

As an interesting side note, once it started this behavior, every reset seemed to change the pointer, though sometimes it would oscillate back and forth between a couple values.

How do we get this to the libretro fork? That's where the players are going to get their core. I believe the same change needs to make it into SameDuck too.

[Jamiras]

BTW, I couldn't get the Makefile to run the rgbds tools in mingw. I ended up generating the bin/BootROMs on linux and copying them over. Also, there were a bunch of errors using the 0.6.0 rgbds release candidate.

[WandKnight]

I can also verify that this appears to have fixed the issue. I was able to consistently mess up the memory beforehand, but now it's working as it should no matter what I do.

[LIJI32]

Good to know it's actually fixed! As for the other points:

1. The binary releases for the libretro core are handled by the RetroArch folks (I don't have the environments and knowledge to build the core for all these platforms), so it's mostly out of my control, but generally end users should only be using tagged commits/stable releases of SameBoy (standalone or otherwise), as untagged versions are often broken with regressions, and have save state compatibility issues (The current master, for example, will fail to load a save states created by earlier untagged commits). A proper core release is going to have to wait for the 0.15 release (unless the RetroArch folks decide to cherry pick this commit into the 0.14.x branch).
2. As for SameDuck I'm not sure what's their policy is, as there isn't an official binary release of SameDuck yet (I plan to release a stable standalone version together with SameBoy 0.15, which will fix several emulation bugs as well)
3. I similarly can't really help with Windows build issues, as I only ever build the SDL port on Windows which doesn't use MinGW, but I can try if you post the errors you're getting.
4. I'm going to check rgbds 0.6.0 breaking on SameBoot, thanks for the heads up!