From 5869e0dc6082254d414a8120f6657ccde4a5651b Mon Sep 17 00:00:00 2001 From: Rangi Date: Mon, 13 Jul 2026 12:24:14 -0400 Subject: [PATCH] Fix RGBLINK scrambling algorithm to work for SRAM Since SRAM sections start from 0, we had been using a signed `int8_t curScrambleSRAM` counter to allow 0 as a scrambled bank, but this actually just placed all floating SRAM sections in bank 0. --- man/rgblink.1 | 17 +++++--- src/link/assign.cpp | 37 ++++++++++-------- test/link/scramble-romx/a.asm | 18 --------- test/link/scramble-specs/a.asm | 25 ++++++++++++ .../{scramble-romx => scramble-specs}/out.err | 0 test/link/scramble-specs/out.gb | Bin 0 -> 81920 bytes test/link/test.sh | 15 ++----- 7 files changed, 59 insertions(+), 53 deletions(-) delete mode 100644 test/link/scramble-romx/a.asm create mode 100644 test/link/scramble-specs/a.asm rename test/link/{scramble-romx => scramble-specs}/out.err (100%) create mode 100644 test/link/scramble-specs/out.gb diff --git a/man/rgblink.1 b/man/rgblink.1 index a3df1fd3..dfc2e212 100644 --- a/man/rgblink.1 +++ b/man/rgblink.1 @@ -257,22 +257,27 @@ an at-file, it only disables option processing within that at-file, and processi The default section placement algorithm tries to place sections into as few banks as possible. (It turns out that section placement is an NP-complete problem known as "bin packing", so .Nm -does not attempt to find the optimal solution, but instead uses a "first-fit" heuristic to find a good one in a reasonable amount of time. -There are no guarantees about where this algorithm will place sections, apart from the bank, address, and alignment constraints manually specified for the sections.) +does not attempt to find the optimal solution, but instead uses a "first-fit" heuristic to find a good one in a reasonable amount of time.) +There are no guarantees about where this algorithm will place sections, apart from the bank, address, and alignment constraints manually specified for the sections. .Pp .Dq Scrambling instead places sections into a given pool of banks, trying to minimize the number of sections sharing a given bank. This is useful to catch broken bank assumptions, such as expecting two different sections to land in the same bank (that is not guaranteed unless both are manually assigned the same bank number). +There are still no guarantees about where this algorithm will place sections, apart from the bank pool size and any manually specified section constraints. .Pp A scrambling spec is a comma-separated list of region specs. A trailing comma is allowed, as well as whitespace between all specs and their components. Each region spec has the following form: .D1 Ar region Ns Op = Ns Ar size .Ar region +specifies the section type, and +.Ar size +specifies the number of banks in the pool for that section type. +.Ar region must be one of the following (case-insensitive), while .Ar size must be a positive decimal integer between 1 and the corresponding maximum. -Certain regions allow omitting the size, in which case it defaults to its max value. +Certain regions allow omitting the size, in which case it defaults to its maximum value. .Bl -column "Region name" "Max value" "Size optional" Region name Ta Max size Ta Size optional .Cm romx Ta 65535 Ta \&No @@ -285,14 +290,14 @@ A of 0 disables scrambling for that region. .Pp For example, -.Ql romx=64,wramx=4 +.Ql romx=64,wramx,sram=4 will scramble .Ic ROMX sections among ROM banks 1 to 64, .Ic WRAMX -sections among RAM banks 1 to 4, and will not scramble +sections among RAM banks 1 to 7, and .Ic SRAM -sections. +sections among SRAM banks 0 to 3. .Pp Later region specs override earlier ones; for example, .Ql romx=42, Romx=0 diff --git a/src/link/assign.cpp b/src/link/assign.cpp index f1e61969..334960bd 100644 --- a/src/link/assign.cpp +++ b/src/link/assign.cpp @@ -70,31 +70,34 @@ static bool isLocationSuitable( static MemoryLocation getStartLocation(Section const §ion) { static uint16_t curScrambleROM = 0; - static uint8_t curScrambleWRAM = 0; - static int8_t curScrambleSRAM = 0; + static uint16_t curScrambleWRAM = 0; + static uint16_t curScrambleSRAM = 0; MemoryLocation location; // Determine which bank we should start searching in if (section.isBankFixed) { location.bank = section.bank; - } else if (options.scrambleROMX && section.type == SECTTYPE_ROMX) { - if (curScrambleROM < 1) { - curScrambleROM = options.scrambleROMX; - } - location.bank = curScrambleROM--; - } else if (options.scrambleWRAMX && section.type == SECTTYPE_WRAMX) { - if (curScrambleWRAM < 1) { - curScrambleWRAM = options.scrambleWRAMX; - } - location.bank = curScrambleWRAM--; - } else if (options.scrambleSRAM && section.type == SECTTYPE_SRAM) { - if (curScrambleSRAM < 0) { - curScrambleSRAM = options.scrambleSRAM; - } - location.bank = curScrambleSRAM--; } else { location.bank = sectionTypeInfo[section.type].firstBank; + + // Scramble the bank if applicable + if (options.scrambleROMX && section.type == SECTTYPE_ROMX) { + if (curScrambleROM == 0) { + curScrambleROM = options.scrambleROMX; + } + location.bank += --curScrambleROM; + } else if (options.scrambleWRAMX && section.type == SECTTYPE_WRAMX) { + if (curScrambleWRAM == 0) { + curScrambleWRAM = options.scrambleWRAMX; + } + location.bank += --curScrambleWRAM; + } else if (options.scrambleSRAM && section.type == SECTTYPE_SRAM) { + if (curScrambleSRAM == 0) { + curScrambleSRAM = options.scrambleSRAM; + } + location.bank += --curScrambleSRAM; + } } return location; diff --git a/test/link/scramble-romx/a.asm b/test/link/scramble-romx/a.asm deleted file mode 100644 index e9dbce7d..00000000 --- a/test/link/scramble-romx/a.asm +++ /dev/null @@ -1,18 +0,0 @@ -SECTION "fixed", ROMX, BANK[3] -db BANK(xLabel1), BANK(xLabel2), BANK(xLabel3), BANK(wLabel), BANK(sLabel) -ds $1000 - 5, 4 - -SECTION "floating1", ROMX -xLabel1:: ds $3000, 1 - -SECTION "floating2", ROMX -xLabel2:: ds $3000, 2 - -SECTION "floating3", ROMX -xLabel3:: ds $3000, 3 - -SECTION "wram", WRAMX -wLabel:: ds 2 - -SECTION "sram", SRAM -sLabel:: ds 2 diff --git a/test/link/scramble-specs/a.asm b/test/link/scramble-specs/a.asm new file mode 100644 index 00000000..4fb38895 --- /dev/null +++ b/test/link/scramble-specs/a.asm @@ -0,0 +1,25 @@ +DEF N = 6 + +SECTION "fixed", ROMX, BANK[3] +; XXX: We rely on these landing at certain banks, which isn't *guaranteed*... +FOR i, 1, N + 1 + db BANK(xLabel{d:i}) +ENDR +FOR i, 1, N + 1 + db BANK(wLabel{d:i}) +ENDR +FOR i, 1, N + 1 + db BANK(sLabel{d:i}) +ENDR +ds $1000 - N * 3, $ff + +FOR i, 1, N + 1 + SECTION "floating{d:i}", ROMX + xLabel{d:i}:: ds $2000, i + + SECTION "wram{d:i}", WRAMX + wLabel{d:i}:: dw + + SECTION "sram{d:i}", SRAM + sLabel{d:i}:: dw +ENDR diff --git a/test/link/scramble-romx/out.err b/test/link/scramble-specs/out.err similarity index 100% rename from test/link/scramble-romx/out.err rename to test/link/scramble-specs/out.err diff --git a/test/link/scramble-specs/out.gb b/test/link/scramble-specs/out.gb new file mode 100644 index 0000000000000000000000000000000000000000..ef9d918631274466a083ac590bffda20b5e7d57c GIT binary patch literal 81920 zcmeI&yAgme3}#9009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs0RjZZ3#_cH?mfM#W6vW31PBly zK!5-N0t5&UAVA"$outtemp" tryDiff "$test"/out.err "$outtemp" evaluateTest -test="scramble-romx" +test="scramble-specs" startTest "$RGBASM" -o "$otemp" "$test"/a.asm continueTest rgblinkQuiet -o "$gbtemp" -S "romx=3,wramx=4,sram=4" "$otemp" 2>"$outtemp" tryDiff "$test"/out.err "$outtemp" -# This test does not compare its exact output with 'tryCmpRom' because no scrambling order is guaranteed -tryCmpRomSize "$gbtemp" 65536 +# This test does not trim its output with 'dd' because it needs to verify the correct output size +tryCmp "$test"/out.gb "$gbtemp" evaluateTest test="script-include"