Fix PRG bank shown in debugger#757
Conversation
The old code used a hardcoded 16 KiB bank size, regardless of how the mapper actually maps banks. There is still a lot of code making this assumption. See `debuggerPageSize`. Mostly for symbolic debugging.
|
Does it have the same problem in the tracelog? |
|
Yes, it does. Everything that uses the Here's a list of notable places where the bank number is getting updated:
Possibly broken now:
The symbol table code is all hardcoded with the old 16 KiB bank size assumption. Additional context: I made this patch for an MMC3 game, and the debugger and trace logger are showing the correct bank numbers with it. MMC3's fixed-size 16 KiB bank window is treated as two 8 KiB banks, but that seems reasonable. MMC5 is an example that I expect has problems presenting a sane bank number (it can use 8 KiB, 16 KiB, or 32 KiB bank sizes, selectable at runtime). It would probably be helpful if bank size information was provided in addition to a bank index number. E.g., "4th 8 KiB bank" and "2nd 16 KiB bank" both point to the same location, but in address-form |
Corrupt the stack with the following process (prior to this commit):
- Open FCEUX, do NOT load a ROM.
- Open the debugger window.
- Resize the debugger window to force it to refresh the disassembler.
- (May not be necessary if you have already saved the debugger state
with a larger-than-default window size.)
- Double click on any address that is not $0000.
- The Add Breakpoint window will open with the condition string filled
with `K==#FFFFFFFF`, which is at least 13 characters long.
- The `str` array that this string is written to only has capacity for 8
characters.
- Whoops!
This commit fixes a bug in the original `getBank()` implementation when
`GetNesFileAddress()` returns -1.
See: https://github.com/TASEmulators/fceux/blob/f980ec2bc7dc962f6cd76b9ae3131f2eb902c9e7/src/debug.cpp#L303-L307
`addr` will be -17 in this error condition after the iNES header size is
subtracted. This causes the following error checks to fail and weird
integer arithmetic (specifically `-17 / (1 << 14)` is 0!) then returns 0
to the caller, indicating a successful result for bank number 0.
With the fix, `getBank()` now properly returns -1 and causes the stack
corruption with unrelated code as described above. This commit adds
proper error handling to the code in question.
Additionally, the previous commit also kept the original
`-17 / 0x1000 == 0` behavior for NSFs. That is now corrected in this
commit; `getBank()` always returns -1 for errors instead of integer
divisions truncating negative results to 0.
ab1ef8c to
43f09cd
Compare
The old code used a hardcoded 16 KiB bank size, regardless of how the mapper actually maps banks.
There is still a lot of code making this assumption. See
debuggerPageSize. Mostly for symbolic debugging.TBD: This PR is a draft because it may be confusing if the mapper simultaneously uses varying bank sizes.