Fix linking tiny overlay files (#755)

* Fix compatibility of rgblink -O and -t

The -t "tiny mode" option makes ROM0 cover 0x8000 bytes,
not 0x4000. The -O "overlay" option fills areas uncovered
by sections with data from an overlay file. These needed
to cooperate so that the calculated uncovered overlay size
does not exceed the actual size of the ROM.

Fixes #754

* Print link test names like asm tests do

* Make the three test.sh scripts more similar

src/link/output.c

@@ -17,7 +17,11 @@
 
 #include "extern/err.h"
 
-FILE * outputFile;
+#include "linkdefs.h"
+
+#define BANK_SIZE 0x4000
+
+FILE *outputFile;
 FILE *overlayFile;
 FILE *symFile;
 FILE *mapFile;
@@ -103,43 +107,63 @@ struct Section const *out_OverlappingSection(struct Section const *section)
 
 /**
  * Performs sanity checks on the overlay file.
+ * @return The number of ROM banks in the overlay file
  */
-static void checkOverlay(void)
+static uint32_t checkOverlaySize(void)
 {
 	if (!overlayFile)
-		return;
+		return 0;
 
 	if (fseek(overlayFile, 0, SEEK_END) != 0) {
 		warnx("Overlay file is not seekable, cannot check if properly formed");
-		return;
+		return 0;
 	}
 
 	long overlaySize = ftell(overlayFile);
 
-	if (overlaySize % 0x4000)
-		errx(1, "Overlay file must have a size multiple of 0x4000");
-
 	/* Reset back to beginning */
 	fseek(overlayFile, 0, SEEK_SET);
 
-	uint32_t nbOverlayBanks = overlaySize / 0x4000 - 1;
+	if (overlaySize % BANK_SIZE)
+		errx(1, "Overlay file must have a size multiple of 0x4000");
 
-	if (nbOverlayBanks < 1)
+	uint32_t nbOverlayBanks = overlaySize / BANK_SIZE;
+
+	if (is32kMode && nbOverlayBanks != 2)
+		errx(1, "Overlay must be exactly 0x8000 bytes large");
+
+	if (nbOverlayBanks < 2)
 		errx(1, "Overlay must be at least 0x8000 bytes large");
 
-	if (nbOverlayBanks > sections[SECTTYPE_ROMX].nbBanks) {
+	return nbOverlayBanks;
+}
+
+/**
+ * Expand sections[SECTTYPE_ROMX].banks to cover all the overlay banks.
+ * This ensures that writeROM will output each bank, even if some are not
+ * covered by any sections.
+ * @param nbOverlayBanks The number of banks in the overlay file
+ */
+static void coverOverlayBanks(uint32_t nbOverlayBanks)
+{
+	/* 2 if is32kMode, 1 otherwise */
+	uint32_t nbRom0Banks = maxsize[SECTTYPE_ROM0] / BANK_SIZE;
+	/* Discount ROM0 banks to avoid outputting too much */
+	uint32_t nbUncoveredBanks = nbOverlayBanks - nbRom0Banks > sections[SECTTYPE_ROMX].nbBanks
+				    ? nbOverlayBanks - nbRom0Banks
+				    : 0;
+
+	if (nbUncoveredBanks > sections[SECTTYPE_ROMX].nbBanks) {
 		sections[SECTTYPE_ROMX].banks =
 			realloc(sections[SECTTYPE_ROMX].banks,
-				sizeof(*sections[SECTTYPE_ROMX].banks) *
-					nbOverlayBanks);
+				sizeof(*sections[SECTTYPE_ROMX].banks) * nbUncoveredBanks);
 		if (!sections[SECTTYPE_ROMX].banks)
 			err(1, "Failed to realloc banks for overlay");
-		for (uint32_t i = sections[SECTTYPE_ROMX].nbBanks;
-		     i < nbOverlayBanks; i++) {
+		for (uint32_t i = sections[SECTTYPE_ROMX].nbBanks; i < nbUncoveredBanks; i++) {
 			sections[SECTTYPE_ROMX].banks[i].sections = NULL;
 			sections[SECTTYPE_ROMX].banks[i].zeroLenSections = NULL;
 		}
-		sections[SECTTYPE_ROMX].nbBanks = nbOverlayBanks;
+		sections[SECTTYPE_ROMX].nbBanks = nbUncoveredBanks;
 	}
 }
 
@@ -195,16 +219,19 @@ static void writeROM(void)
 	outputFile = openFile(outputFileName, "wb");
 	overlayFile = openFile(overlayFileName, "rb");
 
-	checkOverlay();
+	uint32_t nbOverlayBanks = checkOverlaySize();
+
+	if (nbOverlayBanks > 0)
+		coverOverlayBanks(nbOverlayBanks);
 
 	if (outputFile) {
 		if (sections[SECTTYPE_ROM0].nbBanks > 0)
 			writeBank(sections[SECTTYPE_ROM0].banks[0].sections,
-				  0x0000, 0x4000);
+				  startaddr[SECTTYPE_ROM0], maxsize[SECTTYPE_ROM0]);
 
 		for (uint32_t i = 0 ; i < sections[SECTTYPE_ROMX].nbBanks; i++)
 			writeBank(sections[SECTTYPE_ROMX].banks[i].sections,
-				  0x4000, 0x4000);
+				  startaddr[SECTTYPE_ROMX], maxsize[SECTTYPE_ROMX]);
 	}
 
 	closeFile(outputFile);

src/linkdefs.c

@@ -13,11 +13,11 @@ uint16_t startaddr[] = {
 };
 
 uint16_t maxsize[] = {
-	[SECTTYPE_ROM0]  = 0x8000,
+	[SECTTYPE_ROM0]  = 0x8000, // patched to 0x4000 if !is32kMode
 	[SECTTYPE_ROMX]  = 0x4000,
 	[SECTTYPE_VRAM]  = 0x2000,
 	[SECTTYPE_SRAM]  = 0x2000,
-	[SECTTYPE_WRAM0] = 0x2000,
+	[SECTTYPE_WRAM0] = 0x2000, // patched to 0x1000 if !isWRA0Mode
 	[SECTTYPE_WRAMX] = 0x1000,
 	[SECTTYPE_OAM]   = 0x00A0,
 	[SECTTYPE_HRAM]  = 0x007F

test/asm/test.sh

@@ -17,12 +17,23 @@ red="$(tput setaf 1)"
 green="$(tput setaf 2)"
 orange="$(tput setaf 3)"
 rescolors="$(tput op)"
+
+RGBASM=../../rgbasm
+RGBLINK=../../rgblink
+
 tryDiff () {
-	diff -u --strip-trailing-cr $1 $2 || (echo "${bold}${red}${i%.asm}${variant}.$3 mismatch!${rescolors}${resbold}"; false)
+	if ! diff -u --strip-trailing-cr "$1" "$2"; then
+		echo "${bold}${red}${i%.asm}${variant}.$3 mismatch!${rescolors}${resbold}"
+		false
+	fi
 }
 
 tryCmp () {
-	cmp $1 $2 || (../../contrib/gbdiff.bash $1 $2; echo "${bold}${red}${i%.asm}${variant}.out.bin mismatch!${rescolors}${resbold}"; false)
+	if ! cmp "$1" "$2"; then
+		../../contrib/gbdiff.bash "$1" "$2"
+		echo "${bold}${red}${i%.asm}${variant}.out.bin mismatch!${rescolors}${resbold}"
+		false
+	fi
 }
 
 # Add the version constants test, outputting the closest tag to the HEAD
@@ -49,7 +60,7 @@ fi
 
 # Check whether to use '.simple.err' files if they exist
 # (rgbasm with pre-3.0 Bison just reports "syntax error")
-../../rgbasm -Weverything -o $o syntax-error.asm > $output 2> $errput
+$RGBASM -Weverything -o $o syntax-error.asm > $output 2> $errput
 cmp syntax-error.err $errput > /dev/null 2> /dev/null
 simple_error=$?
 if [ "$simple_error" -eq 1 ]; then
@@ -64,7 +75,7 @@ for i in *.asm; do
 			desired_errname=${i%.asm}.simple.err
 		fi
 		if [ -z "$variant" ]; then
-			../../rgbasm -Weverything -o $o $i > $output 2> $errput
+			$RGBASM -Weverything -o $o $i > $output 2> $errput
 			desired_output=${i%.asm}.out
 			desired_errput=$desired_errname
 		else
@@ -78,7 +89,7 @@ for i in *.asm; do
 			# stdin redirection makes the input an unseekable pipe - a scenario
 			# that's harder to deal with and was broken when the feature was
 			# first implemented.
-			cat $i | ../../rgbasm -Weverything -o $o - > $output 2> $errput
+			cat $i | $RGBASM -Weverything -o $o - > $output 2> $errput
 
 			# Use two otherwise unused files for temp storage
 			desired_output=$input
@@ -97,7 +108,7 @@ for i in *.asm; do
 
 		bin=${i%.asm}.out.bin
 		if [ -f $bin ]; then
-			../../rgblink -o $gb $o
+			$RGBLINK -o $gb $o
 			dd if=$gb count=1 bs=$(printf %s $(wc -c < $bin)) > $output 2>/dev/null
 			tryCmp $bin $output
 			our_rc=$(($? || $our_rc))

test/fix/test.sh

@@ -16,6 +16,8 @@ red="$(tput setaf 1)"
 green="$(tput setaf 2)"
 rescolors="$(tput op)"
 
+RGBFIX=./rgbfix
+
 tryDiff () {
 	if ! diff -u --strip-trailing-cr "$1" "$2"; then
 		echo "${bold}${red}${3:-$1} mismatch!${rescolors}${resbold}"
@@ -41,7 +43,7 @@ runTest () {
 		fi
 		if [[ -z "$variant" ]]; then
 			cp "$2/$1.bin" out.gb
-			if [[ -n "$(eval ./rgbfix $flags out.gb '2>out.err')" ]]; then
+			if [[ -n "$(eval $RGBFIX $flags out.gb '2>out.err')" ]]; then
 				echo "${bold}${red}Fixing $1 in-place shouldn't output anything on stdout!${rescolors}${resbold}"
 				our_rc=1
 			fi
@@ -50,14 +52,14 @@ runTest () {
 			# Stop! This is not a Useless Use Of Cat. Using cat instead of
 			# stdin redirection makes the input an unseekable pipe - a scenario
 			# that's harder to deal with.
-			cat "$2/$1.bin" | eval ./rgbfix "$flags" '>out.gb' '2>out.err'
+			cat "$2/$1.bin" | eval $RGBFIX "$flags" '>out.gb' '2>out.err'
 			subst='<stdin>'
 		fi
 
-		sed "s/$subst/<filename>/g" "out.err" | tryDiff "$2/$1.err" -  "$1.err${variant}"
+		sed "s/$subst/<filename>/g" "out.err" | tryDiff "$2/$1.err" - "$1.err${variant}"
 		our_rc=$(($? || $our_rc))
 		if [[ -r "$2/$1.gb" ]]; then
-			tryCmp "$2/$1.gb" "out.gb"  "$1.gb${variant}"
+			tryCmp "$2/$1.gb" "out.gb" "$1.gb${variant}"
 			our_rc=$(($? || $our_rc))
 		fi
 
@@ -91,9 +93,9 @@ printf "\rDone! \n"
 # TODO: check MBC names
 
 # Check that RGBFIX errors out when inputting a non-existent file...
-./rgbfix noexist 2>out.err
+$RGBFIX noexist 2>out.err
 rc=$(($rc || $? != 1))
-tryDiff "$src/noexist.err" out.err  noexist.err
+tryDiff "$src/noexist.err" out.err noexist.err
 rc=$(($rc || $?))
 
 exit $rc

test/link/overlay/a.asm

@@ -0,0 +1,2 @@
+SECTION "0", ROM0[0]
+DS $8000

test/link/test.sh

@@ -1,37 +1,44 @@
 #!/bin/bash
+
 export LC_ALL=C
 set -o pipefail
 
-otemp=$(mktemp)
-gbtemp=$(mktemp)
-gbtemp2=$(mktemp)
-outtemp=$(mktemp)
+otemp="$(mktemp)"
+gbtemp="$(mktemp)"
+gbtemp2="$(mktemp)"
+outtemp="$(mktemp)"
 rc=0
 
 trap "rm -f '$otemp' '$gbtemp' '$gbtemp2' '$outtemp'" EXIT
 
-bold=$(tput bold)
-resbold=$(tput sgr0)
-red=$(tput setaf 1)
-rescolors=$(tput op)
+bold="$(tput bold)"
+resbold="$(tput sgr0)"
+red="$(tput setaf 1)"
+green="$(tput setaf 2)"
+rescolors="$(tput op)"
+
+RGBASM=../../rgbasm
+RGBLINK=../../rgblink
+
+startTest () {
+	echo "$bold$green${i%.asm}...$rescolors$resbold"
+}
 
 tryDiff () {
-	if ! diff -u --strip-trailing-cr $1 $2; then
+	if ! diff -u --strip-trailing-cr "$1" "$2"; then
 		echo "${bold}${red}${i%.asm}.out mismatch!${rescolors}${resbold}"
 		false
 	fi
 }
 
 tryCmp () {
-	if ! cmp $1 $2; then
-		../../contrib/gbdiff.bash $1 $2
+	if ! cmp "$1" "$2"; then
+		../../contrib/gbdiff.bash "$1" "$2"
 		echo "${bold}${red}${i%.asm}.out.bin mismatch!${rescolors}${resbold}"
 		false
 	fi
 }
 
-RGBASM=../../rgbasm
-RGBLINK=../../rgblink
 rgblink() {
 	out="$(env $RGBLINK "$@")" || return $?
 	if [[ -n "$out" ]]; then
@@ -41,6 +48,7 @@ rgblink() {
 }
 
 for i in *.asm; do
+	startTest
 	$RGBASM -o $otemp $i
 
 	# Some tests have variants depending on flags
@@ -91,13 +99,16 @@ done
 
 # These tests do their own thing
 
+i="bank-const.asm"
+startTest
 $RGBASM -o $otemp bank-const/a.asm
 $RGBASM -o $gbtemp2 bank-const/b.asm
 rgblink -o $gbtemp $gbtemp2 $otemp > $outtemp 2>&1
-i="bank-const.asm" tryDiff bank-const/err.out $outtemp
+tryDiff bank-const/out.err $outtemp
 rc=$(($? || $rc))
 
 for i in fragment-align/*; do
+	startTest
 	$RGBASM -o $otemp $i/a.asm
 	$RGBASM -o $gbtemp2 $i/b.asm
 	rgblink -o $gbtemp $otemp $gbtemp2 2>$outtemp
@@ -110,27 +121,46 @@ for i in fragment-align/*; do
 	fi
 done
 
+i="high-low.asm"
+startTest
 $RGBASM -o $otemp high-low/a.asm
 rgblink -o $gbtemp $otemp
 $RGBASM -o $otemp high-low/b.asm
 rgblink -o $gbtemp2 $otemp
-i="high-low.asm" tryCmp $gbtemp $gbtemp2
+tryCmp $gbtemp $gbtemp2
 rc=$(($? || $rc))
 
+i="overlay.asm"
+startTest
+$RGBASM -o $otemp overlay/a.asm
+rgblink -o $gbtemp -t -O overlay/overlay.gb $otemp > $outtemp 2>&1
+# This test does not trim its output with 'dd' because it needs to verify the correct output size
+tryDiff overlay/out.err $outtemp
+rc=$(($? || $rc))
+tryCmp overlay/out.gb $gbtemp
+rc=$(($? || $rc))
+
+i="section-union/good.asm"
+startTest
 $RGBASM -o $otemp section-union/good/a.asm
 $RGBASM -o $gbtemp2 section-union/good/b.asm
 rgblink -o $gbtemp -l section-union/good/script.link $otemp $gbtemp2
 dd if=$gbtemp count=1 bs=$(printf %s $(wc -c < section-union/good/ref.out.bin)) > $otemp 2>/dev/null
-i="section-union/good.asm" tryCmp section-union/good/ref.out.bin $otemp
+tryCmp section-union/good/ref.out.bin $otemp
 rc=$(($? || $rc))
+
+i="section-union/fragments.asm"
+startTest
 $RGBASM -o $otemp section-union/fragments/a.asm
 $RGBASM -o $gbtemp2 section-union/fragments/b.asm
 rgblink -o $gbtemp $otemp $gbtemp2
 dd if=$gbtemp count=1 bs=$(printf %s $(wc -c < section-union/fragments/ref.out.bin)) > $otemp 2>/dev/null
-i="section-union/fragments.asm" tryCmp section-union/fragments/ref.out.bin $otemp
+tryCmp section-union/fragments/ref.out.bin $otemp
 rc=$(($? || $rc))
+
 for i in section-union/*.asm; do
-	$RGBASM -o $otemp   $i
+	startTest
+	$RGBASM -o $otemp $i
 	$RGBASM -o $gbtemp2 $i -DSECOND
 	if rgblink $otemp $gbtemp2 2>$outtemp; then
 		echo -e "${bold}${red}$i didn't fail to link!${rescolors}${resbold}"