Report any section overflows at the end of assembly (#1482)

* Report any section overflows at the end of assembly

* Immediately handle overflow of the 32-bit size counter

include/asm/section.hpp

@@ -78,6 +78,8 @@ uint32_t sect_GetOutputOffset();
 uint32_t sect_GetAlignBytes(uint8_t alignment, uint16_t offset);
 void sect_AlignPC(uint8_t alignment, uint16_t offset);
 
+void sect_CheckSizes();
+
 void sect_StartUnion();
 void sect_NextUnionMember();
 void sect_EndUnion();

src/asm/main.cpp

@@ -381,6 +381,7 @@ int main(int argc, char *argv[]) {
 		nbErrors = 1;
 
 	sect_CheckUnionClosed();
+	sect_CheckSizes();
 
 	if (nbErrors != 0)
 		errx("Assembly aborted (%u error%s)!", nbErrors, nbErrors == 1 ? "" : "s");

src/asm/section.cpp

@@ -72,41 +72,17 @@ int32_t loadOffset; // Offset into the LOAD section's parent (see sect_GetOutput
 	return false;
 }
 
-[[nodiscard]] static bool checkSectionSize(Section const &sect, uint32_t size) {
-	uint32_t maxSize = sectionTypeInfo[sect.type].size;
-
-	// If the new size is reasonable, keep going
-	if (size <= maxSize)
-		return true;
-
+void sect_CheckSizes() {
+	for (Section const &sect : sectionList) {
+		if (uint32_t maxSize = sectionTypeInfo[sect.type].size; sect.size > maxSize)
 			error(
-	    "Section '%s' grew too big (max size = 0x%" PRIX32 " bytes, reached 0x%" PRIX32 ").\n",
+				"Section '%s' grew too big (max size = 0x%" PRIX32 " bytes, reached 0x%" PRIX32
+				").\n",
 				sect.name.c_str(),
 				maxSize,
-	    size
+				sect.size
 			);
-	return false;
 	}
-
-// Check if the section has grown too much.
-[[nodiscard]] static bool reserveSpace(uint32_t delta_size) {
-	// This check is here to trap broken code that generates sections that are too big and to
-	// prevent the assembler from generating huge object files or trying to allocate too much
-	// memory.
-	// A check at the linking stage is still necessary.
-
-	// If the section has already overflowed, skip the check to avoid erroring out ad nauseam
-	if (currentSection->size != UINT32_MAX
-	    && !checkSectionSize(*currentSection, curOffset + loadOffset + delta_size))
-		// Mark the section as overflowed, to avoid repeating the error
-		currentSection->size = UINT32_MAX;
-
-	if (currentLoadSection && currentLoadSection->size != UINT32_MAX
-	    && !checkSectionSize(*currentLoadSection, curOffset + delta_size))
-		currentLoadSection->size = UINT32_MAX;
-
-	return currentSection->size != UINT32_MAX
-	       && (!currentLoadSection || currentLoadSection->size != UINT32_MAX);
 }
 
 Section *sect_FindSectionByName(std::string const &name) {
@@ -583,6 +559,8 @@ void sect_AlignPC(uint8_t alignment, uint16_t offset) {
 }
 
 static void growSection(uint32_t growth) {
+	if (growth > 0 && curOffset > UINT32_MAX - growth)
+		fatalerror("Section size would overflow internal counter\n");
 	curOffset += growth;
 	if (curOffset + loadOffset > currentSection->size)
 		currentSection->size = curOffset + loadOffset;
@@ -591,7 +569,8 @@ static void growSection(uint32_t growth) {
 }
 
 static void writebyte(uint8_t byte) {
-	currentSection->data[sect_GetOutputOffset()] = byte;
+	if (uint32_t index = sect_GetOutputOffset(); index < currentSection->data.size())
+		currentSection->data[index] = byte;
 	growSection(1);
 }
 
@@ -665,8 +644,6 @@ void sect_CheckUnionClosed() {
 void sect_AbsByte(uint8_t b) {
 	if (!checkcodesection())
 		return;
-	if (!reserveSpace(1))
-		return;
 
 	writebyte(b);
 }
@@ -674,8 +651,6 @@ void sect_AbsByte(uint8_t b) {
 void sect_AbsByteString(std::vector<int32_t> const &s) {
 	if (!checkcodesection())
 		return;
-	if (!reserveSpace(s.size()))
-		return;
 
 	for (int32_t v : s) {
 		if (!checkNBit(v, 8, "All character units"))
@@ -689,8 +664,6 @@ void sect_AbsByteString(std::vector<int32_t> const &s) {
 void sect_AbsWordString(std::vector<int32_t> const &s) {
 	if (!checkcodesection())
 		return;
-	if (!reserveSpace(s.size() * 2))
-		return;
 
 	for (int32_t v : s) {
 		if (!checkNBit(v, 16, "All character units"))
@@ -704,8 +677,6 @@ void sect_AbsWordString(std::vector<int32_t> const &s) {
 void sect_AbsLongString(std::vector<int32_t> const &s) {
 	if (!checkcodesection())
 		return;
-	if (!reserveSpace(s.size() * 4))
-		return;
 
 	for (int32_t v : s)
 		writelong(static_cast<uint32_t>(v));
@@ -715,8 +686,6 @@ void sect_AbsLongString(std::vector<int32_t> const &s) {
 void sect_Skip(uint32_t skip, bool ds) {
 	if (!checksection())
 		return;
-	if (!reserveSpace(skip))
-		return;
 
 	if (!sect_HasData(currentSection->type)) {
 		growSection(skip);
@@ -740,8 +709,6 @@ void sect_Skip(uint32_t skip, bool ds) {
 void sect_RelByte(Expression &expr, uint32_t pcShift) {
 	if (!checkcodesection())
 		return;
-	if (!reserveSpace(1))
-		return;
 
 	if (!expr.isKnown()) {
 		createPatch(PATCHTYPE_BYTE, expr, pcShift);
@@ -756,8 +723,6 @@ void sect_RelByte(Expression &expr, uint32_t pcShift) {
 void sect_RelBytes(uint32_t n, std::vector<Expression> &exprs) {
 	if (!checkcodesection())
 		return;
-	if (!reserveSpace(n))
-		return;
 
 	for (uint32_t i = 0; i < n; i++) {
 		Expression &expr = exprs[i % exprs.size()];
@@ -776,8 +741,6 @@ void sect_RelBytes(uint32_t n, std::vector<Expression> &exprs) {
 void sect_RelWord(Expression &expr, uint32_t pcShift) {
 	if (!checkcodesection())
 		return;
-	if (!reserveSpace(2))
-		return;
 
 	if (!expr.isKnown()) {
 		createPatch(PATCHTYPE_WORD, expr, pcShift);
@@ -792,8 +755,6 @@ void sect_RelWord(Expression &expr, uint32_t pcShift) {
 void sect_RelLong(Expression &expr, uint32_t pcShift) {
 	if (!checkcodesection())
 		return;
-	if (!reserveSpace(2))
-		return;
 
 	if (!expr.isKnown()) {
 		createPatch(PATCHTYPE_LONG, expr, pcShift);
@@ -808,11 +769,8 @@ void sect_RelLong(Expression &expr, uint32_t pcShift) {
 void sect_PCRelByte(Expression &expr, uint32_t pcShift) {
 	if (!checkcodesection())
 		return;
-	if (!reserveSpace(1))
-		return;
-	Symbol const *pc = sym_GetPC();
 
-	if (!expr.isDiffConstant(pc)) {
+	if (Symbol const *pc = sym_GetPC(); !expr.isDiffConstant(pc)) {
 		createPatch(PATCHTYPE_JR, expr, pcShift);
 		writebyte(0);
 	} else {
@@ -863,35 +821,32 @@ void sect_BinaryFile(std::string const &name, int32_t startPos) {
 	}
 	Defer closeFile{[&] { fclose(file); }};
 
-	int32_t fsize = -1;
-
 	if (fseek(file, 0, SEEK_END) != -1) {
-		fsize = ftell(file);
-
-		if (startPos > fsize) {
-			error("Specified start position is greater than length of file\n");
+		if (startPos > ftell(file)) {
+			error("Specified start position is greater than length of file '%s'\n", name.c_str());
 			return;
 		}
-
+		// The file is seekable; skip to the specified start position
 		fseek(file, startPos, SEEK_SET);
-
-		if (!reserveSpace(fsize - startPos))
-			return;
 	} else {
 		if (errno != ESPIPE)
 			error(
 			    "Error determining size of INCBIN file '%s': %s\n", name.c_str(), strerror(errno)
 			);
-		// The file isn't seekable, so we'll just skip bytes
-		while (startPos--)
-			(void)fgetc(file);
+		// The file isn't seekable, so we'll just skip bytes one at a time
+		while (startPos--) {
+			if (fgetc(file) == EOF) {
+				error(
+				    "Specified start position is greater than length of file '%s'\n",
+				    name.c_str()
+				);
+				return;
+			}
+		}
 	}
 
-	for (int byte; (byte = fgetc(file)) != EOF;) {
-		if (fsize == -1)
-			growSection(1);
+	for (int byte; (byte = fgetc(file)) != EOF;)
 		writebyte(byte);
-	}
 
 	if (ferror(file))
 		error("Error reading INCBIN file '%s': %s\n", name.c_str(), strerror(errno));
@@ -903,18 +858,14 @@ void sect_BinaryFileSlice(std::string const &name, int32_t startPos, int32_t len
 		error("Start position cannot be negative (%" PRId32 ")\n", startPos);
 		startPos = 0;
 	}
-
 	if (length < 0) {
 		error("Number of bytes to read cannot be negative (%" PRId32 ")\n", length);
 		length = 0;
 	}
-
 	if (!checkcodesection())
 		return;
 	if (length == 0) // Don't even bother with 0-byte slices
 		return;
-	if (!reserveSpace(length))
-		return;
 
 	FILE *file = nullptr;
 	if (std::optional<std::string> fullPath = fstk_FindFile(name); fullPath)
@@ -932,44 +883,50 @@ void sect_BinaryFileSlice(std::string const &name, int32_t startPos, int32_t len
 	Defer closeFile{[&] { fclose(file); }};
 
 	if (fseek(file, 0, SEEK_END) != -1) {
-		int32_t fsize = ftell(file);
-
-		if (startPos > fsize) {
-			error("Specified start position is greater than length of file\n");
+		if (int32_t fsize = ftell(file); startPos > fsize) {
+			error("Specified start position is greater than length of file '%s'\n", name.c_str());
 			return;
-		}
-
-		if ((startPos + length) > fsize) {
+		} else if (startPos + length > fsize) {
 			error(
-			    "Specified range in INCBIN is out of bounds (%" PRIu32 " + %" PRIu32 " > %" PRIu32
-			    ")\n",
+			    "Specified range in INCBIN file '%s' is out of bounds (%" PRIu32 " + %" PRIu32
+			    " > %" PRIu32 ")\n",
+			    name.c_str(),
 			    startPos,
 			    length,
 			    fsize
 			);
 			return;
 		}
-
+		// The file is seekable; skip to the specified start position
 		fseek(file, startPos, SEEK_SET);
 	} else {
 		if (errno != ESPIPE)
 			error(
 			    "Error determining size of INCBIN file '%s': %s\n", name.c_str(), strerror(errno)
 			);
-		// The file isn't seekable, so we'll just skip bytes
-		while (startPos--)
-			(void)fgetc(file);
+		// The file isn't seekable, so we'll just skip bytes one at a time
+		while (startPos--) {
+			if (fgetc(file) == EOF) {
+				error(
+				    "Specified start position is greater than length of file '%s'\n",
+				    name.c_str()
+				);
+				return;
+			}
+		}
 	}
 
 	while (length--) {
-		int byte = fgetc(file);
-
-		if (byte != EOF) {
+		if (int byte = fgetc(file); byte != EOF) {
 			writebyte(byte);
 		} else if (ferror(file)) {
 			error("Error reading INCBIN file '%s': %s\n", name.c_str(), strerror(errno));
 		} else {
-			error("Premature end of file (%" PRId32 " bytes left to read)\n", length + 1);
+			error(
+			    "Premature end of INCBIN file '%s' (%" PRId32 " bytes left to read)\n",
+			    name.c_str(),
+			    length + 1
+			);
 		}
 	}
 }

test/asm/incbin-empty-bad.err

@@ -1,3 +1,3 @@
 error: incbin-empty-bad.asm(3):
-    Specified range in INCBIN is out of bounds (0 + 1 > 0)
+    Specified range in INCBIN file 'empty.bin' is out of bounds (0 + 1 > 0)
 error: Assembly aborted (1 error)!

test/asm/incbin-end-bad.err

@@ -1,3 +1,3 @@
 error: incbin-end-bad.asm(3):
-    Specified range in INCBIN is out of bounds (123 + 1 > 123)
+    Specified range in INCBIN file 'data.bin' is out of bounds (123 + 1 > 123)
 error: Assembly aborted (1 error)!

test/asm/load-overflow.asm

@@ -1,25 +1,25 @@
 SECTION "Overflow", ROM0
 	ds $6000
 LOAD "oops",WRAM0
-	; We might get an error for "oops", but it can also make sense to no-op the directive
-	ds $2001
-	; We shouldn't get any more errors for "Overflow" nor "oops"
+	ds $2000
+	db
 	db
 ENDL
 
 SECTION "Moar overflow", ROM0
-	ds $6001
+	ds $4000
+	ds $4000
 LOAD "hmm", WRAM0
 	ds $2000
-	; Since the `ds` overflows "Moar overflow", it could be no-op'd, making this `db` not error
-	db
+	ds $2000
 ENDL
+	ds $1000
 
 SECTION "Not overflowing", ROM0
-	ds $5FFF
+	ds $800
 LOAD "lol", WRAM0
-	ds $2000
-	db
-	; Since the LOAD block is overflowed, this may be no-op'd, not affecting the "parent"
+	ds $1000
+	ds $1000
+	ds $1000
 ENDL
-	dw ; This, however...
+	ds $800

test/asm/load-overflow.err

@@ -1,11 +1,15 @@
-error: load-overflow.asm(5):
-    Section 'Overflow' grew too big (max size = 0x8000 bytes, reached 0x8001).
-error: load-overflow.asm(5):
-    Section 'oops' grew too big (max size = 0x2000 bytes, reached 0x2001).
-error: load-overflow.asm(13):
-    Section 'Moar overflow' grew too big (max size = 0x8000 bytes, reached 0x8001).
-error: load-overflow.asm(22):
-    Section 'lol' grew too big (max size = 0x2000 bytes, reached 0x2001).
-error: load-overflow.asm(25):
-    Section 'Not overflowing' grew too big (max size = 0x8000 bytes, reached 0x8001).
+warning: load-overflow.asm(5): [-Wempty-data-directive]
+    DB directive without data in ROM
+warning: load-overflow.asm(6): [-Wempty-data-directive]
+    DB directive without data in ROM
+error: load-overflow.asm(26):
+    Section 'Overflow' grew too big (max size = 0x8000 bytes, reached 0x8002).
+error: load-overflow.asm(26):
+    Section 'oops' grew too big (max size = 0x2000 bytes, reached 0x2002).
+error: load-overflow.asm(26):
+    Section 'Moar overflow' grew too big (max size = 0x8000 bytes, reached 0xD000).
+error: load-overflow.asm(26):
+    Section 'hmm' grew too big (max size = 0x2000 bytes, reached 0x4000).
+error: load-overflow.asm(26):
+    Section 'lol' grew too big (max size = 0x2000 bytes, reached 0x3000).
 error: Assembly aborted (5 errors)!

test/asm/section-unsigned-overflow.asm

@@ -0,0 +1,9 @@
+section "overflow", rom0
+rept 10
+	ds $4000_0000
+endr
+
+section "moar overflow", romx
+rept 10
+	ds $4000_0000
+endr

test/asm/section-unsigned-overflow.err

@@ -0,0 +1,2 @@
+FATAL: section-unsigned-overflow.asm(2) -> section-unsigned-overflow.asm::REPT~4(3):
+    Section size would overflow internal counter