From aef622f87ed8ac0dbafc3964d49ca295640c15b4 Mon Sep 17 00:00:00 2001 From: Jay Oster Date: Fri, 13 Sep 2024 00:39:24 -0700 Subject: [PATCH 1/2] Fix PRG bank shown in debugger 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. --- src/cart.cpp | 46 +++++++++++++++++++++++++++++++++++++--------- src/cart.h | 1 + src/debug.cpp | 12 +++++------- 3 files changed, 43 insertions(+), 16 deletions(-) diff --git a/src/cart.cpp b/src/cart.cpp index 73b5d57ed..71a9e2230 100644 --- a/src/cart.cpp +++ b/src/cart.cpp @@ -78,7 +78,11 @@ uint32 genieaddr[3]; CartInfo *currCartInfo; -static INLINE void setpageptr(int s, uint32 A, uint8 *p, int ram) { +// Reverse mapping of the address space back to bank numbers. +// Split into 2 KiB pages, like `Page`. +int PRGBankMapping[32]; + +static INLINE void setpageptr(int s, uint32 A, uint8 *p, int ram, int bank) { uint32 AB = A >> 11; int x; @@ -86,11 +90,13 @@ static INLINE void setpageptr(int s, uint32 A, uint8 *p, int ram) { for (x = (s >> 1) - 1; x >= 0; x--) { PRGIsRAM[AB + x] = ram; Page[AB + x] = p - A; + PRGBankMapping[AB + x] = bank; } else for (x = (s >> 1) - 1; x >= 0; x--) { PRGIsRAM[AB + x] = 0; Page[AB + x] = 0; + PRGBankMapping[AB + x] = bank; } } @@ -104,6 +110,7 @@ void ResetCartMapping(void) { Page[x] = nothing - x * 2048; PRGptr[x] = CHRptr[x] = 0; PRGsize[x] = CHRsize[x] = 0; + PRGBankMapping[x] = -1; } for (x = 0; x < 8; x++) { MMC5SPRVPage[x] = MMC5BGVPage[x] = VPageR[x] = nothing - 0x400 * x; @@ -140,6 +147,12 @@ void SetupCartCHRMapping(int chip, uint8 *p, uint32 size, int ram) { CHRram[chip] = ram; } +// Get the mapper-specific bank number for the given CPU address. +// Returns -1 when the page has not been mapped. +int GetPRGBank(uint16 address) { + return PRGBankMapping[address >> 11]; +} + DECLFR(CartBR) { return Page[A >> 11][A]; } @@ -157,9 +170,20 @@ DECLFR(CartBROB) { return Page[A >> 11][A]; } +// Get the cumulative size of PRG chips. +// I.e. the ROM file offset (without iNES header). +static uint32 PRGOffset(int chip) { + uint32 bank = 0; + for (int i = 0; i < chip; i ++) { + bank += PRGsize[i]; + } + return bank; +} + void setprg2r(int r, uint32 A, uint32 V) { V &= PRGmask2[r]; - setpageptr(2, A, PRGptr[r] ? (&PRGptr[r][V << 11]) : 0, PRGram[r]); + int bank = PRGptr[r] ? (PRGOffset(r) / (1 << 11) + V) : -1; + setpageptr(2, A, PRGptr[r] ? (&PRGptr[r][V << 11]) : 0, PRGram[r], bank); } void setprg2(uint32 A, uint32 V) { @@ -168,7 +192,8 @@ void setprg2(uint32 A, uint32 V) { void setprg4r(int r, uint32 A, uint32 V) { V &= PRGmask4[r]; - setpageptr(4, A, PRGptr[r] ? (&PRGptr[r][V << 12]) : 0, PRGram[r]); + int bank = PRGptr[r] ? (PRGOffset(r) / (1 << 12) + V) : -1; + setpageptr(4, A, PRGptr[r] ? (&PRGptr[r][V << 12]) : 0, PRGram[r], bank); } void setprg4(uint32 A, uint32 V) { @@ -176,14 +201,15 @@ void setprg4(uint32 A, uint32 V) { } void setprg8r(int r, uint32 A, uint32 V) { + int bank = PRGptr[r] ? (PRGOffset(r) / (1 << 13) + (V & PRGmask8[r])) : -1; if (PRGsize[r] >= 8192) { V &= PRGmask8[r]; - setpageptr(8, A, PRGptr[r] ? (&PRGptr[r][V << 13]) : 0, PRGram[r]); + setpageptr(8, A, PRGptr[r] ? (&PRGptr[r][V << 13]) : 0, PRGram[r], bank); } else { uint32 VA = V << 2; int x; for (x = 0; x < 4; x++) - setpageptr(2, A + (x << 11), PRGptr[r] ? (&PRGptr[r][((VA + x) & PRGmask2[r]) << 11]) : 0, PRGram[r]); + setpageptr(2, A + (x << 11), PRGptr[r] ? (&PRGptr[r][((VA + x) & PRGmask2[r]) << 11]) : 0, PRGram[r], bank); } } @@ -192,15 +218,16 @@ void setprg8(uint32 A, uint32 V) { } void setprg16r(int r, uint32 A, uint32 V) { + int bank = PRGptr[r] ? (PRGOffset(r) / (1 << 14) + (V & PRGmask16[r])) : -1; if (PRGsize[r] >= 16384) { V &= PRGmask16[r]; - setpageptr(16, A, PRGptr[r] ? (&PRGptr[r][V << 14]) : 0, PRGram[r]); + setpageptr(16, A, PRGptr[r] ? (&PRGptr[r][V << 14]) : 0, PRGram[r], bank); } else { uint32 VA = V << 3; int x; for (x = 0; x < 8; x++) - setpageptr(2, A + (x << 11), PRGptr[r] ? (&PRGptr[r][((VA + x) & PRGmask2[r]) << 11]) : 0, PRGram[r]); + setpageptr(2, A + (x << 11), PRGptr[r] ? (&PRGptr[r][((VA + x) & PRGmask2[r]) << 11]) : 0, PRGram[r], bank); } } @@ -209,15 +236,16 @@ void setprg16(uint32 A, uint32 V) { } void setprg32r(int r, uint32 A, uint32 V) { + int bank = PRGptr[r] ? (PRGOffset(r) / (1 << 15) + (V & PRGmask32[r])) : -1; if (PRGsize[r] >= 32768) { V &= PRGmask32[r]; - setpageptr(32, A, PRGptr[r] ? (&PRGptr[r][V << 15]) : 0, PRGram[r]); + setpageptr(32, A, PRGptr[r] ? (&PRGptr[r][V << 15]) : 0, PRGram[r], bank); } else { uint32 VA = V << 4; int x; for (x = 0; x < 16; x++) - setpageptr(2, A + (x << 11), PRGptr[r] ? (&PRGptr[r][((VA + x) & PRGmask2[r]) << 11]) : 0, PRGram[r]); + setpageptr(2, A + (x << 11), PRGptr[r] ? (&PRGptr[r][((VA + x) & PRGmask2[r]) << 11]) : 0, PRGram[r], bank); } } diff --git a/src/cart.h b/src/cart.h index 0b5813353..769b754e6 100644 --- a/src/cart.h +++ b/src/cart.h @@ -92,6 +92,7 @@ void ResetCartMapping(void); void SetupCartPRGMapping(int chip, uint8 *p, uint32 size, int ram); void SetupCartCHRMapping(int chip, uint8 *p, uint32 size, int ram); void SetupCartMirroring(int m, int hard, uint8 *extra); +int GetPRGBank(uint16 address); DECLFR(CartBROB); DECLFR(CartBR); diff --git a/src/debug.cpp b/src/debug.cpp index eef528e56..c0e1668cf 100644 --- a/src/debug.cpp +++ b/src/debug.cpp @@ -288,8 +288,6 @@ int GetPRGAddress(int A){ /** * Returns the bank for a given offset. -* Technically speaking this function does not calculate the actual bank -* where the offset resides but the 0x4000 bytes large chunk of the ROM of the offset. * * @param offs The offset * @return The bank of that offset or -1 if the offset is not part of the ROM. @@ -298,13 +296,13 @@ int getBank(int offs) { //NSF data is easy to overflow the return on. //Anything over FFFFF will kill it. + if (GameInfo && GameInfo->type == GIT_NSF) { + int addr = GetNesFileAddress(offs) - NES_HEADER_SIZE; - //GetNesFileAddress doesn't work well with Unif files - int addr = GetNesFileAddress(offs)-NES_HEADER_SIZE; - - if (GameInfo && GameInfo->type==GIT_NSF) return addr != -1 ? addr / 0x1000 : -1; - return addr != -1 ? addr / (1<= 0x6000) && (offs <= 0xFFFF)) ? GetPRGBank(offs) : -1; } int GetNesFileAddress(int A){ From 43f09cd72d262eb5050a9aa3fd9368b480a833da Mon Sep 17 00:00:00 2001 From: Jay Oster Date: Tue, 17 Sep 2024 17:42:57 -0700 Subject: [PATCH 2/2] Fix stack corruption in debugger 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. --- src/debug.cpp | 4 ++-- src/drivers/win/debugger.cpp | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/debug.cpp b/src/debug.cpp index c0e1668cf..84e1267fb 100644 --- a/src/debug.cpp +++ b/src/debug.cpp @@ -297,9 +297,9 @@ int getBank(int offs) //NSF data is easy to overflow the return on. //Anything over FFFFF will kill it. if (GameInfo && GameInfo->type == GIT_NSF) { - int addr = GetNesFileAddress(offs) - NES_HEADER_SIZE; + int addr = GetNesFileAddress(offs); - return addr != -1 ? addr / 0x1000 : -1; + return addr != -1 ? (addr - NES_HEADER_SIZE) / 0x1000 : -1; } return ((offs >= 0x6000) && (offs <= 0xFFFF)) ? GetPRGBank(offs) : -1; diff --git a/src/drivers/win/debugger.cpp b/src/drivers/win/debugger.cpp index 2ae4c47dd..e6f4139a8 100644 --- a/src/drivers/win/debugger.cpp +++ b/src/drivers/win/debugger.cpp @@ -388,8 +388,11 @@ INT_PTR CALLBACK AddbpCallB(HWND hwndDlg, UINT uMsg, WPARAM wParam, LPARAM lPara sprintf(str, "%04X", (unsigned int)lParam); SetDlgItemText(hwndDlg,IDC_ADDBP_ADDR_START,str); // also set the condition to only break at this Bank - sprintf(str, "K==#%02X", getBank(lParam)); - SetDlgItemText(hwndDlg, IDC_ADDBP_CONDITION, str); + auto bank = getBank(lParam); + if (bank > -1) { + sprintf(str, "K==#%02X", bank); + SetDlgItemText(hwndDlg, IDC_ADDBP_CONDITION, str); + } } } break;