Add more tests for RGBFIX

Author: Rangi42

src/fix/fix.cpp

@@ -240,16 +240,17 @@ static void
 	std::vector<uint8_t> romx; // Buffer of ROMX bank data
 	uint32_t nbBanks = 1;      // Number of banks *targeted*, including ROM0
 	size_t totalRomxLen = 0;   // *Actual* size of ROMX data
-	uint8_t bank[BANK_SIZE];   // Temp buffer used to store a whole bank's worth of data
 
 	// Handle ROMX
+	auto errorTooLarge = [&name]() {
+		error("\"%s\" has more than 65536 banks", name); // LCOV_EXCL_LINE
+	};
+	static constexpr off_t NB_BANKS_LIMIT = 0x10000;
+	static_assert(NB_BANKS_LIMIT * BANK_SIZE <= SSIZE_MAX, "Max input file size too large for OS");
 	if (input == output) {
-		if (fileSize >= 0x10000 * BANK_SIZE) {
-			error("\"%s\" has more than 65536 banks", name);
-			return;
+		if (fileSize >= NB_BANKS_LIMIT * BANK_SIZE) {
+			return errorTooLarge(); // LCOV_EXCL_LINE
 		}
-		// This should be guaranteed from the size cap...
-		static_assert(0x10000 * BANK_SIZE <= SSIZE_MAX, "Max input file size too large for OS");
 		// Compute number of banks and ROMX len from file size
 		nbBanks = (fileSize + (BANK_SIZE - 1)) / BANK_SIZE; // ceil(fileSize / BANK_SIZE)
 		totalRomxLen = fileSize >= BANK_SIZE ? fileSize - BANK_SIZE : 0;
@@ -258,19 +259,13 @@ static void
 		for (;;) {
 			romx.resize(nbBanks * BANK_SIZE);
 			ssize_t bankLen = readBytes(input, &romx[(nbBanks - 1) * BANK_SIZE], BANK_SIZE);
-
 			// Update bank count, ONLY IF at least one byte was read
 			if (bankLen) {
 				// We're going to read another bank, check that it won't be too much
-				static_assert(
-				    0x10000 * BANK_SIZE <= SSIZE_MAX, "Max input file size too large for OS"
-				);
-				if (nbBanks == 0x10000) {
-					error("\"%s\" has more than 65536 banks", name);
-					return;
+				if (nbBanks == NB_BANKS_LIMIT) {
+					return errorTooLarge(); // LCOV_EXCL_LINE
 				}
 				++nbBanks;
-
 				// Update global checksum, too
 				for (uint16_t i = 0; i < bankLen; ++i) {
 					globalSum += romx[totalRomxLen + i];
@@ -341,6 +336,7 @@ static void
 		// Pipes have already read ROMX and updated globalSum, but not regular files
 		if (input == output) {
 			for (;;) {
+				uint8_t bank[BANK_SIZE];
 				ssize_t bankLen = readBytes(input, bank, sizeof(bank));
 
 				for (uint16_t i = 0; i < bankLen; ++i) {
@@ -432,8 +428,9 @@ static void
 				// LCOV_EXCL_STOP
 			}
 		}
+		uint8_t bank[BANK_SIZE];
 		memset(bank, options.padValue, sizeof(bank));
-		size_t len = (nbBanks - 1) * BANK_SIZE - totalRomxLen; // Don't count ROM0!
+		size_t len = (nbBanks - 1) * sizeof(bank) - totalRomxLen; // Don't count ROM0!
 
 		while (len) {
 			static_assert(sizeof(bank) <= SSIZE_MAX, "Bank too large for reading");
@@ -470,8 +467,10 @@ bool fix_ProcessFile(char const *name, char const *outputName) {
 		} else {
 			output = open(outputName, O_WRONLY | O_BINARY | O_CREAT, 0600);
 			if (output == -1) {
+				// LCOV_EXCL_START
 				error("Failed to open \"%s\" for writing: %s", outputName, strerror(errno));
 				return true;
+				// LCOV_EXCL_STOP
 			}
 			openedOutput = true;
 		}

src/fix/main.cpp

@@ -122,7 +122,9 @@ static void initLogo() {
 			logoFile = stdin;
 		}
 		if (!logoFile) {
+			// LCOV_EXCL_START
 			fatal("Failed to open \"%s\" for reading: %s", options.logoFilename, strerror(errno));
+			// LCOV_EXCL_STOP
 		}
 		Defer closeLogo{[&] { fclose(logoFile); }};
 

src/fix/mbc.cpp

@@ -98,10 +98,6 @@ bool mbc_HasRAM(MbcType type) {
 	return search != mbcData.end() && search->second.second;
 }
 
-static void skipBlankSpace(char const *&ptr) {
-	ptr += strspn(ptr, " \t");
-}
-
 static void skipMBCSpace(char const *&ptr) {
 	ptr += strspn(ptr, " \t_");
 }
@@ -115,16 +111,6 @@ static char normalizeMBCChar(char c) {
 	return c;
 }
 
-static bool readMBCSlice(char const *&name, char const *expected) {
-	while (*expected) {
-		// If `name` is too short, the character will be '\0' and this will return `false`
-		if (normalizeMBCChar(*name++) != *expected++) {
-			return false;
-		}
-	}
-	return true;
-}
-
 [[noreturn]]
 static void fatalUnknownMBC(char const *name) {
 	fatal("Unknown MBC \"%s\"\n%s", name, acceptedMBCNames);
@@ -136,8 +122,7 @@ static void fatalWrongMBCFeatures(char const *name) {
 }
 
 MbcType mbc_ParseName(char const *name, uint8_t &tpp1Major, uint8_t &tpp1Minor) {
-	char const *ptr = name;
-	skipBlankSpace(ptr); // Trim off leading blank space
+	char const *ptr = name + strspn(name, " \t"); // Skip leading blank space
 
 	if (!strcasecmp(ptr, "help") || !strcasecmp(ptr, "list")) {
 		puts(acceptedMBCNames); // Outputs to stdout and appends a newline
@@ -156,14 +141,16 @@ MbcType mbc_ParseName(char const *name, uint8_t &tpp1Major, uint8_t &tpp1Minor)
 	}
 
 	// Begin by reading the MBC type:
-	uint16_t mbc;
+	uint16_t mbc = UINT16_MAX;
 
-#define tryReadSlice(expected) \
-	do { \
-		if (!readMBCSlice(ptr, expected)) { \
-			fatalUnknownMBC(name); \
-		} \
-	} while (0)
+	auto tryReadSlice = [&ptr, &name](char const *expected) {
+		while (*expected) {
+			// If `name` is too short, the character will be '\0' and this will return `false`
+			if (normalizeMBCChar(*ptr++) != *expected++) {
+				fatalUnknownMBC(name);
+			}
+		}
+	};
 
 	switch (*ptr++) {
 	case 'R': // ROM / ROM_ONLY
@@ -183,13 +170,7 @@ MbcType mbc_ParseName(char const *name, uint8_t &tpp1Major, uint8_t &tpp1Minor)
 		switch (*ptr++) {
 		case 'B':
 		case 'b':
-			switch (*ptr++) {
-			case 'C':
-			case 'c':
-				break;
-			default:
-				fatalUnknownMBC(name);
-			}
+			tryReadSlice("C");
 			switch (*ptr++) {
 			case '1':
 				mbc = MBC1;
@@ -209,17 +190,13 @@ MbcType mbc_ParseName(char const *name, uint8_t &tpp1Major, uint8_t &tpp1Minor)
 			case '7':
 				mbc = MBC7_SENSOR_RUMBLE_RAM_BATTERY;
 				break;
-			default:
-				fatalUnknownMBC(name);
 			}
 			break;
 		case 'M':
 		case 'm':
 			tryReadSlice("M01");
 			mbc = MMM01;
 			break;
-		default:
-			fatalUnknownMBC(name);
 		}
 		break;
 
@@ -242,7 +219,7 @@ MbcType mbc_ParseName(char const *name, uint8_t &tpp1Major, uint8_t &tpp1Minor)
 			tryReadSlice("MA5");
 			mbc = BANDAI_TAMA5;
 			break;
-		case 'P': {
+		case 'P':
 			tryReadSlice("P1");
 			// Parse version
 			skipMBCSpace(ptr);
@@ -266,9 +243,6 @@ MbcType mbc_ParseName(char const *name, uint8_t &tpp1Major, uint8_t &tpp1Minor)
 			mbc = TPP1;
 			break;
 		}
-		default:
-			fatalUnknownMBC(name);
-		}
 		break;
 
 	case 'H': // HuC{1, 3}
@@ -281,12 +255,11 @@ MbcType mbc_ParseName(char const *name, uint8_t &tpp1Major, uint8_t &tpp1Minor)
 		case '3':
 			mbc = HUC3;
 			break;
-		default:
-			fatalUnknownMBC(name);
 		}
 		break;
+	}
 
-	default:
+	if (mbc == UINT16_MAX) {
 		fatalUnknownMBC(name);
 	}
 
@@ -301,18 +274,10 @@ MbcType mbc_ParseName(char const *name, uint8_t &tpp1Major, uint8_t &tpp1Minor)
 	static constexpr uint8_t MULTIRUMBLE = 1 << 2;
 	// clang-format on
 
-	for (;;) {
-		skipBlankSpace(ptr); // Trim off trailing blank space
-
-		// If done, start processing "features"
-		if (!*ptr) {
-			break;
-		}
+	while (*ptr) {
 		// We expect a '+' at this point
 		skipMBCSpace(ptr);
-		if (*ptr++ != '+') {
-			fatalUnknownMBC(name);
-		}
+		tryReadSlice("+");
 		skipMBCSpace(ptr);
 
 		switch (*ptr++) {
@@ -341,8 +306,6 @@ MbcType mbc_ParseName(char const *name, uint8_t &tpp1Major, uint8_t &tpp1Minor)
 				tryReadSlice("M");
 				features |= RAM;
 				break;
-			default:
-				fatalUnknownMBC(name);
 			}
 			break;
 
@@ -357,12 +320,8 @@ MbcType mbc_ParseName(char const *name, uint8_t &tpp1Major, uint8_t &tpp1Minor)
 			tryReadSlice("IMER");
 			features |= TIMER;
 			break;
-
-		default:
-			fatalUnknownMBC(name);
 		}
 	}
-#undef tryReadSlice
 
 	switch (mbc) {
 	case ROM:
@@ -459,37 +418,34 @@ MbcType mbc_ParseName(char const *name, uint8_t &tpp1Major, uint8_t &tpp1Minor)
 		}
 		break;
 
-	case TPP1:
+	case TPP1: {
+		// clang-format off: vertically align values
+		static constexpr uint8_t BATTERY_TPP1     = 1 << 3;
+		static constexpr uint8_t TIMER_TPP1       = 1 << 2;
+		static constexpr uint8_t MULTIRUMBLE_TPP1 = 1 << 1;
+		static constexpr uint8_t RUMBLE_TPP1      = 1 << 0;
+		// clang-format on
+
 		if (features & RAM) {
 			warning(WARNING_MBC, "TPP1 requests RAM implicitly if given a non-zero RAM size");
 		}
 		if (features & BATTERY) {
-			mbc |= 0x08;
+			mbc |= BATTERY_TPP1;
 		}
 		if (features & TIMER) {
-			mbc |= 0x04;
-		}
-		if (features & MULTIRUMBLE) {
-			mbc |= 0x03; // Also set the rumble flag
+			mbc |= TIMER_TPP1;
 		}
 		if (features & RUMBLE) {
-			mbc |= 0x01;
+			mbc |= RUMBLE_TPP1;
 		}
 		if (features & SENSOR) {
 			fatalWrongMBCFeatures(name);
 		}
-		// Multiple rumble speeds imply rumble
-		if (mbc & 0x01) {
-			assume(mbc & 0x02);
+		if (features & MULTIRUMBLE) {
+			mbc |= MULTIRUMBLE_TPP1 | RUMBLE_TPP1; // Multiple rumble speeds imply rumble
 		}
 		break;
 	}
-
-	skipBlankSpace(ptr); // Trim off trailing blank space
-
-	// If there is still something left, error out
-	if (*ptr) {
-		fatalUnknownMBC(name);
 	}
 
 	return static_cast<MbcType>(mbc);

test/fix/bad-huc1-features.err

@@ -0,0 +1,23 @@
+FATAL: Features incompatible with MBC ("HUC1")
+Accepted MBC names:
+	ROM ($00) [aka ROM_ONLY]
+	MBC1 ($01), MBC1+RAM ($02), MBC1+RAM+BATTERY ($03)
+	MBC2 ($05), MBC2+BATTERY ($06)
+	ROM+RAM ($08) [deprecated], ROM+RAM+BATTERY ($09) [deprecated]
+	MMM01 ($0B), MMM01+RAM ($0C), MMM01+RAM+BATTERY ($0D)
+	MBC3+TIMER+BATTERY ($0F), MBC3+TIMER+RAM+BATTERY ($10)
+	MBC3 ($11), MBC3+RAM ($12), MBC3+RAM+BATTERY ($13)
+	MBC5 ($19), MBC5+RAM ($1A), MBC5+RAM+BATTERY ($1B)
+	MBC5+RUMBLE ($1C), MBC5+RUMBLE+RAM ($1D), MBC5+RUMBLE+RAM+BATTERY ($1E)
+	MBC6 ($20)
+	MBC7+SENSOR+RUMBLE+RAM+BATTERY ($22)
+	POCKET_CAMERA ($FC)
+	BANDAI_TAMA5 ($FD) [aka TAMA5]
+	HUC3 ($FE)
+	HUC1+RAM+BATTERY ($FF)
+
+	TPP1_1.0, TPP1_1.0+RUMBLE, TPP1_1.0+MULTIRUMBLE, TPP1_1.0+TIMER,
+	TPP1_1.0+TIMER+RUMBLE, TPP1_1.0+TIMER+MULTIRUMBLE, TPP1_1.0+BATTERY,
+	TPP1_1.0+BATTERY+RUMBLE, TPP1_1.0+BATTERY+MULTIRUMBLE,
+	TPP1_1.0+BATTERY+TIMER, TPP1_1.0+BATTERY+TIMER+RUMBLE,
+	TPP1_1.0+BATTERY+TIMER+MULTIRUMBLE

test/fix/bad-huc1-features.flags

@@ -0,0 +1 @@
+-m HUC1

test/fix/bad-mbc-name.err

@@ -0,0 +1,23 @@
+FATAL: Unknown MBC "MBC9"
+Accepted MBC names:
+	ROM ($00) [aka ROM_ONLY]
+	MBC1 ($01), MBC1+RAM ($02), MBC1+RAM+BATTERY ($03)
+	MBC2 ($05), MBC2+BATTERY ($06)
+	ROM+RAM ($08) [deprecated], ROM+RAM+BATTERY ($09) [deprecated]
+	MMM01 ($0B), MMM01+RAM ($0C), MMM01+RAM+BATTERY ($0D)
+	MBC3+TIMER+BATTERY ($0F), MBC3+TIMER+RAM+BATTERY ($10)
+	MBC3 ($11), MBC3+RAM ($12), MBC3+RAM+BATTERY ($13)
+	MBC5 ($19), MBC5+RAM ($1A), MBC5+RAM+BATTERY ($1B)
+	MBC5+RUMBLE ($1C), MBC5+RUMBLE+RAM ($1D), MBC5+RUMBLE+RAM+BATTERY ($1E)
+	MBC6 ($20)
+	MBC7+SENSOR+RUMBLE+RAM+BATTERY ($22)
+	POCKET_CAMERA ($FC)
+	BANDAI_TAMA5 ($FD) [aka TAMA5]
+	HUC3 ($FE)
+	HUC1+RAM+BATTERY ($FF)
+
+	TPP1_1.0, TPP1_1.0+RUMBLE, TPP1_1.0+MULTIRUMBLE, TPP1_1.0+TIMER,
+	TPP1_1.0+TIMER+RUMBLE, TPP1_1.0+TIMER+MULTIRUMBLE, TPP1_1.0+BATTERY,
+	TPP1_1.0+BATTERY+RUMBLE, TPP1_1.0+BATTERY+MULTIRUMBLE,
+	TPP1_1.0+BATTERY+TIMER, TPP1_1.0+BATTERY+TIMER+RUMBLE,
+	TPP1_1.0+BATTERY+TIMER+MULTIRUMBLE

test/fix/bad-mbc-name.flags

@@ -0,0 +1 @@
+-m MBC9

test/fix/bad-mbc-parse.err

@@ -0,0 +1,23 @@
+FATAL: Unknown MBC "123abc"
+Accepted MBC names:
+	ROM ($00) [aka ROM_ONLY]
+	MBC1 ($01), MBC1+RAM ($02), MBC1+RAM+BATTERY ($03)
+	MBC2 ($05), MBC2+BATTERY ($06)
+	ROM+RAM ($08) [deprecated], ROM+RAM+BATTERY ($09) [deprecated]
+	MMM01 ($0B), MMM01+RAM ($0C), MMM01+RAM+BATTERY ($0D)
+	MBC3+TIMER+BATTERY ($0F), MBC3+TIMER+RAM+BATTERY ($10)
+	MBC3 ($11), MBC3+RAM ($12), MBC3+RAM+BATTERY ($13)
+	MBC5 ($19), MBC5+RAM ($1A), MBC5+RAM+BATTERY ($1B)
+	MBC5+RUMBLE ($1C), MBC5+RUMBLE+RAM ($1D), MBC5+RUMBLE+RAM+BATTERY ($1E)
+	MBC6 ($20)
+	MBC7+SENSOR+RUMBLE+RAM+BATTERY ($22)
+	POCKET_CAMERA ($FC)
+	BANDAI_TAMA5 ($FD) [aka TAMA5]
+	HUC3 ($FE)
+	HUC1+RAM+BATTERY ($FF)
+
+	TPP1_1.0, TPP1_1.0+RUMBLE, TPP1_1.0+MULTIRUMBLE, TPP1_1.0+TIMER,
+	TPP1_1.0+TIMER+RUMBLE, TPP1_1.0+TIMER+MULTIRUMBLE, TPP1_1.0+BATTERY,
+	TPP1_1.0+BATTERY+RUMBLE, TPP1_1.0+BATTERY+MULTIRUMBLE,
+	TPP1_1.0+BATTERY+TIMER, TPP1_1.0+BATTERY+TIMER+RUMBLE,
+	TPP1_1.0+BATTERY+TIMER+MULTIRUMBLE

test/fix/bad-mbc-parse.flags

@@ -0,0 +1 @@
+-m 123abc

test/fix/bad-mbc-range.err

@@ -0,0 +1 @@
+FATAL: Specified MBC ID out of range 0-255: "999"