Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PRG bank shown in debugger #757

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

parasyte
Copy link
Contributor

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.

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.
@vadosnaprimer
Copy link

Does it have the same problem in the tracelog?

@parasyte
Copy link
Contributor Author

Yes, it does. Everything that uses the getBank() function will now display a more accurate bank number based on how the address space is mapped. The old behavior was to assume that all banks are 16 KiB.

Here's a list of notable places where the bank number is getting updated:

  • Disassembler
  • Breakpoint conditions
  • Memory editor context menu
  • Trace logger

Possibly broken now:

  • generateNLFilenameForAddress() in debuggersp.cpp.
  • getBankIndexForAddress() in debuggersp.cpp.
  • loadNameFiles() in debuggersp.cpp.
  • generateNLFilenameForAddress() in debugsymboltable.cpp (dead code).

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 04:8000 and 02:8000 are ambiguous without the size (PRG window) context.

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.
@parasyte parasyte force-pushed the fix/debugger-prg-bank branch from ab1ef8c to 43f09cd Compare September 19, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants