Added scramble flags to RGBLINK. (#921)

* Add scramble flags to RGBLINK

-S and -W will scramble ROMX and WRAMX respectively.

* Modify scramble CLI

CLI now takes a list of comma-separated values.
Added arg_error to clean up messages.

Co-authored-by: Eldred Habert <eldredhabert0@gmail.com>

* Document scrambling functionality

Co-authored-by: Eldred Habert <eldredhabert0@gmail.com>

contrib/zsh_compl/_rgblink

@@ -16,6 +16,7 @@ local args=(
 	'(-O --overlay)'{-O,--overlay}'+[Overlay sections over on top of bin file]:base overlay:_files'
 	'(-o --output)'{-o,--output}"+[Write ROM image to this file]:rom file:_files -g '*.{gb,sgb,gbc}'"
 	'(-p --pad-value)'{-p,--pad-value}'+[Set padding byte]:padding byte:'
+	'(-S --scramble)'{-s,--scramble}'+[Activate scrambling]:scramble spec'
 	'(-s --smart)'{-s,--smart}'+[!BROKEN! Perform smart linking from this symbol]:symbol name:'
 
 	'*'":object files:_files -g '*.o'"

include/link/main.h

@@ -24,6 +24,9 @@ extern char const *symFileName;
 extern char const *overlayFileName;
 extern char const *outputFileName;
 extern uint8_t padValue;
+extern uint16_t scrambleROMX;
+extern uint8_t scrambleWRAMX;
+extern uint8_t scrambleSRAM;
 extern bool is32kMode;
 extern bool beVerbose;
 extern bool isWRA0Mode;

src/link/assign.c

@@ -159,9 +159,28 @@ static bool isLocationSuitable(struct Section const *section,
 static struct FreeSpace *getPlacement(struct Section const *section,
 				      struct MemoryLocation *location)
 {
-	location->bank = section->isBankFixed
+	static uint16_t curScrambleROM = 1;
-				? section->bank
+	static uint8_t curScrambleWRAM = 1;
-				: bankranges[section->type][0];
+	static uint8_t curScrambleSRAM = 1;
+
+	// Determine which bank we should start searching in
+	if (section->isBankFixed) {
+		location->bank = section->bank;
+	} else if (scrambleROMX && section->type == SECTTYPE_ROMX) {
+		location->bank = curScrambleROM++;
+		if (curScrambleROM > scrambleROMX)
+			curScrambleROM = 1;
+	} else if (scrambleWRAMX && section->type == SECTTYPE_WRAMX) {
+		location->bank = curScrambleWRAM++;
+		if (curScrambleWRAM > scrambleWRAMX)
+			curScrambleWRAM = 1;
+	} else if (scrambleSRAM && section->type == SECTTYPE_SRAM) {
+		location->bank = curScrambleSRAM++;
+		if (curScrambleSRAM > scrambleSRAM)
+			curScrambleSRAM = 0;
+	} else {
+		location->bank = bankranges[section->type][0];
+	}
 	struct FreeSpace *space;
 
 	for (;;) {

src/link/main.c

@@ -8,6 +8,7 @@
 
 #include <assert.h>
 #include <inttypes.h>
+#include <limits.h>
 #include <stdbool.h>
 #include <stdarg.h>
 #include <stdio.h>
@@ -26,6 +27,7 @@
 
 #include "extern/err.h"
 #include "extern/getopt.h"
+#include "platform.h"
 #include "version.h"
 
 bool isDmgMode;               /* -d */
@@ -35,6 +37,10 @@ char const *symFileName;      /* -n */
 char const *overlayFileName;  /* -O */
 char const *outputFileName;   /* -o */
 uint8_t padValue;             /* -p */
+// Setting these three to 0 disables the functionality
+uint16_t scrambleROMX = 0;    /* -S */
+uint8_t scrambleWRAMX = 0;
+uint8_t scrambleSRAM = 0;
 bool is32kMode;               /* -t */
 bool beVerbose;               /* -v */
 bool isWRA0Mode;              /* -w */
@@ -100,6 +106,20 @@ void error(struct FileStackNode const *where, uint32_t lineNo, char const *fmt,
 		nbErrors++;
 }
 
+void argErr(char flag, char const *fmt, ...)
+{
+	va_list ap;
+
+	fprintf(stderr, "error: Invalid argument for option '%c': ", flag);
+	va_start(ap, fmt);
+	vfprintf(stderr, fmt, ap);
+	va_end(ap);
+	putc('\n', stderr);
+
+	if (nbErrors != UINT32_MAX)
+		nbErrors++;
+}
+
 _Noreturn void fatal(struct FileStackNode const *where, uint32_t lineNo, char const *fmt, ...)
 {
 	va_list ap;
@@ -142,7 +162,7 @@ FILE *openFile(char const *fileName, char const *mode)
 }
 
 /* Short options */
-static char const *optstring = "dl:m:n:O:o:p:s:tVvwx";
+static const char *optstring = "dl:m:n:O:o:p:S:s:tVvWwx";
 
 /*
  * Equivalent long options
@@ -155,20 +175,21 @@ static char const *optstring = "dl:m:n:O:o:p:s:tVvwx";
  * over short opt matching
  */
 static struct option const longopts[] = {
-	{ "dmg",          no_argument,       NULL, 'd' },
+	{ "dmg",           no_argument,       NULL, 'd' },
-	{ "linkerscript", required_argument, NULL, 'l' },
+	{ "linkerscript",  required_argument, NULL, 'l' },
-	{ "map",          required_argument, NULL, 'm' },
+	{ "map",           required_argument, NULL, 'm' },
-	{ "sym",          required_argument, NULL, 'n' },
+	{ "sym",           required_argument, NULL, 'n' },
-	{ "overlay",      required_argument, NULL, 'O' },
+	{ "overlay",       required_argument, NULL, 'O' },
-	{ "output",       required_argument, NULL, 'o' },
+	{ "output",        required_argument, NULL, 'o' },
-	{ "pad",          required_argument, NULL, 'p' },
+	{ "pad",           required_argument, NULL, 'p' },
-	{ "smart",        required_argument, NULL, 's' },
+	{ "scramble",      required_argument, NULL, 'S' },
-	{ "tiny",         no_argument,       NULL, 't' },
+	{ "smart",         required_argument, NULL, 's' },
-	{ "version",      no_argument,       NULL, 'V' },
+	{ "tiny",          no_argument,       NULL, 't' },
-	{ "verbose",      no_argument,       NULL, 'v' },
+	{ "version",       no_argument,       NULL, 'V' },
-	{ "wramx",        no_argument,       NULL, 'w' },
+	{ "verbose",       no_argument,       NULL, 'v' },
-	{ "nopad",        no_argument,       NULL, 'x' },
+	{ "wramx",         no_argument,       NULL, 'w' },
-	{ NULL,           no_argument,       NULL, 0   }
+	{ "nopad",         no_argument,       NULL, 'x' },
+	{ NULL,            no_argument,       NULL, 0   }
 };
 
 /**
@@ -178,8 +199,8 @@ static void printUsage(void)
 {
 	fputs(
 "Usage: rgblink [-dtVvwx] [-l script] [-m map_file] [-n sym_file]\n"
-"               [-O overlay_file] [-o out_file] [-p pad_value] [-s symbol]\n"
+"               [-O overlay_file] [-o out_file] [-p pad_value]\n"
-"               <file> ...\n"
+"               [-S spec] [-s symbol] <file> ...\n"
 "Useful options:\n"
 "    -l, --linkerscript <path>  set the input linker script\n"
 "    -m, --map <path>           set the output map file\n"
@@ -202,6 +223,132 @@ static void cleanup(void)
 	obj_Cleanup();
 }
 
+enum ScrambledRegion {
+	SCRAMBLE_ROMX,
+	SCRAMBLE_SRAM,
+	SCRAMBLE_WRAMX,
+
+	SCRAMBLE_UNK, // Used for errors
+};
+
+struct {
+	char const *name;
+	uint16_t max;
+} scrambleSpecs[SCRAMBLE_UNK] = {
+	[SCRAMBLE_ROMX]  = { "romx",  65535 },
+	[SCRAMBLE_SRAM]  = { "sram",  255 },
+	[SCRAMBLE_WRAMX] = { "wramx", 7},
+};
+
+static void parseScrambleSpec(char const *spec)
+{
+	// Skip any leading whitespace
+	spec += strspn(spec, " \t");
+
+	// The argument to `-S` should be a comma-separated list of sections followed by an '='
+	// indicating their scramble limit.
+	while (spec) {
+		// Invariant: we should not be pointing at whitespace at this point
+		assert(*spec != ' ' && *spec != '\t');
+
+		// Remember where the region's name begins and ends
+		char const *regionName = spec;
+		size_t regionNameLen = strcspn(spec, "=, \t");
+		// Length of region name string slice for printing, truncated if too long
+		int regionNamePrintLen = regionNameLen > INT_MAX ? INT_MAX : (int)regionNameLen;
+
+		// If this trips, `spec` must be pointing at a ',' or '=' (or NUL) due to the assert
+		if (regionNameLen == 0) {
+			argErr('S', "Missing region name");
+
+			if (*spec == '\0')
+				break;
+			if (*spec == '=') // Skip the limit, too
+				spec = strchr(&spec[1], ','); // Skip to next comma, if any
+			goto next;
+		}
+
+		// Find the next non-blank char after the region name's end
+		spec += regionNameLen + strspn(&spec[regionNameLen], " \t");
+		if (*spec != '\0' && *spec != ',' && *spec != '=') {
+			argErr('S', "Unexpected '%c' after region name \"%.*s\"",
+			       regionNamePrintLen, regionName);
+			// Skip to next ',' or '=' (or NUL) and keep parsing
+			spec += 1 + strcspn(&spec[1], ",=");
+		}
+
+		// Now, determine which region type this is
+		enum ScrambledRegion region = 0;
+
+		while (region < SCRAMBLE_UNK) {
+			// If the strings match (case-insensitively), we got it!
+			// It's OK not to use `strncasecmp` because `regionName` is still
+			// NUL-terminated, since the encompassing spec is.
+			if (!strcasecmp(scrambleSpecs[region].name, regionName))
+				break;
+			region++;
+		}
+
+		if (region == SCRAMBLE_UNK)
+			argErr('S', "Unknown region \"%.*s\"", regionNamePrintLen, regionName);
+
+		if (*spec == '=') {
+			spec++; // `strtoul` will skip the whitespace on its own
+			unsigned long limit;
+			char *endptr;
+
+			if (*spec == '\0' || *spec == ',') {
+				argErr('S', "Empty limit for region \"%.*s\"",
+				       regionNamePrintLen, regionName);
+				goto next;
+			}
+			limit = strtoul(spec, &endptr, 10);
+			endptr += strspn(endptr, " \t");
+			if (*endptr != '\0' && *endptr != ',') {
+				argErr('S', "Invalid non-numeric limit for region \"%.*s\"",
+				       regionNamePrintLen, regionName);
+				endptr = strchr(endptr, ',');
+			}
+			spec = endptr;
+
+			if (region != SCRAMBLE_UNK && limit >= scrambleSpecs[region].max) {
+				argErr('S', "Limit for region \"%.*s\" may not exceed %" PRIu16,
+				       regionNamePrintLen, regionName, scrambleSpecs[region].max);
+				limit = scrambleSpecs[region].max;
+			}
+
+			switch (region) {
+			case SCRAMBLE_ROMX:
+				scrambleROMX = limit;
+				break;
+			case SCRAMBLE_SRAM:
+				scrambleSRAM = limit;
+				break;
+			case SCRAMBLE_WRAMX:
+				scrambleWRAMX = limit;
+				break;
+			case SCRAMBLE_UNK: // The error has already been reported, do nothing
+				break;
+			}
+		} else if (region == SCRAMBLE_WRAMX) {
+			// Only WRAMX can be implied, since ROMX and SRAM size may vary
+			scrambleWRAMX = 7;
+		} else {
+			argErr('S', "Cannot imply limit for region \"%.*s\"",
+			       regionNamePrintLen, regionName);
+		}
+
+next:
+		if (spec) {
+			assert(*spec == ',' || *spec == '\0');
+			if (*spec == ',')
+				spec += 1 + strspn(&spec[1], " \t");
+			if (*spec == '\0')
+				break;
+		}
+	}
+}
+
 int main(int argc, char *argv[])
 {
 	int optionChar;
@@ -234,15 +381,18 @@ int main(int argc, char *argv[])
 		case 'p':
 			value = strtoul(musl_optarg, &endptr, 0);
 			if (musl_optarg[0] == '\0' || *endptr != '\0') {
-				error(NULL, 0, "Invalid argument for option 'p'");
+				argErr('p', "");
 				value = 0xFF;
 			}
 			if (value > 0xFF) {
-				error(NULL, 0, "Argument for 'p' must be a byte (between 0 and 0xFF)");
+				argErr('p', "Argument for 'p' must be a byte (between 0 and 0xFF)");
 				value = 0xFF;
 			}
 			padValue = value;
 			break;
+		case 'S':
+			parseScrambleSpec(musl_optarg);
+			break;
 		case 's':
 			/* FIXME: nobody knows what this does, figure it out */
 			(void)musl_optarg;

src/link/rgblink.1

@@ -20,6 +20,7 @@
 .Op Fl O Ar overlay_file
 .Op Fl o Ar out_file
 .Op Fl p Ar pad_value
+.Op Fl S Ar spec
 .Op Fl s Ar symbol
 .Ar
 .Sh DESCRIPTION
@@ -89,6 +90,14 @@ Has no effect if
 .Fl O
 is specified.
 The default is 0.
+.It Fl S Ar spec , Fl Fl scramble Ar spec
+Enables a different
+.Dq scrambling
+algorithm for placing sections.
+See
+.Sx Scrambling algorithm
+below for an explanation and a description of
+.Ar spec .
 .It Fl s Ar symbol , Fl Fl smart Ar symbol
 This option is ignored.
 It was supposed to perform smart linking but fell into disrepair, and so has been removed.
@@ -113,6 +122,56 @@ When making a ROM, be careful that not using this is not a replacement for
 .Xr rgbfix 1 Ap s Fl p
 option!
 .El
+.Ss Scrambling algorithm
+The default section placement algorithm tries to minimize the number of banks used;
+.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).
+.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
+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.
+.Bl -column "Region name" "Max value" "Size optional"
+Region name Ta Max size Ta Size optional
+.Cm romx Ta 65535 Ta \&No
+.Cm sram Ta 255 Ta \&No
+.Cm wramx Ta 7 Ta Yes
+.El
+.Pp
+A
+.Ar size
+of 0 disables scrambling for that region.
+.Pp
+For example,
+.Ql romx=64,wramx=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
+.Ic SRAM
+sections.
+.Pp
+Later region specs override earlier ones; for example,
+.Ql romx=42, Romx=0
+disables scrambling for
+.Cm romx .
+.Pp
+.Cm wramx
+scrambling is silently ignored if
+.Fl w
+is passed (including if implied by
+.Fl d ) ,
+as
+.Ic WRAMX
+sections will be treated as
+.Ic WRAM0 .
 .Sh EXAMPLES
 All you need for a basic ROM is an object file, which can be made into a ROM image like so:
 .Pp