BizHawk
BizHawk copied to clipboard
TASEmulators
Fix (or at least provide a workaround for) mGBA System Bus domain implementation
https://github.com/TASEmulators/BizHawk/blob/15056f11f2ac1f1b186e9144a259cc35d9bd633e/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBA/MGBAHawk.IMemoryDomains.cs#L41-L53
The a variable can point to a location out of range, leading to a BizHawk crash. Unfortunately, it is possible to create a GBA game that messes with mGBA core, literally.
There's already a guard on a though? Are you suggesting we remove the guard?
It only covers 28 bits, and the pointer type in the armv4 is a flat 32 bit value. I think he wants something to happen when the top 4 bits of the bus address are not 0000. These are not mirrors of the bottom 256MB, you just get dead reads.
The core emulates this properly, so if the CPU actually tries to load from one of those addresses, it "works" as it should. As far as the memory domain goes, it gives its size as 256MB so nothing should ever try to read from higher values than that. If it does, then that's a bug, either in the lua code or the hex editor code or whereever that happens; not the memory domain.
I don't see any useful value in expanding the memory domain's size to 4GB, we cover all the canonical addresses with the right thing.
The workaround is simple, error check your lua/c#/whatever code in the remote chance you are reading out ""pointers"" that go out of range (these pointers would not be valid in the first place, so games are not going to produce them naturally)
I didn't want to remove the a guard. I want this to be updated so BizHawk doesn't crash from this.
What were you doing to hit this? Every part of the frontend should properly handle the domain size.
When I said c# code, I meant external tool code (likely written in C#). You should not be hitting this at all in hawk's own tools (even then, crashing??? lua code should not crash bizhawk entirely, although an external tool could maybe crash depending on where it occurs?). I assume the only place this ever is relevant is with lua code / external tools reading out memory for a pointer, then that pointer being just invalid and thus you get thrown the out of range exception trying to dereference that pointer (which if this pointer being invalid is a possiblity you should be error checking it anyways)
Even with memory.readbyte(memory.readbyte(0xABCD)), the outer call wouldn't ever hit the throw in mGBA, because the implementation for readbyte returns 0 when an address is out of range. Hence my confusion.
http://problemkaputt.de/gbatek-gba-memory-map.htm says that 0x10000000-0xFFFFFFFF memory range actually exists, but it is never used at all. I don't know if implementing this would fix it (BizHawk is 64-bit only, though).
This crash can be reproduced by pausing the game, opening the Debugger, editing the PC register (R15), then advancing one frame. This might take a lot of attempts, but it will eventually crash.
Ah, it's from the disassembler.
System.ArgumentOutOfRangeException: address out of range
Parameter name: addr
Actual value was 3814588416.
at BizHawk.Emulation.Cores.Nintendo.GBA.MGBAHawk.<CreateMemoryDomains>b__61_0 (System.Int64 addr) [0x00028] in <ad3f109599c349dd889c0e41265c858e>:0
at BizHawk.Emulation.Common.MemoryDomainDelegate.PeekByte (System.Int64 addr) [0x00007] in <613e3acad40749bab2e5f1f9172f83ce>:0
at BizHawk.Emulation.Cores.Nintendo.GBA.ArmV4Disassembler.Disassemble (BizHawk.Emulation.Common.MemoryDomain m, System.UInt32 addr, System.Int32& length) [0x00050] in <ad3f109599c349dd889c0e41265c858e>:0
at BizHawk.Client.EmuHawk.GenericDebugger.Disassemble () [0x0003a] in <5d7b405aa28d4a058c0ad6c2017f808c>:0
at BizHawk.Client.EmuHawk.GenericDebugger.UpdateDisassembler () [0x0000c] in <5d7b405aa28d4a058c0ad6c2017f808c>:0
at BizHawk.Client.EmuHawk.GenericDebugger.FullUpdate () [0x00014] in <5d7b405aa28d4a058c0ad6c2017f808c>:0
at BizHawk.Client.EmuHawk.GenericDebugger.OnPauseChanged (System.Boolean isPaused) [0x00006] in <5d7b405aa28d4a058c0ad6c2017f808c>:0
at BizHawk.Client.EmuHawk.MainForm.set_EmulatorPaused (System.Boolean value) [0x00035] in <5d7b405aa28d4a058c0ad6c2017f808c>:0
at BizHawk.Client.EmuHawk.MainForm.PauseEmulator () [0x00001] in <5d7b405aa28d4a058c0ad6c2017f808c>:0
at (wrapper remoting-invoke-with-check) BizHawk.Client.EmuHawk.MainForm.PauseEmulator()
at BizHawk.Client.EmuHawk.MainForm.StepRunLoop_Core (System.Boolean force) [0x00132] in <5d7b405aa28d4a058c0ad6c2017f808c>:0
at BizHawk.Client.EmuHawk.MainForm.ProgramRunLoop () [0x001e7] in <5d7b405aa28d4a058c0ad6c2017f808c>:0
at (wrapper remoting-invoke-with-check) BizHawk.Client.EmuHawk.MainForm.ProgramRunLoop()
at BizHawk.Client.EmuHawk.Program.SubMain (System.String[] args) [0x003a4] in <5d7b405aa28d4a058c0ad6c2017f808c>:0
Finally, actual repro steps :tada:
Not sure where best to fix this. As I mentioned, for general usage the current definition of the memory domain makes the most sense. But for a debugger/disassembler, it will need something that exactly matches what hardware pointers resolve to. Would a separate "disassembler memory domain" makes sense?
Disassemblers like libdarm are only concerned with the opcode†, so IMO this is the Disassembler tool's problem to deal with. It's 1 line to check if the PC is in the system bus' range.
Unrelated: We check for Thumb mode, but then call darm_disasm in both branches. That function then checks again (though it distinguishes two Thumb modes?) and calls the relevant darm_armv7_disasm/darm_thumb_disasm.
†darm_disasm only uses the address parameter to check for Thumb mode.
If mgba can handle invalid address reads just fine, then why not just remove the exception? The only alternative I see is to throw memory domain size checks everywhere, and even then it's not clear what to do for invalid reads. The system bus is one of the few memory domains where I believe it makes sense to not enforce the domain size
For things like search, it makes sense to leave out address ranges that never correspond to any useful value.
Of course, I'm only talking about the debugger/disassembler situation here.
My past self seemed convinced that ArmV4Disassembler should be checking the address. Which is simple. Would that solution create any further problems?
It would probably fix this specific issue, but it begs the question why only one of the ~15 disassembler implementations needs to check the supplied address for validity. Are all other implementations error-prone as well, and it's just a matter of time when the next one shows the same issue in a similar scenario?
It looks like most implementations handle arbitrary addresses fine, just mgba's system bus is special in throwing an exception.
Also, looking at the actual implementation for mgba, there seems to be a special GBALoadBad function for reading invalid system bus addresses, so it seems like just returning 0 is not correct anyway.