mappers/m4_MMC3.c: Fix memory mirroring

[rene]

• ppu.c: Check maximum rendering sprites
• Implement mirroring for MMC3 memory areas
• Pass main mapper struct to m4_mapper_IRQ_count()

[rene]

@claude

[github-actions]

Claude finished @rene's task —— View job

---

Code Review

• Gather context
• Analyze diff
• Post review

---

Summary

The PR makes three related fixes: correcting the m4_mapper_IRQ_count callback to dereference m->data instead of casting m directly, passing the parent mapper_t * (instead of mmc3) to ppu_set_mapper_trigger_cb, and adding power-of-two masking to PRG/CHR ROM accesses. The PPU change guards the sprite shift-register loop against out-of-bounds access. Overall the changes are correct and well-targeted. A few issues are worth noting:

---

src/mappers/m4_MMC3.c

Bug fix (correct) — m4_mapper_IRQ_count cast fix (m->data)

The original code cast m (a mapper_t *) directly to struct _mmc3_mapper *, which would read the mapper_t fields as if they were _mmc3_mapper fields — clearly wrong. The fix is correct.

---

prg_mask / chr_mask — assumption that sizes are powers of two

prg_mask = cartridge->rom->prg_size - 1;
// ...
return cartridge->rom->prg_rom[idx & prg_mask];

The bitmask trick (size - 1) only works correctly when prg_size / chr_size are powers of two. For standard iNES ROMs this is almost always true (16 KB * N or 8 KB * N), but it is not enforced anywhere in the code. If a ROM with a non-power-of-two size were loaded, the mask would be incorrect and produce wrong (but bounded) indices, silently giving garbled graphics/audio instead of a clean out-of-bounds crash or error.

Consider adding an assertion or a runtime check at init time:

/* In m4_mapper_init, after prg_size/chr_size are known */
assert((c->rom->prg_size & (c->rom->prg_size - 1)) == 0);
assert((c->rom->chr_size & (c->rom->chr_size - 1)) == 0);

Or at minimum add a comment documenting this assumption.

---

chr_mask computed after the address-decode block, but used for both read and write

chr_mask is computed at src/mappers/m4_MMC3.c:458 (after the large if/else if chain that computes idx). This is fine functionally, but if chr_size is ever zero (e.g. a CHR-RAM only cart where chr_size is not set), chr_mask would wrap to UINT64_MAX, turning every idx & chr_mask into idx — i.e. no protection. The existing cartridge->chr_ram check guards the write path but not the read path. A zero-size guard at the top of the function (or in init) would be safer.

---

m4_mapper_finalize: use-after-free

mmc3 = (struct _mmc3_mapper *)m->data;
free(mmc3->prg_ram);
free(m->data);
m->data = NULL;

if (mmc3->mirroring_en == 0) {   // ← mmc3 points to freed memory

mmc3 is read after m->data (and thus the memory mmc3 points to) has been freed. This is a pre-existing bug not introduced by this PR, but worth flagging since the PR touches this file.

---

src/ppu.c

Correct fix — bounds guard on sprite loop

for (i = 0; i < ppu.sprite_cnt && i < TOTAL_SPRITES_AUX; i++) {

ppu.sprite_cnt can reach 9 (the overflow detection path increments it one extra time at ppu.c:659), while TOTAL_SPRITES_AUX (the secondary OAM / shift-register array size) is presumably 8. Without the guard, the previous code would access ppu.sp_lsb[8] / ppu.sp_msb[8] and ppu.OAM_aux.sprites[8] — all out-of-bounds. The fix is correct and necessary.

The same guard was also applied to the pixel-drawing loop (ppu.c:741), which is equally correct.

---

Minor / style

• src/mappers/m4_MMC3.c:461: the break; after return in the CMEM_READ case is unreachable — pre-existing, but a minor style inconsistency.