Audit pass 5 - five distinct cleanups bundled into one omnibus.
1. Element-stride byte-swapping for savestate fields (state.h, state.c,
fceu-endian.{h,c})
The SFORMAT 's' field was previously {bit 31 = RLSB, bits 0..30 =
byte size}. RLSB triggers FlipByteOrder() on MSB hosts, which
reverses the entire entry buffer end-to-end. That is correct for a
single primitive (size <= 8 bytes) but wrong for an array of
multi-byte primitives - reversing the whole buffer would swap
element 0 with element N-1 and reverse their bytes too, scrambling
the data.
The previous workaround was either splitting an N-element array
into N separate single-primitive entries with distinct chunk IDs
(n106 PlayIndex split into IDX0..IDX7) or skipping the entry
entirely on big-endian hosts (the GEKKO #ifndef in vrc6.c / vrc7.c).
Both approaches mean BE saves are not portable to LE and vice
versa, and force the same workaround at every new array site.
This pass adds proper stride support:
* SFORMAT 's' encoding is now {bit 31 = RLSB, bits 24..30 =
stride in bytes (0 = legacy/unset), bits 0..23 = byte size}.
16 MiB max size, well above any actual savestate field.
* FCEUSTATE_RLSB_ARRAY(stride) macro for the new pattern.
* FlipByteOrderStrided() byte-swaps each element of an array
independently. Round-trip identity verified: [01 00 00 00 ...]
-> [00 00 00 01 ...] -> [01 00 00 00 ...].
* state.c's SubWrite / ReadStateChunk / CheckS use new helpers
sf_size() / sf_stride() / sf_flip() that mask the size out of
the new bit layout and dispatch to the strided variant when
stride < size.
Backwards compatible: legacy single-primitive entries (size == 1,
2, 4, 8) leave the stride bits at zero, which sf_stride() reads
as "stride equals size" and falls through to FlipByteOrder() as
before. No on-disk format change. Existing single-primitive RLSB
sites are unchanged.
The infrastructure is now in place so any future SFORMAT entry
that is an array of multi-byte primitives can be expressed as a
single entry (e.g. "{ buf, sizeof(buf) | FCEUSTATE_RLSB_ARRAY(4),
"BUF." }") without splitting or skipping. The existing PlayIndex
split and GEKKO #ifndefs are intentionally left untouched -
migrating them would alter the on-disk savestate format and is a
separate decision.
2. iNES1-vs-iNES2 sizing helpers (cart.h)
Twelve sites across the codebase encoded the same conditional:
info->iNES2 ? (info->PRGRamSize + info->PRGRamSaveSize) : default
Sometimes for PRGRAM, sometimes for CHRRAM, sometimes in bytes,
sometimes after dividing by 1024. The pattern is verbose and easy
to write inconsistently.
Added two inline helpers in cart.h:
- CartInfo_PRGRAM_bytes(info, default_bytes)
- CartInfo_CHRRAM_bytes(info, default_bytes)
Migrated 9 of the 13 sites: cartram.c (2), 162.c, 163.c, 134.c,
399.c, 478.c, 480.c, 484.c. The remaining 4 are non-helper-fitting
variants (164.c special masking, 2 cartram SaveGameLen sites with
different fallback semantics, mmc3.c Boogerman submapper detection).
3. -Wundef enabled permanently (Makefile.libretro)
Zero warnings out of the box - no #if-on-undefined-macro footguns
in the codebase. Now part of WARNING_DEFINES alongside the existing
-Wsign-compare.
4. -Wmissing-prototypes enabled permanently (Makefile.libretro)
Started at 198 warnings, cleared all of them:
* Mass-static-ified ~96 functions across 75 files that were
defined non-static but only used within their own translation
unit. (See static_prototype_fixer.py in the development notes.)
* K&R-style empty-parens prototypes "()" replaced with explicit
"(void)" across all asic_*.{c,h} files - GCC treats "()" as
"any args" and refuses to match it against a separate K&R
definition.
* Added missing forward declarations to public headers:
- fds.h (FDSLoad)
- nsf.h (NSFLoad)
- ines.h (iNESLoad)
- unif.h (UNIFLoad)
- latch.h (LatchHardReset, K&R fix)
- eeprom_93Cx6.h (eeprom_93Cx6_read, K&R fix)
Each header gained an "#include "file.h"" where needed.
* fds_apu.c now includes its own fds_apu.h header (was missing).
* fds_apu.h: removed unused FDSSoundRead declaration (the function
is internal-static).
* cartram.h: removed unused CartRAM_close declaration (the function
is internal-static).
* input.h: added a centralised block of FCEU_Init* prototypes
(Zapper, Mouse, Powerpad, Arkanoid, VirtualBoy, FKB, SuborKB,
PEC586KB, HS, Mahjong, FamilyTrainerA/B, OekaKids, TopRider,
BarcodeWorld, BattleBox, QuizKing, FTrainerA/B, SpaceShadow,
LCDCompZapper, ArkanoidFC) plus FCEU_ZapperSetTolerance. These
were previously declared as "extern" inside src/input.c.
* Static-ified FP_FASTAPASS callbacks in 106.c, 65.c, 67.c,
asic_h3001.c, asic_vrc3.c (those with no external callers);
left non-static for those that have header decls or are
referenced from sibling .c files (asic_mmc1, asic_vrc6,
asic_vrc7, flashrom).
* For a small set of cross-file functions where adding a header
was disproportionate to the value (MMC5_hb, NSFMMC5_Close,
GetKeyboard, FCEU_GetJoyJoy), placed a forward declaration
immediately above the definition. This satisfies
-Wmissing-prototypes (which checks for any prior declaration
in scope) without churning the public-header layout.
5. -Wshadow partial cleanup (not enabled permanently)
Fixed five real shadows that were either bugs or actively
misleading:
* src/boards/476.c: removed an inner "int i" that shadowed the
outer loop counter.
* src/boards/mmc5.c MMC5_hb: parameter "scanline" renamed to
"sl_param" (was shadowing the global "scanline").
* src/boards/n106.c DoNamcoSound: parameter "Wave" renamed to
"WaveBuf" (was shadowing the global Wave audio buffer); also
updated the forward declaration and the matching parameter
on sound.h's NeoFill function pointer typedef.
* src/boards/vrc7.c UpdateOPLNEO: same Wave -> WaveBuf rename.
* src/ntsc/nes_ntsc_impl.h: renamed an inner loop counter "n"
that shadowed an outer "n".
* src/drivers/libretro/libretro.c FCEUD_RegionOverride: local
"pal" renamed to "is_pal" (was shadowing the typedef "pal" from
palette.h).
* src/palette.c FCEUI_SetPaletteArray: parameter "pal" renamed
to "data" (same shadow); driver.h declaration updated to match.
-Wshadow itself is NOT enabled permanently because the remaining
warnings are deliberate parameter naming conventions (XBuf in
draw functions, X in cpu hooks) and third-party blargg ntsc code.
In addition, four files were touched as part of an MSVC-build fix
that came up mid-pass: src/fds.c, src/nsf.c, src/ines.c, and
src/drivers/libretro/libretro_dipswitch.c had snprintf() calls
introduced in pass 4 that fail to link on pre-MSVC2015 toolchains
when STATIC_LINKING=1 (the libretro-common compat_snprintf.c shim
isn't compiled in those configurations). Replaced each snprintf with
either sprintf-into-bounded-buffer (the format strings have known
maximum output) or strlcpy/strlcat for the dipswitch key-build case.
All output is still bounded; truncation happens via strl*'s normal
truncation semantics where applicable.
All added code is C89-clean (top-of-block declarations only, no
mixed decls, no // comments, INLINE macro from fceu-types.h instead
of bare "inline"). Builds clean under -std=gnu11 with -Wno-write-
strings -Wsign-compare -Wundef -Wmissing-prototypes; zero errors,
zero warnings.
Determinism audit (audit_determinism.py): no rand/time/long
double/threads issues introduced.
Omnibus cleanup pass. Build is clean on `make platform=unix` with
zero errors and zero warnings, including under
`-Wsign-compare -Wstrict-aliasing=2 -Wcast-align`. The
`-Wsign-compare` flag is now permanently enabled in
WARNING_DEFINES.
================================================================
A. FCEU_gmalloc no longer exit()s on OOM
================================================================
A libretro core must not call exit(): doing so tears down the entire
frontend. FCEU_gmalloc previously did exactly that on allocation
failure ("Doing a hard exit"). It now returns NULL with the same
diagnostic message.
Loader-level call sites that can fail their parent function (i.e.
return 0 from FDSLoad/iNESLoad/NSFLoad to refuse the cart) now check
the return value:
- ines.c: trainerpoo, ExtraNTARAM
- nsf.c: ExWRAM (both branches)
- fds.c: FDSBIOS, CHRRAM, FDSRAM
Mapper-level callers (~200 sites) are intentionally left as-is:
they live in void-returning Init/Power functions where graceful
failure isn't possible without a much larger restructuring. With
NULL returns those mappers will null-deref on first access, which
is contained to the core - the libretro frontend stays up. This is
strictly better than the previous behaviour of exit()ing the entire
frontend.
================================================================
B. -Wsign-compare cleanup (27 warnings -> 0)
================================================================
Surveyed every signed/unsigned comparison the compiler flagged and
fixed each one. Most fell into a few patterns:
Loop variables: changed `int x` to `uint32_t x` for loops over
uint32_t counts (TotalSides in fds.c, the trainer-copy loop in
6_8_12_17_561_562.c, soundOffset->SOUNDTS in 594.c).
scanline (signed) vs totalscanlines/normal_scanlines (unsigned)
in ppu.c: cast (unsigned)scanline at the comparison sites. The
scanline values are guaranteed >= 0 at every comparison site
(the loop initialises scanline=0 and increments).
rate_adjust macro in emu2413.c: the ?: was returning either
signed or unsigned depending on `rate`. Cast both branches to
uint32_t.
Mixed ?: branches in cartram.c (PRGRamSaveSize signed vs
WRAMSize unsigned): cast PRGRamSaveSize to uint32_t at use.
Other one-off casts in libretro.c, n625092.c, nsf.c, sound.c,
zapper.c, eeprom_93Cx6.c.
The Makefile change keeps -Wsign-compare permanently enabled so
new sign-compare bugs trip CI immediately.
Files touched: 594.c, 6_8_12_17_561_562.c, cartram.c,
eeprom_93Cx6.c, emu2413.c, n625092.c, fds.c, nsf.c, ppu.c,
sound.c, input/zapper.c, drivers/libretro/libretro.c, plus
Makefile.libretro.
================================================================
C. NULL-deref hardening on libretro callbacks
================================================================
retro_serialize and retro_unserialize now reject NULL data
pointers before passing them to memstream_set_buffer (which
would have null-deref'd inside the memstream code).
Other retro_* entry points that take pointers were already guarded
in earlier passes (retro_set_controller_port_device,
retro_get_memory_data, retro_get_memory_size, retro_load_game,
retro_cheat_set).
================================================================
D. Mapper coverage spot-check
================================================================
Wrote a heuristic scanner to find the savestate-load-array-index
bug class fixed in pass 1 (variable masked at write but unmasked
at restore -> OOB index). Scanner flagged 3 candidates across 424
mappers; manual review confirmed all three are false positives:
- sachen.c `cmd`: theoretical bug, but only one writer is wired
per game and that writer masks at use.
- unrom512.c `flash_state` and `latcha`: both already clamped
in StateRestore (added in pass 1).
The systematic bug class was thoroughly addressed in pass 1.
================================================================
E. Strip never-defined #ifdef symbols
================================================================
Surveyed every #ifdef symbol in the build and cross-checked
against #defines (in source and in build files). Removed code
gated on symbols that are never defined for any platform target:
- DEBUG_MAPPER (datalatch.c, 2 sites): a debug-print NROMWrite
handler. Dead.
- FRAMESKIP (fceu.h, driver.h, ppu.c, 4 sites): a legacy-FCEU
frameskip path. The libretro driver's `skip` argument to
FCEUI_Emulate is always 0, and FCEUI_FrameSkip is never
called by any libretro frontend. Removed the conditional
rendering branch in FCEUPPU_Loop along with the
FCEU_PutImageDummy declaration.
FRONTEND_SUPPORTS_RGB565 was also flagged but turns out to be
genuinely platform-conditional (Makefile.common defines it when
WANT_32BPP=0). Kept.
================================================================
F. assert() audit
================================================================
Two assert() calls live in src/ntsc/nes_ntsc_impl.h - third-party
NTSC filter code from blargg, both NaN sanity checks
(`assert(x == x)`). NDEBUG is in the build flags so they compile
to no-ops. No fceumm code uses assert. Nothing to do here.
================================================================
G. const-correctness
================================================================
Function signatures that take strings they don't modify now take
const char *:
FCEU_printf, FCEU_PrintError (const char *format)
FCEUD_PrintError, FCEUD_Message (const char *)
FCEU_MakeFName (const char *cd1)
md5_update (const uint8_t *input)
md5_process (const uint8_t data[64])
also made `static`
================================================================
H. Const-fold static lookup tables
================================================================
Marked static lookup tables const where they are never written:
x6502.c: CycTable[256] (cycle counts)
md5.c: md5_padding[64] (MD5 padding)
vsuni.c: secdata[2][32], secptr (VS security data)
palette.c: rtmul/gtmul/btmul[7] (palette multipliers)
input/cursor.c: GunSight, FCEUcursor (sprite data)
input/pec586kb.c, fkb.c, suborkb.c: matrix (key matrices)
boards/8237.c: regperm, adrperm, protarray (mapper perms)
boards/datalatch.c: M538Banks (bank table)
boards/187.c: prot_data
boards/121.c: prot_array
boards/bonza.c: sim0reset
boards/pec-586.c: bs_tbl, br_tbl
boards/178.c: step_size, step_adj (ADPCM tables)
boards/244.c: prg_perm, chr_perm
boards/bmc42in1r.c: banks
boards/emu2413.c: SL (sustain levels)
NSFROM in nsf.c looks like a lookup table but is rewritten at
runtime to patch in addresses, so it stays mutable.
================================================================
I. Reduce strlen calls
================================================================
Replaced `strlen(STRING_LITERAL)` with `sizeof(STRING_LITERAL) - 1`
where the argument is a compile-time-known string literal:
- libretro.c retro_set_environment APU loop: was calling
strlen("fceumm_apu_") on every loop iteration to compute the
same constant offset.
- nsf.c visualizer: strlen("Song:").
Other strlen sites in cheat.c, libretro.c, unif.c either already
cache to a local size_t or operate on runtime-supplied strings
where caching would not help.
================================================================
Build status
================================================================
`make platform=unix` clean: zero errors, zero warnings.
With -Wsign-compare -Wstrict-aliasing=2 -Wcast-align: zero warnings.
audit_determinism.py: 0 issues.
Output binary 4,388,408 bytes (was 4,388,576 from upstream
004c147; -168 bytes from dead-code removal).
- this implements blargg's nes ntsc filters using core options
- an optional height doubling is also added but disabled for performance reasons (might make that optional as a core option)
- since PS2 and PSP have their own blitter branches, these platforms do not have the ntsc filters since i dont have the means to test on those systems.
- compile with HAVE_NTSC=1 to have these options, HAVE_NTSC=0 disabled filter including core options
- HAVE_NTSC=1 is set as default, other than PS2 and PSP as stated above.
