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

@@ -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

src/link/assign.cpp

@@ -70,31 +70,34 @@ static bool isLocationSuitable(
 
 static MemoryLocation getStartLocation(Section const &section) {
 	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;

test/link/scramble-romx/a.asm

@@ -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

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

test/link/test.sh

@@ -62,15 +62,6 @@ tryCmpRom () {
 	tryCmp "$1" "$otemp"
 }
 
-tryCmpRomSize () {
-	rom_size=$(printf %s $(wc -c <"$1"))
-	if [ "$rom_size" -ne "$2" ]; then
-		echo "$bold${red}${test} binary size mismatch! ${rescolors}${resbold}"
-		false
-	fi
-	(( our_rc = our_rc || $? ))
-}
-
 rgblinkQuiet () {
 	out="$(env "$RGBLINK" -Weverything -Bcollapse "$@")" || return $?
 	if [[ -n "$out" ]]; then
@@ -321,14 +312,14 @@ rgblinkQuiet -o "$gbtemp" -S "romx := 4" "$otemp" 2>"$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"