Refactor and fix sect packing's bank iteration

Moving both pieces of related code together, as it were.
This also ends up changing the behaviour of scrambling,
where sections could “leak” out of the scrambling's bank pool
and resume being placed normally.

We have discussed it offline, and decided that this was a bug.
The test suite has been updated accordingly, which even gives
the occasion to move this test from a special case to the main
generic loop!

src/link/assign.cpp

@@ -23,11 +23,6 @@
 #include "link/symbol.hpp"
 #include "link/warning.hpp"
 
-struct MemoryLocation {
-	uint16_t address;
-	uint32_t bank;
-};
-
 struct FreeSpace {
 	uint16_t address;
 	uint16_t size;
@@ -80,6 +75,71 @@ struct Scrambling {
 };
 static Scrambling scrambling;
 
+struct MemoryLocation {
+	uint16_t address;
+	uint32_t bank;
+
+	static MemoryLocation initFor(Section const &section) {
+		MemoryLocation location;
+
+		if (section.isAddressFixed) { // This will never change.
+			location.address = section.org;
+		}
+
+		if (section.isBankFixed) {
+			location.bank = section.bank;
+		} else {
+			location.bank = section.typeInfo().firstBank;
+
+			if (auto info = scrambling.getInfoFor(section.type);
+			    info.has_value() && info->maxOfs != 0) { // If scrambling is enabled...
+				// ...then we will begin the search from a different offset for each section.
+				// Go to the next offset (backwards), wrapping around. (Thus, no underflow!)
+				info->curOfs = (info->curOfs != 0 ? info->curOfs : info->maxOfs) - 1;
+				location.bank += info->curOfs;
+			}
+		}
+
+		return location;
+	}
+
+	// Try again in the next bank, if one is available.
+	[[nodiscard("This returns whether iteration can be continued")]]
+	bool goToNextApplicableBankFor(Section const &section) {
+		assume(bank >= section.typeInfo().firstBank);
+		assume(bank <= section.typeInfo().lastBank);
+
+		if (section.isBankFixed) {
+			// We have already tried the only possible bank.
+			return false;
+		}
+
+		// Try scrambled banks in descending order until no bank in the scrambled range is
+		// available.
+		if (auto info = scrambling.getInfoFor(section.type);
+		    info.has_value() && info->maxOfs != 0) {
+			// All floating sections within a scrambled region should be
+			// within the scrambled bank pool.
+			assume(bank < info->maxOfs + section.typeInfo().firstBank);
+
+			uint16_t ofsWithinPool = bank - section.typeInfo().firstBank;
+			// Go to the next bank (backwards), wrapping around. (Thus, no overflow!)
+			ofsWithinPool = (ofsWithinPool != 0 ? ofsWithinPool : info->maxOfs) - 1;
+
+			bank = ofsWithinPool + section.typeInfo().firstBank;
+			// Keep iterating unless we have wrapped back around to the start offset.
+			return ofsWithinPool != info->curOfs;
+		}
+
+		// Otherwise, try in ascending order.
+		if (bank == section.typeInfo().lastBank) {
+			return false;
+		}
+		++bank;
+		return true;
+	}
+};
+
 // Checks whether a given location is suitable for placing a given section
 // This checks not only that the location has enough room for the section, but
 // also that the constraints (alignment...) are respected.
@@ -101,31 +161,12 @@ static bool isLocationSuitable(
 	return location.address + section.size <= freeSpace.address + freeSpace.size;
 }
 
-static MemoryLocation getStartLocation(Section const &section) {
-	MemoryLocation location;
-
-	// Determine which bank we should start searching in
-	if (section.isBankFixed) {
-		location.bank = section.bank;
-	} else {
-		location.bank = section.typeInfo().firstBank;
-
-		if (auto info = scrambling.getInfoFor(section.type);
-		    info.has_value() && info->maxOfs != 0) {
-			info->curOfs = (info->curOfs != 0 ? info->curOfs : info->maxOfs) - 1;
-			location.bank += info->curOfs;
-		}
-	}
-
-	return location;
-}
-
 // Returns a suitable free space index into `memory[section->type]` at which to place the given
 // section, or `std::nullopt` if none was found.
 static std::optional<size_t> getPlacement(Section const &section, MemoryLocation &location) {
 	SectionTypeInfo const &typeInfo = section.typeInfo();
 
-	for (;;) {
+	do {
 		if (location.bank < typeInfo.firstBank
 		    || location.bank >= memory[section.type].size() + typeInfo.firstBank) {
 			fatal(
@@ -191,29 +232,9 @@ static std::optional<size_t> getPlacement(Section const &section, MemoryLocation
 			// Try again with the new location/free space combo
 		}
 
-		// Try again in the next bank, if one is available.
-		// Try scrambled banks in descending order until no bank in the scrambled range is
-		// available. Otherwise, try in ascending order.
-		if (section.isBankFixed) {
-			return std::nullopt;
-		} else if (
-		    auto info = scrambling.getInfoFor(section.type); info.has_value() && info->maxOfs != 0
-		) {
-			if (location.bank > typeInfo.firstBank) {
-				--location.bank;
-			} else if (info->maxOfs < typeInfo.lastBank) {
-				location.bank = info->maxOfs + 1;
-			} else {
-				return std::nullopt;
-			}
-		} else if (location.bank < typeInfo.lastBank) {
-			++location.bank;
-		} else {
-			return std::nullopt;
-		}
-
 		// Try again in the next iteration.
-	}
+	} while (location.goToNextApplicableBankFor(section));
+	return std::nullopt;
 }
 
 static std::string getSectionDescription(Section const &section) {
@@ -245,7 +266,15 @@ static std::string getSectionDescription(Section const &section) {
 		} else {
 			description = description + "anywhere";
 		}
+
+		if (auto info = scrambling.getInfoFor(section.type);
+		    info.has_value() && info->maxOfs != 0) { // Only mention scrambling if it is enabled.
+			char size[6];
+			snprintf(size, sizeof(size), "%" PRIu16, info->maxOfs);
+			description = description + " within the " + size + " scrambled banks";
+		}
 	}
+
 	return description;
 }
 
@@ -263,16 +292,13 @@ static void assignSection(Section &section, MemoryLocation const &location) {
 // Places a section in a suitable location, or error out if it fails to.
 // Due to the implemented algorithm, this should be called with sections of decreasing size!
 static void placeSection(Section &section) {
-	SectionTypeInfo const &typeInfo = section.typeInfo();
+	MemoryLocation location = MemoryLocation::initFor(section);
 
 	// Specially handle 0-byte SECTIONs, as they can't overlap anything
 	if (section.size == 0) {
-		// Unless the SECTION has a fixed address or non-zero alignment, the starting
+		// Unless the SECTION has a fixed address or non-zero alignment offset, the starting
 		// address is fine for any alignment, as checked in `sect_DoSanityChecks`.
-		MemoryLocation location = {
-		    .address = section.isAddressFixed ? section.org : typeInfo.startAddr,
-		    .bank = section.isBankFixed ? section.bank : typeInfo.firstBank,
-		};
+		location.address = section.isAddressFixed ? section.org : section.typeInfo().startAddr;
 		if (section.isAlignFixed && !section.isAddressFixed) {
 			if (uint16_t offset = (location.address - section.alignOfs) & section.alignMask;
 			    offset != 0) {
@@ -285,9 +311,9 @@ static void placeSection(Section &section) {
 
 	// Place section using first-fit decreasing algorithm
 	// https://en.wikipedia.org/wiki/Bin_packing_problem#First-fit_algorithm
-	MemoryLocation location = getStartLocation(section);
 	if (std::optional<size_t> spaceIdx = getPlacement(section, location); spaceIdx) {
-		std::deque<FreeSpace> &bankMem = memory[section.type][location.bank - typeInfo.firstBank];
+		std::deque<FreeSpace> &bankMem =
+		    memory[section.type][location.bank - section.typeInfo().firstBank];
 		FreeSpace &freeSpace = bankMem[*spaceIdx];
 
 		assignSection(section, location);
@@ -324,9 +350,8 @@ static void placeSection(Section &section) {
 	if (!section.isBankFixed || !section.isAddressFixed) {
 		// If a section failed to go to several places, nothing we can report
 		fatal("Unable to place %s", getSectionDescription(section).c_str());
-	} else if (
-	    uint16_t onePastEnd = typeInfo.endAddr() + 1; section.org + section.size > onePastEnd
-	) {
+	} else if (uint16_t onePastEnd = section.typeInfo().endAddr() + 1;
+	           section.org + section.size > onePastEnd) {
 		// If the section just can't fit the bank, report that
 		fatal(
 		    "Unable to place %s: section runs past end of region ($%04x > $%04x)",

test/link/scramble-overflow.asm

@@ -0,0 +1,15 @@
+DEF N equ 6
+
+SECTION "fixed", ROMX, BANK[3]
+ds $1000, $ff
+
+FOR i, N
+	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/scramble-overflow.flags

@@ -0,0 +1 @@
+-S romx=3,wramx=4,sram=4

test/link/scramble-overflow.out

@@ -0,0 +1,2 @@
+FATAL: Unable to place "floating0" (ROMX section) anywhere within the 3 scrambled banks
+Linking aborted with 1 error

test/link/scramble-specs.asm

@@ -0,0 +1,38 @@
+; XXX: This test is brittle, since it relies on more than just the scrambling algorithm.
+; For example, if the order in which sections are processed changes,
+; then they will also be scrambled differently, and this test will fail.
+; As long as the actual values are coherent, feel free to change the assertions.
+
+DEF N equ 6
+
+assert BANK(xLabel5) == 3
+assert BANK(xLabel4) == 2
+assert BANK(xLabel3) == 1
+assert BANK(xLabel2) == 3
+assert BANK(xLabel1) == 2
+assert BANK(xLabel0) == 1
+
+assert BANK(sLabel5) == 3
+assert BANK(sLabel4) == 2
+assert BANK(sLabel3) == 1
+assert BANK(sLabel2) == 0
+assert BANK(sLabel1) == 3
+assert BANK(sLabel0) == 2
+
+assert BANK(wLabel5) == 4
+assert BANK(wLabel4) == 3
+assert BANK(wLabel3) == 2
+assert BANK(wLabel2) == 1
+assert BANK(wLabel1) == 4
+assert BANK(wLabel0) == 3
+
+FOR i, N
+	SECTION "floating{d:i}", ROMX
+	xLabel{d:i}:: ds $2000, i
+
+	SECTION "sram{d:i}", SRAM
+	sLabel{d:i}:: dw
+
+	SECTION "wram{d:i}", WRAMX
+	wLabel{d:i}:: dw
+ENDR

test/link/scramble-specs.flags

@@ -0,0 +1 @@
+-S romx=3,wramx=4,sram=4

test/link/scramble-specs/a.asm

@@ -1,25 +0,0 @@
-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

@@ -330,16 +330,6 @@ rgblinkQuiet -o "$gbtemp" -S "romx := 4" "$otemp" 2>"$outtemp"
 tryDiff "$test"/out.err "$outtemp"
 evaluateTest
 
-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 trim its output with 'dd' because it needs to verify the correct output size
-tryCmp "$test"/out.gb "$gbtemp"
-evaluateTest
-
 test="script-ds"
 startTest
 "$RGBASM" -o "$otemp" "$test"/a.asm