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).
Three follow-up cleanups on top of the determinism / typedef / LE
work in f2a5be3.
================================================================
Pass 1: fix strict-aliasing UB in PPU sprite buffer and FCEU_dwmemset
================================================================
Three sites were doing type-punning through uint32_t* casts that
GCC flags with -Wstrict-aliasing=2:
1. ppu.c FetchSpriteData (two near-identical sites). After populating
a local SPRB struct (4 bytes: ca[2], atr, x), the prior code did
'*(uint32_t*)&SPRBUF[ns << 2] = *(uint32_t*)&dst' to store the
struct as one 32-bit word. This violates strict-aliasing (uint8_t
buffer accessed as uint32_t, struct accessed as uint32_t) and
silently assumes 4-byte alignment of SPRBUF + (ns << 2). Use
memcpy instead - GCC and Clang lower it to the same single
32-bit store.
2. fceu-memory.h FCEU_dwmemset macro. Same pattern every iteration:
'*(uint32_t*)& (d)[_x] = c'. Replaced with memcpy(... &v, 4) in
the macro body. Same generated code, now alias- and align-clean.
Added #include <string.h> to fceu-memory.h itself (4 callers
relied on transitive includes for memcpy).
================================================================
Pass 2: bounded string operations
================================================================
Sweep across the core and libretro driver to replace every unbounded
string function with its size-aware counterpart:
strcpy -> strlcpy (with explicit destination size)
strncpy -> strlcpy (silent truncation -> guaranteed NUL termination
and known-size semantics; behaviourally identical
at every call site here because callers either
pre-zeroed the buffer or treated truncation as
a guaranteed terminator)
strcat -> strlcpy at offset (track 'pos' across appends to avoid
rescanning the buffer with strlen and to keep
the bound explicit at every step)
sprintf -> snprintf (with sizeof(buf) as the bound)
The compat/strl.h header from libretro-common is on the include path
for every translation unit; the libretro driver files were already
using strlcpy via stdstring.h transitive include. Core files (ines.c,
unif.c, cheat.c, nsf.c, state.c, general.c) gain explicit
'#include <compat/strl.h>'.
Per-file changes:
general.c
- FCEUI_SetBaseDirectory: strncpy + manual NUL -> strlcpy
- FCEU_MakeFName: malloc(strlen+1) + strcpy -> sized strlcpy,
malloc NULL-check added (was unchecked).
ines.c
- 'gigastr' iNES-header-warning building.
Pre-existing bug fixed: every sprintf(gigastr + gigastr_len, ...)
wrote at the SAME offset captured once at the top of the block,
so when multiple 'tofix' bits were set, only the LAST fragment
survived (each sprintf clobbered the previous). Replaced with a
running 'pos' offset across snprintf and strlcpy-at-offset calls.
- 6 sprintf -> snprintf, 3 strcat -> strlcpy at offset, 1 strcpy
-> strlcpy.
unif.c
- GameInfo->name allocation: strcpy -> strlcpy with explicit size.
cheat.c
- FCEUI_AddCheat: malloc + strcpy -> sized strlcpy.
- FCEUI_SetCheat: realloc + strcpy -> sized strlcpy.
nsf.c
- FCEUI_NSFGetInfo: 3x strncpy -> strlcpy.
- Visualiser snbuf sprintf -> snprintf (the 16-byte buffer was
technically overflowable for very large song counts).
state.c
- AddExState description copy: strncpy -> strlcpy. Preserves
4-byte SFORMAT-tag semantics.
fds.c
- Disk-tag formatting: sprintf "DDT%d" -> snprintf.
boards/__serial.c
- 2 sprintf -> snprintf (Windows-only SerialOpen path).
drivers/libretro/libretro.c
- retro_cheat_set: sprintf "N/A" + strcpy literal -> strlcpy.
drivers/libretro/libretro_dipswitch.c
- VS-DIP key building: sprintf -> snprintf.
- core_key calloc + strcpy -> sized strlcpy. calloc NULL-check
added (was unchecked - dereferencing NULL on OOM).
drivers/libretro/libretro_core_options.h
- values_buf assembly: single strcpy + N strcat replaced with
strlcpy + strlcpy-at-offset using a running 'pos'. Each step
bounded by remaining buffer space.
================================================================
Pass 3: remove FCEUDEF_DEBUGGER and the unused debugger scaffolding
================================================================
The FCEUDEF_DEBUGGER macro was never defined for libretro builds, so
every block guarded by it was dead code. Removing the macro takes
out:
- src/debug.c (FCEUI_DumpMem, FCEUI_DumpVid, FCEUI_LoadMem,
FCEUI_Disassemble, FCEUI_MemDump, FCEUI_MemSafePeek,
FCEUI_MemPoke, breakpoint set/get/list, FCEUI_SetCPUCallback).
Not in Makefile.common SOURCES_C, never compiled. None of the
declared functions had any caller in any compiled .c file.
- src/debug.h (declarations for the above).
- x6502.c X6502_RunDebug. The dual-implementation pattern with a
function pointer that switched between RunNormal and RunDebug
is now a single direct X6502_Run.
- x6502.c X6502_Debug, FCEUI_NMI, FCEUI_IRQ, FCEUI_GetIVectors.
Set/get debugger hooks. No callers.
- x6502.c RdMemHook, WrMemHook, XSave, debugmode. Hook scaffolding
used only by RunDebug.
- x6502struct.h X6502 fields preexec, CPUHook, ReadHook, WriteHook.
Set only inside RunDebug, never read elsewhere.
- fceuindbg variable. Set to 1 only in FCEUI_GetIVectors and
X6502_RunDebug (both removed); was always 0 elsewhere, so every
'if (!fceuindbg)' check was a no-op. Removed both the variable
(defined in ppu.c, declared in fceu.h) and every check site
across sound.c, input.c, fds.c, nsf.c, ppu.c, mmc5.c, n106.c,
BMW8544.c, and the input drivers (arkanoid, mahjong, mouse,
pec586kb, powerpad, zapper).
- All FCEUDEF_DEBUGGER conditional declarations from driver.h.
The 'if (!fceuindbg)' checks gated side-effects (joypad-bit-counter
increments, PPU register reads, etc.) so a future debugger could peek
at memory without advancing the emulator state. With the debugger
gone, those side-effects are now unconditional - which is what they
should have been anyway in a libretro build.
If a future developer needs a debugger they should resurrect this
out of git history into a separate debugger-frontend project rather
than re-introducing a build-time toggle that nothing in the libretro
build can ever exercise.
================================================================
Build status
================================================================
Build clean on `make platform=unix` with zero errors and zero
warnings. Output binary 184 bytes smaller than upstream f2a5be3
(4,388,840 -> 4,388,656). 31 files changed, 127 insertions, 863
deletions.
Three follow-up audit passes on top of the memory-safety / leak /
savestate-portability work in 1185db8.
==============================================================
Pass 1: replace custom typedefs with C99 stdint types throughout
==============================================================
The custom uint8 / uint16 / uint32 / uint64 / int8 / int16 / int32 /
int64 typedefs in src/fceu-types.h were just simple aliases for the
C99 stdint.h types. Replace them with the standard names directly.
- 498 files modified
- ~3,400 token replacements (uint8 -> uint8_t, etc)
- fceu-types.h slimmed down to just INLINE / GINLINE / FASTAPASS
macros and the readfunc / writefunc function-pointer typedefs
(those now use uint8_t / uint32_t natively)
- Build clean on `make platform=unix` with zero new warnings
- Output binary size unchanged - confirming semantic equivalence
Mechanical replacement done with a Python script that uses word-
boundary regex to avoid false positives (e.g. 'uint32_t' was
correctly left alone because '_' is a word character so 'uint32'
is not a complete word inside it).
================================================================
Pass 2: prefer memcpy on LE hosts for endian read/write helpers
================================================================
fceu-endian.c's write32le_mem, FCEU_en32lsb, and FCEU_de32lsb
performed bytewise composition/decomposition unconditionally. On
LE hosts the in-memory representation already matches the desired
LE on-disk format, so a single memcpy is equivalent and lets the
compiler emit a single load/store rather than four byte ops.
- The bytewise path is kept inside #ifdef MSB_FIRST for BE hosts
where it implements the actual byte swap
- Both forms produce identical results; this is a code-clarity
change more than a performance one (the optimizer was already
merging the shifts on LE), but it documents the intent and
removes a strict-aliasing-flavoured cast through
*(uint32_t*)Bufo
- Added missing #include <string.h> in fceu-endian.c which was
relying on transitive includes for memcpy
Other MSB_FIRST sites in the codebase (state.c FlipByteOrder
guards, ppu.c sprite-line rendering, boards/unrom512.c flash-write-
counter access) were already optimized for LE; they were verified
correct rather than changed.
================================================================
Pass 3: frame determinism for replay and netplay
================================================================
Two libc rand() sites in core were replaced with a local xorshift32
PRNG so that NES games which read uninitialised memory or hit
hardware "weak bit" emulation produce reproducible behaviour across
runs. NES titles routinely read uninitialised RAM (variables not
zeroed before use, sprite Y-position set by junk-on-stack), so the
RAM contents at power-on subtly affect game behaviour. With libc
rand(), those contents depend on whether anyone else seeded rand()
in the same process - a different libretro frontend, a different
audio backend init order, or any frontend that does srand(time(0))
all break replay / netplay frame-determinism.
1. fceu.c FCEU_MemoryRand. Used to fill RAM (PowerNES) and CHR-RAM
(iNES_Init) at power-on when option_ramstate=2 (random init).
Replaced with a local xorshift32 PRNG, exposed via a new
FCEU_MemoryRand_Reseed(uint32_t) function called once per
power-on:
- PowerNES seeds from the first 4 bytes of GameInfo->MD5 (set
by all loaders before PowerNES runs) so identical ROMs
produce identical RAM, different ROMs differ
- iNES_Init seeds from iNESCart.PRGCRC32 before the CHR-RAM
fill so two builds of the same ROM get the same CHR-RAM
- The PRNG state advances across multiple FCEU_MemoryRand
calls within one power-on so RAM and CHR-RAM get different
content (matching NES hardware reality)
2. boards/rt-01.c UNLRT01Read. The RT-01 board has 'weak bit'
protected EPROM regions; reads of 0xCE80-0xCEFF and 0xFE80-
0xFEFF return 0xF2 with the low 3 bits randomised. Replaced
libc rand() with a local xorshift32 seeded at power-on, and
added the PRNG state to the savestate via AddExState with key
"WBKS" so save / load / rewind / netplay rollback all stay
deterministic.
In addition, two long-double-to-int truncations were changed to
double for cross-platform FP determinism:
- sound.c SetSoundVariables: soundtsinc
- boards/n106.c DoNamcoSound: inc
long double has platform-dependent precision (80-bit on x87,
64-bit with -mfpmath=sse, 128-bit on PowerPC), so the truncated
integer result varied across these platforms. double is
guaranteed 64-bit IEEE-754 portably.
After this pass, the core has no time(), clock(), gettimeofday(),
clock_gettime(), getpid(), getuid(), getgid(), getenv(), gethostid(),
pthread, std::thread, OpenMP, signal handler, or non-deterministic-
malloc dependency. Verified with a Python scanner that greps the
source for these patterns; runs clean.
The PPU / APU / CPU power-on already explicitly memset all state
buffers to 0 (deterministic), and ROM/CHR-ROM allocation already
memsets to 0xFF before partial fread (deterministic regardless of
file truncation).
Combined with the memory-safety hardening in 1185db8 (which
prevents savestate-loaded indices from going out-of-bounds and
producing unpredictable behaviour), the core now offers genuine
frame-deterministic replay across runs, builds, and host endian.
