Massively simplified, by not trying to shoehorn differently-behaving
cases into the same one loop!
This may have introduced bugs (the test suite has caught three different
oversights, two of which via external projects, in fact!), but this also
makes the logic clearer and more streamlined, so that we are also less
likely to have any latent or future bugs.
In particular, previously, the first iteration of the loop could attempt
placement at an address not matching the section's constraints,
which made advancing to the next target address unnecessarily complicated
(https://github.com/gbdev/rgbds/pull/2064#discussion_r3985466596),
among other weirdness. The code ended up being defensive, and thus the
overall logic was murky.
I'm also expecting that this should provide a performance improvement due
to being essentially a form of loop-invariant code motion (and very likely
one a compiler couldn't have performed automatically), though I haven't
measured.
Turns out the checks could fail spuriously if the starting alignment is non-zero,
e.g. `align 8,$C0` in HRAM. (Fixes #2113.)
This was tripped up by a test added to exercise the linker's section placement algorithm
on a similar aligment-related issue. :D
Note that this may be caused by the code in question pre-dating alignment offsets,
and we never checked. That said, these checks only work due to the regions' starting
addresses being no more aligned than their size (..if that makes sense?), so allowing
custom memory regions (#524) could violate that assumption and make those checks incorrect.
Assembling is never *supposed* to fail, but in this case I ended up adding
a test case that triggered a bug in the assembler, and that caused the rest
of the test to become garbled. This is thus more robust.
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!
RGBASM computes these with unsigned arithmetic and casts back, since
signed overflow is UB in C++, but RGBLINK's RPN evaluator used `int32_t`
directly, so `src/link/patch.cpp` tripped UBSan on lines 117, 121, and
124. Share the three operators through `opmath.cpp` so both evaluators
stay in step.
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.
RGBASM tests now use `rgblink -x` instead of `dd` to get trimmed
test output. RGBLINK tests cannot do so because some of them
(e.g. bank-numbers.asm and sizeof-startof.asm) rely on ROMX
sections above 1, and `-x` implies `-t` which breaks that.
Rangi
ISSOtm
Rangi
Max Freedom Pollard