From ef709fade6be4076cf2a65b4892c16bb94bdb631 Mon Sep 17 00:00:00 2001 From: ISSOtm Date: Wed, 16 Sep 2026 21:17:58 -0400 Subject: [PATCH] Refactor and fix sect packing's bank iteration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 141 +++++++++++------- test/link/scramble-overflow.asm | 15 ++ test/link/scramble-overflow.flags | 1 + test/link/scramble-overflow.out | 2 + test/link/scramble-specs.asm | 38 +++++ test/link/scramble-specs.flags | 1 + .../out.err => scramble-specs.out} | 0 test/link/scramble-specs/a.asm | 25 ---- test/link/scramble-specs/out.gb | Bin 81920 -> 0 bytes test/link/test.sh | 10 -- 10 files changed, 140 insertions(+), 93 deletions(-) create mode 100644 test/link/scramble-overflow.asm create mode 100644 test/link/scramble-overflow.flags create mode 100644 test/link/scramble-overflow.out create mode 100644 test/link/scramble-specs.asm create mode 100644 test/link/scramble-specs.flags rename test/link/{scramble-specs/out.err => scramble-specs.out} (100%) delete mode 100644 test/link/scramble-specs/a.asm delete mode 100644 test/link/scramble-specs/out.gb diff --git a/src/link/assign.cpp b/src/link/assign.cpp index 683d64bf..dd45ae45 100644 --- a/src/link/assign.cpp +++ b/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 §ion) { + 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 §ion) { + 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 §ion) { - 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 getPlacement(Section const §ion, 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 getPlacement(Section const §ion, 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 §ion) { @@ -245,7 +266,15 @@ static std::string getSectionDescription(Section const §ion) { } 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 §ion, 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 §ion) { - 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 §ion) { // 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 spaceIdx = getPlacement(section, location); spaceIdx) { - std::deque &bankMem = memory[section.type][location.bank - typeInfo.firstBank]; + std::deque &bankMem = + memory[section.type][location.bank - section.typeInfo().firstBank]; FreeSpace &freeSpace = bankMem[*spaceIdx]; assignSection(section, location); @@ -324,9 +350,8 @@ static void placeSection(Section §ion) { 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)", diff --git a/test/link/scramble-overflow.asm b/test/link/scramble-overflow.asm new file mode 100644 index 00000000..af066887 --- /dev/null +++ b/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 diff --git a/test/link/scramble-overflow.flags b/test/link/scramble-overflow.flags new file mode 100644 index 00000000..8beff92c --- /dev/null +++ b/test/link/scramble-overflow.flags @@ -0,0 +1 @@ +-S romx=3,wramx=4,sram=4 diff --git a/test/link/scramble-overflow.out b/test/link/scramble-overflow.out new file mode 100644 index 00000000..8a7c6413 --- /dev/null +++ b/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 diff --git a/test/link/scramble-specs.asm b/test/link/scramble-specs.asm new file mode 100644 index 00000000..412bcaab --- /dev/null +++ b/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 diff --git a/test/link/scramble-specs.flags b/test/link/scramble-specs.flags new file mode 100644 index 00000000..8beff92c --- /dev/null +++ b/test/link/scramble-specs.flags @@ -0,0 +1 @@ +-S romx=3,wramx=4,sram=4 diff --git a/test/link/scramble-specs/out.err b/test/link/scramble-specs.out similarity index 100% rename from test/link/scramble-specs/out.err rename to test/link/scramble-specs.out diff --git a/test/link/scramble-specs/a.asm b/test/link/scramble-specs/a.asm deleted file mode 100644 index 4fb38895..00000000 --- a/test/link/scramble-specs/a.asm +++ /dev/null @@ -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 diff --git a/test/link/scramble-specs/out.gb b/test/link/scramble-specs/out.gb deleted file mode 100644 index ef9d918631274466a083ac590bffda20b5e7d57c..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 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-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