[mGBA] NRE caused by peeking SRAM (i.e. in Hex Editor)

[NarryG]

Under certain games (e.g. Mother 3, Superstar Saga), if you look at the SRAM domain in the hex editor, a NullReferenceException will be thrown at PeekByte(). Pulling it up in the debugger, it appears that the backing array doesn't exist

I think it's a a resurfacing of #738. This is on 2.3.2 stable, but it doesn't look like mGBA has been touched since then.

[RetroEdit]

Also had this occur with Shrek 2. In both BizHawk 2.4 and the latest development build at time of writing (48a62943f153cb34715a10ce9416fe0970a71c9c), the whole emulator will crash if SRAM is selected in the dropdown menu for the hex editor.

I had the same traceback as NarryG found.

[RetroEdit]

From some debugging: the culprit is in BizGetMemoryAreas() of bizinterface.c. dst->sram is assigned a null value at the linked line for some reason. I don't know enough further details currently about the mGBA side of things to know the exact issue.

[RetroEdit]

This issue is not simple to resolve because it appears to be caused by core->board->memory.savedata.data returning null in _GBAGetMemoryBlock() in src/gba/core.c (side note: this is likely identical to ctx->gba->memory.savedata.data BizGetMemoryAreas()). As a fix, we might be able to use the code BizGetMemoryAreas() used before feos's commit that resulted in the current code (mgba: 5680ababe4f308f2cc6f0f6aa41df230c6989d81; BizHawk: 3a8b3361e344bf9707573c43dba2e40c7278365a).

Edit: Alright, I've confirmed the old code did indeed work. Adding it back as a quick fix would work, but it's probably ideal to figure out why the other interface didn't work like expected.

---

Another thing that is probably worth looking into while we're fixing one part of the SaveRAM interfacing in BizHawk:

<natt> Even if those games have saveram, the fact seems to remain that there's nowhere in the current code that will conditionally not create the saveram memory domain if the game in question doesn't have saveram

Depending on how the interfacing code works, this may or may not be an issue.

[RetroEdit]

A little summary of my current understanding of the issue:

First, in BizHawk mGBA's SaveRAM is initialized within BizCreate() and eventually ends up calling GBASavedataInit(). mGBA's internal savedata format has a uint8_t* data field that is initially set to 0 in GBASavedataInit() My guess is that this is never set to non-null value. Instead, mGBA probably uses the struct VFile* vf field of the savedata in its own internal accessing of savedata, as evidenced by the BizPutSavedata interface using this method and properly functioning.

This is where the problem arises: when _GBAGetMemoryBlock() is called to fetch the savedata, it does access the data field of the savedata, which is why it returns a null value.

---

For convenience, the relevant code is included below:

Initializing savedata

// mgba/src/platform/bizhawk/bizinterface.c
    memset(ctx->sram, 0xff, 131072);
    ctx->sramvf = VFileFromMemory(ctx->sram, 131072);
    ctx->core->loadSave(ctx->core, ctx->sramvf);

// mgba/src/gba/core.c
core->loadSave = _GBACoreLoadSave;

static bool _GBACoreLoadSave(struct mCore* core, struct VFile* vf) {
    return GBALoadSave(core->board, vf);
}

// mgba/src/gba/gba.c
bool GBALoadSave(struct GBA* gba, struct VFile* sav) {
    GBASavedataInit(&gba->memory.savedata, sav);
    return true;
}

// mgba/src/gba/savedata.c
void GBASavedataInit(struct GBASavedata* savedata, struct VFile* vf) {
    savedata->type = SAVEDATA_AUTODETECT;
    savedata->data = 0;
    savedata->command = EEPROM_COMMAND_NULL;
    savedata->flashState = FLASH_STATE_RAW;
    savedata->vf = vf;
    savedata->realVf = vf;
    savedata->mapMode = MAP_WRITE;
    savedata->maskWriteback = false;
    savedata->dirty = 0;
    savedata->dirtAge = 0;
    savedata->dust.name = "GBA Savedata Settling";
    savedata->dust.priority = 0x70;
    savedata->dust.context = savedata;
    savedata->dust.callback = _ashesToAshes;
}

BizHawk trying to peek/poke at savedata

// mgba/src/gba/core.c
void* _GBAGetMemoryBlock(struct mCore* core, size_t id, size_t* sizeOut) {
    struct GBA* gba = core->board;
    switch (id) {
    // ...
    case REGION_CART_SRAM_MIRROR:
        *sizeOut = GBASavedataSize(&gba->memory.savedata);
        return gba->memory.savedata.data;
    }
}

[RetroEdit]

This does not affect all games. One example of a game not affected is Pokemon Emerald Version, which also appears to not be affected by #1593. Therefore, there's a reasonable chance the two issues are related.

[zeromus]

Emerald works because its save type is hardcoded in mgba's overrides.c

One fundamental problem is that save types aren't generally known at startup time (it's in an autodetect process in mgba) and bizhawk isn't currently equipped (at least, the mgba c# side) to have memory domains changing size. I suspect the frontend won't handle it well, either. But one thing at a time. We can try tackling this without too much trouble, but it may get messy.

Now for the next problem. Mgba's savedata code is a maze and I can't make sense out of it. The "mask" and "unmask" operations in particular make no sense at all. However, if we assume it works--somehow--then fixing prior "fundamental problem" should solve THIS bug, at least.

A final problem is that GBASavedataSize() returns the "vf" size in case of SAVEDATA_AUTODETECT.. which is sensible, I guess, insofar as it is like, not specially-memory-mapped due to knowledge of what chip has been detected. However it CANNOT return the pointer!! Because the way it's implemented, until the autodetection works, a pointer hasn't come out yet.

Now, bearing in mind all this is related and how I do this may be irrelevant after whatever we do to fix the other issue, I have fixed the proximate issues here.... HOWEVER, I'm not sure anyone will like it. It's possibly more useful if we just expose the whole 128KB buffer that we handle ourselves and give to mgba to (mis?)manage. We will have to let the users decide. I think you can figure out how to do that, if it's demanded. Otherwise I can handle it.

With shrek 2 (my main test case here) you have to wait until the file select menu scene appears for the SRAM memory to get detected.

[zeromus]

Seems not to have been related to #1593 indeed. You guys need to evaluate whether you like this new behaviour before we can close it though

[RetroEdit]

I would prefer for the buffer itself to be exposed. Here's why: exposing the buffer allows SaveRAM to be modified prior to it being formally identified by mGBA.

SaveRAM not being processed when loading games is an artifact of mGBA's implementation and partially the way ROMs are dumped (i.e. if ROM formats contained specifications for the game's saving hardware, this would not be an issue). It does not need to be a limitation for BizHawk, unless it's literally a technical necessity.

I've been convinced otherwise upon further examination that at the very least we probably don't need writing if we're going to have another SaveRAM domain.

[RetroEdit]

Having a memory domain size of 0 also appears to be a problem for the hex editor currently. If you try to scroll, you have a problem:

https://github.com/TASVideos/BizHawk/blob/27a5fcdd2081d992375ebf904feaf6eb74e1adf5/src/BizHawk.Client.EmuHawk/tools/HexEditor/HexEditor.cs#L2230

System.ArgumentOutOfRangeException: 'Value of '0' is not valid for 'Value'. 'Value' should be between 'minimum' and 'maximum'.
Parameter name: Value'

This exception was originally thrown at this call stack:
    System.Windows.Forms.ScrollBar.Value.set(int)
    BizHawk.Client.EmuHawk.HexEditor.HexScrollBar_ValueChanged(object, System.EventArgs) in HexEditor.cs
    System.Windows.Forms.ScrollBar.OnValueChanged(System.EventArgs)
    System.Windows.Forms.ScrollBar.Value.set(int)
    BizHawk.Client.EmuHawk.HexEditor.HexEditor_MouseWheel(object, System.Windows.Forms.MouseEventArgs) in HexEditor.cs
    ...
    [Call Stack Truncated]

[zeromus]

You can modify saveram in a hex editor prior to loading the rom...

[RetroEdit]

Since #2220 was not deemed an acceptable fix, it needs to be done differently than the current approach. I meant to get around to doing it by now, but I haven't.

@adelikat This is a pretty easy fix, so I'd like to get it fixed before 2.5 releases ideally, especially since Zeromus's implementation can cause lots of random crashes when interacting with the hex editor (and the old code just plain crashes when trying to open the SaveRAM domain in memory, most of the time).

[zeromus]

It's possibly more useful if we just expose the whole 128KB buffer that we handle ourselves and give to mgba to (mis?)manage.