Report locations for RGBLINK errors with conflicting objects (#1494)

This requires updating the object file format to record the
fstack context for sections themselves, not just for patches.

include/link/section.hpp

@@ -46,6 +46,8 @@ struct Section {
 	bool isAlignFixed;
 	uint16_t alignMask;
 	uint16_t alignOfs;
+	FileStackNode const *src;
+	int32_t lineNo;
 	std::vector<uint8_t> data; // Array of size `size`, or 0 if `type` does not have data
 	std::vector<Patch> patches;
 	// Extra info computed during linking

include/link/symbol.hpp

@@ -25,7 +25,6 @@ struct Symbol {
 	// Info contained in the object files
 	std::string name;
 	ExportLevel type;
-	char const *objFileName;
 	FileStackNode const *src;
 	int32_t lineNo;
 	Either<

man/rgbds.5

@@ -145,6 +145,10 @@ If the symbol belongs to a section, this is the offset within that symbol's sect
 .Bl -tag -width Ds -compact
 .It Cm STRING Ar Name
 The section's name.
+.It Cm LONG Ar NodeID
+Context in which the section was defined.
+.It Cm LONG Ar LineNo
+Line number in the context at which the section was defined.
 .It Cm LONG Ar Size
 The section's size, in bytes.
 .It Cm BYTE Ar Type

src/asm/output.cpp

@@ -85,8 +85,13 @@ static void writePatch(Patch const &patch, FILE *file) {
 }
 
 static void writeSection(Section const &sect, FILE *file) {
+	assume(sect.src->ID != (uint32_t)-1);
+
 	putString(sect.name, file);
 
+	putLong(sect.src->ID, file);
+	putLong(sect.fileLine, file);
+
 	putLong(sect.size, file);
 
 	bool isUnion = sect.modifier == SECTION_UNION;

src/asm/section.cpp

@@ -288,6 +288,8 @@ static Section *createSection(
 	sect.align = alignment;
 	sect.alignOfs = alignOffset;
 
+	out_RegisterNode(sect.src);
+
 	// It is only needed to allocate memory for ROM sections.
 	if (sect_HasData(type))
 		sect.data.resize(sectionTypeInfo[type].size);

src/link/object.cpp

@@ -193,7 +193,6 @@ static void readSymbol(
 	);
 	// If the symbol is defined in this file, read its definition
 	if (symbol.type != SYMTYPE_IMPORT) {
-		symbol.objFileName = fileName;
 		uint32_t nodeID;
 		tryReadLong(
 		    nodeID, file, "%s: Cannot read \"%s\"'s node ID: %s", fileName, symbol.name.c_str()
@@ -346,6 +345,18 @@ static void readSection(
 	uint8_t byte;
 
 	tryReadString(section.name, file, "%s: Cannot read section name: %s", fileName);
+	uint32_t nodeID;
+	tryReadLong(
+	    nodeID, file, "%s: Cannot read \"%s\"'s node ID: %s", fileName, section.name.c_str()
+	);
+	section.src = &fileNodes[nodeID];
+	tryReadLong(
+	    section.lineNo,
+	    file,
+	    "%s: Cannot read \"%s\"'s line number: %s",
+	    fileName,
+	    section.name.c_str()
+	);
 	tryReadLong(tmp, file, "%s: Cannot read \"%s\"'s' size: %s", fileName, section.name.c_str());
 	if (tmp < 0 || tmp > UINT16_MAX)
 		errx("\"%s\"'s section size (%" PRId32 ") is invalid", section.name.c_str(), tmp);

src/link/sdas_obj.cpp

@@ -256,6 +256,9 @@ void sdobj_ReadFile(FileStackNode const &where, FILE *file, std::vector<Symbol>
 				);
 			std::unique_ptr<Section> curSection = std::make_unique<Section>();
 
+			curSection->src = &where;
+			curSection->lineNo = lineNo;
+
 			getToken(line.data(), "'A' line is too short");
 			assume(strlen(token) != 0); // This should be impossible, tokens are non-empty
 			// The following is required for fragment offsets to be reliably predicted
@@ -355,7 +358,6 @@ void sdobj_ReadFile(FileStackNode const &where, FILE *file, std::vector<Symbol>
 			Symbol &symbol = fileSymbols.emplace_back();
 
 			// Init other members
-			symbol.objFileName = where.name().c_str();
 			symbol.src = &where;
 			symbol.lineNo = lineNo;
 
@@ -408,16 +410,17 @@ void sdobj_ReadFile(FileStackNode const &where, FILE *file, std::vector<Symbol>
 					    || (symbolSection && !symbolSection->isAddressFixed)) {
 						sym_AddSymbol(symbol); // This will error out
 					} else if (otherValue != symbolValue) {
-						error(
+						fprintf(
-						    &where,
+						    stderr,
-						    lineNo,
+						    "error: \"%s\" is defined as %" PRId32 " at ",
-						    "Definition of \"%s\" conflicts with definition in %s (%" PRId32
-						    " != %" PRId32 ")",
 						    symbol.name.c_str(),
-						    other->objFileName,
+						    symbolValue
-						    symbolValue,
-						    otherValue
 						);
+						symbol.src->dump(symbol.lineNo);
+						fprintf(stderr, ", but as %" PRId32 " at ", otherValue);
+						other->src->dump(other->lineNo);
+						putc('\n', stderr);
+						exit(1);
 					}
 				} else {
 					// Add a new definition

src/link/section.cpp

@@ -21,24 +21,32 @@ void sect_ForEach(void (*callback)(Section &)) {
 static void checkAgainstFixedAddress(Section const &target, Section const &other, uint16_t org) {
 	if (target.isAddressFixed) {
 		if (target.org != org) {
-			errx(
+			fprintf(
-			    "Section \"%s\" is defined with conflicting addresses $%04" PRIx16
+			    stderr,
-			    " and $%04" PRIx16,
+			    "error: Section \"%s\" is defined with address $%04" PRIx16 " at ",
-			    other.name.c_str(),
+			    target.name.c_str(),
-			    target.org,
+			    target.org
-			    other.org
 			);
+			target.src->dump(target.lineNo);
+			fprintf(stderr, ", but with address $%04" PRIx16 " at ", other.org);
+			other.src->dump(other.lineNo);
+			putc('\n', stderr);
+			exit(1);
 		}
 	} else if (target.isAlignFixed) {
 		if ((org - target.alignOfs) & target.alignMask) {
-			errx(
+			fprintf(
-			    "Section \"%s\" is defined with conflicting %d-byte alignment (offset %" PRIu16
+			    stderr,
-			    ") and address $%04" PRIx16,
+			    "error: Section \"%s\" is defined with %d-byte alignment (offset %" PRIu16 ") at ",
-			    other.name.c_str(),
+			    target.name.c_str(),
 			    target.alignMask + 1,
-			    target.alignOfs,
+			    target.alignOfs
-			    other.org
 			);
+			target.src->dump(target.lineNo);
+			fprintf(stderr, ", but with address $%04" PRIx16 " at ", other.org);
+			other.src->dump(other.lineNo);
+			putc('\n', stderr);
+			exit(1);
 		}
 	}
 }
@@ -46,27 +54,43 @@ static void checkAgainstFixedAddress(Section const &target, Section const &other
 static bool checkAgainstFixedAlign(Section const &target, Section const &other, int32_t ofs) {
 	if (target.isAddressFixed) {
 		if ((target.org - ofs) & other.alignMask) {
-			errx(
+			fprintf(
-			    "Section \"%s\" is defined with conflicting address $%04" PRIx16
+			    stderr,
-			    " and %d-byte alignment (offset %" PRIu16 ")",
+			    "error: Section \"%s\" is defined with address $%04" PRIx16 " at ",
-			    other.name.c_str(),
+			    target.name.c_str(),
-			    target.org,
+			    target.org
+			);
+			target.src->dump(target.lineNo);
+			fprintf(
+			    stderr,
+			    ", but with %d-byte alignment (offset %" PRIu16 ") at ",
 			    other.alignMask + 1,
 			    other.alignOfs
 			);
+			other.src->dump(other.lineNo);
+			putc('\n', stderr);
+			exit(1);
 		}
 		return false;
 	} else if (target.isAlignFixed
 	           && (other.alignMask & target.alignOfs) != (target.alignMask & ofs)) {
-		errx(
+		fprintf(
-		    "Section \"%s\" is defined with conflicting %d-byte alignment (offset %" PRIu16
+		    stderr,
-		    ") and %d-byte alignment (offset %" PRIu16 ")",
+		    "error: Section \"%s\" is defined with %d-byte alignment (offset %" PRIu16 ") at ",
-		    other.name.c_str(),
+		    target.name.c_str(),
 		    target.alignMask + 1,
-		    target.alignOfs,
+		    target.alignOfs
+		);
+		target.src->dump(target.lineNo);
+		fprintf(
+		    stderr,
+		    ", but with %d-byte alignment (offset %" PRIu16 ") at ",
 		    other.alignMask + 1,
 		    other.alignOfs
 		);
+		other.src->dump(other.lineNo);
+		putc('\n', stderr);
+		exit(1);
 	} else {
 		return !target.isAlignFixed || (other.alignMask > target.alignMask);
 	}
@@ -106,25 +130,36 @@ static void checkFragmentCompat(Section &target, Section &other) {
 static void mergeSections(Section &target, std::unique_ptr<Section> &&other, SectionModifier mod) {
 	// Common checks
 
-	if (target.type != other->type)
+	if (target.type != other->type) {
-		errx(
+		fprintf(
-		    "Section \"%s\" is defined with conflicting types %s and %s",
+		    stderr,
-		    other->name.c_str(),
+		    "error: Section \"%s\" is defined with type %s at ",
-		    sectionTypeInfo[target.type].name.c_str(),
+		    target.name.c_str(),
-		    sectionTypeInfo[other->type].name.c_str()
+		    sectionTypeInfo[target.type].name.c_str()
 		);
+		target.src->dump(target.lineNo);
+		fprintf(stderr, ", but with type %s at ", sectionTypeInfo[other->type].name.c_str());
+		other->src->dump(other->lineNo);
+		putc('\n', stderr);
+		exit(1);
+	}
 
 	if (other->isBankFixed) {
 		if (!target.isBankFixed) {
 			target.isBankFixed = true;
 			target.bank = other->bank;
 		} else if (target.bank != other->bank) {
-			errx(
+			fprintf(
-			    "Section \"%s\" is defined with conflicting banks %" PRIu32 " and %" PRIu32,
+			    stderr,
-			    other->name.c_str(),
+			    "error: Section \"%s\" is defined with bank %" PRIu32 " at ",
-			    target.bank,
+			    target.name.c_str(),
-			    other->bank
+			    target.bank
 			);
+			target.src->dump(target.lineNo);
+			fprintf(stderr, ", but with bank %" PRIu32 " at ", other->bank);
+			other->src->dump(other->lineNo);
+			putc('\n', stderr);
+			exit(1);
 		}
 	}
 
@@ -164,17 +199,28 @@ static void mergeSections(Section &target, std::unique_ptr<Section> &&other, Sec
 void sect_AddSection(std::unique_ptr<Section> &&section) {
 	// Check if the section already exists
 	if (Section *other = sect_GetSection(section->name); other) {
-		if (section->modifier != other->modifier)
+		if (section->modifier != other->modifier) {
-			errx(
+			fprintf(
-			    "Section \"%s\" defined as %s and %s",
+			    stderr,
+			    "error: Section \"%s\" is defined as %s at ",
 			    section->name.c_str(),
-			    sectionModNames[section->modifier],
+			    sectionModNames[section->modifier]
-			    sectionModNames[other->modifier]
 			);
-		else if (section->modifier == SECTION_NORMAL)
+			section->src->dump(section->lineNo);
-			errx("Section name \"%s\" is already in use", section->name.c_str());
+			fprintf(stderr, ", but as %s at ", sectionModNames[other->modifier]);
-		else
+			other->src->dump(other->lineNo);
+			putc('\n', stderr);
+			exit(1);
+		} else if (section->modifier == SECTION_NORMAL) {
+			fprintf(stderr, "error: Section \"%s\" is defined at ", section->name.c_str());
+			section->src->dump(section->lineNo);
+			fputs(", but also at ", stderr);
+			other->src->dump(other->lineNo);
+			putc('\n', stderr);
+			exit(1);
+		} else {
 			mergeSections(*other, std::move(section), section->modifier);
+		}
 	} else if (section->modifier == SECTION_UNION && sect_HasData(section->type)) {
 		errx(
 		    "Section \"%s\" is of type %s, which cannot be unionized",

src/link/symbol.cpp

@@ -2,6 +2,7 @@
 
 #include "link/symbol.hpp"
 
+#include <inttypes.h>
 #include <stdlib.h>
 #include <unordered_map>
 #include <vector>
@@ -33,9 +34,19 @@ void sym_AddSymbol(Symbol &symbol) {
 
 	// Check if the symbol already exists with a different value
 	if (other && !(symValue && otherValue && *symValue == *otherValue)) {
-		fprintf(stderr, "error: \"%s\" both in %s from ", symbol.name.c_str(), symbol.objFileName);
+		fprintf(stderr, "error: \"%s\" is defined as ", symbol.name.c_str());
+		if (symValue)
+			fprintf(stderr, "%" PRId32, *symValue);
+		else
+			fputs("a label", stderr);
+		fputs(" at ", stderr);
 		symbol.src->dump(symbol.lineNo);
-		fprintf(stderr, " and in %s from ", other->objFileName);
+		fputs(", but as ", stderr);
+		if (otherValue)
+			fprintf(stderr, "%" PRId32, *otherValue);
+		else
+			fputs("another label", stderr);
+		fputs(" at ", stderr);
 		other->src->dump(other->lineNo);
 		putc('\n', stderr);
 		exit(1);

test/link/fragment-align/base-bad/out.err

@@ -1 +1 @@
-error: Section "Frag" is defined with conflicting 2-byte alignment (offset 1) and 4-byte alignment (offset 3)
+error: Section "Frag" is defined with 2-byte alignment (offset 1) at fragment-align/base-bad/a.asm(2), but with 4-byte alignment (offset 3) at fragment-align/base-bad/b.asm(2)

test/link/fragment-align/org-bad/out.err

@@ -1 +1 @@
-error: Section "Frag" is defined with conflicting address $0000 and 4-byte alignment (offset 2)
+error: Section "Frag" is defined with address $0000 at fragment-align/org-bad/a.asm(2), but with 4-byte alignment (offset 2) at fragment-align/org-bad/b.asm(2)

test/link/fragment-align/org-rev-bad/out.err

@@ -1 +1 @@
-error: Section "Frag" is defined with conflicting 4-byte alignment (offset 1) and address $0006
+error: Section "Frag" is defined with 4-byte alignment (offset 1) at fragment-align/org-rev-bad/a.asm(6), but with address $0006 at fragment-align/org-rev-bad/b.asm(2)

test/link/section-normal/same-name/a.asm

@@ -0,0 +1,2 @@
+SECTION "Same", ROM0
+db 1

test/link/section-normal/same-name/b.asm

@@ -0,0 +1,2 @@
+SECTION "Same", ROM0
+db 2

test/link/section-normal/same-name/out.err

@@ -0,0 +1 @@
+error: Section "Same" is defined at section-normal/same-name/b.asm(1), but also at section-normal/same-name/a.asm(1)

test/link/section-union/align-conflict.out

@@ -1,4 +1,4 @@
-error: Section "conflicting alignment" is defined with conflicting 4-byte alignment (offset 0) and address $cafe
+error: Section "conflicting alignment" is defined with 4-byte alignment (offset 0) at section-union/align-conflict.asm(7), but with address $cafe at section-union/align-conflict.asm(7)
 ---
 error: <stdin>(18):
     Section already declared as aligned to 4 bytes (offset 0)

test/link/section-union/align-ofs-conflict.out

@@ -1,4 +1,4 @@
-error: Section "conflicting alignment" is defined with conflicting 8-byte alignment (offset 7) and 16-byte alignment (offset 14)
+error: Section "conflicting alignment" is defined with 8-byte alignment (offset 7) at section-union/align-ofs-conflict.asm(7), but with 16-byte alignment (offset 14) at section-union/align-ofs-conflict.asm(7)
 ---
 error: <stdin>(18):
     Section already declared with incompatible 8-byte alignment (offset 7)

test/link/section-union/bad-types.out

@@ -1,4 +1,4 @@
-error: Section "conflicting types" is defined with conflicting types HRAM and WRAM0
+error: Section "conflicting types" is defined with type HRAM at section-union/bad-types.asm(7), but with type WRAM0 at section-union/bad-types.asm(7)
 ---
 error: <stdin>(18):
     Section already exists but with type HRAM

test/link/section-union/bank-conflict.out

@@ -1,4 +1,4 @@
-error: Section "conflicting banks" is defined with conflicting banks 4 and 1
+error: Section "conflicting banks" is defined with bank 4 at section-union/bank-conflict.asm(5), but with bank 1 at section-union/bank-conflict.asm(5)
 ---
 error: <stdin>(14):
     Section already declared with different bank 4

test/link/section-union/different-ofs.out

@@ -1,4 +1,4 @@
-error: Section "conflicting alignment" is defined with conflicting 8-byte alignment (offset 7) and 8-byte alignment (offset 6)
+error: Section "conflicting alignment" is defined with 8-byte alignment (offset 7) at section-union/different-ofs.asm(7), but with 8-byte alignment (offset 6) at section-union/different-ofs.asm(7)
 ---
 error: <stdin>(18):
     Section already declared with incompatible 8-byte alignment (offset 7)

test/link/section-union/org-conflict.out

@@ -1,4 +1,4 @@
-error: Section "conflicting address" is defined with conflicting addresses $beef and $babe
+error: Section "conflicting address" is defined with address $beef at section-union/org-conflict.asm(7), but with address $babe at section-union/org-conflict.asm(7)
 ---
 error: <stdin>(16):
     Section already declared as fixed at different address $beef

test/link/section-union/same-export/out.err

@@ -1 +1 @@
-error: "Same" both in section-union/same-export/a.o from section-union/same-export/a.asm(2) and in section-union/same-export/b.o from section-union/same-export/b.asm(2)
+error: "Same" is defined as a label at section-union/same-export/a.asm(2), but as another label at section-union/same-export/b.asm(2)

test/link/symbols/conflict/a.asm

@@ -0,0 +1 @@
+EXPORT DEF SAME EQU 1

test/link/symbols/conflict/b.asm

@@ -0,0 +1 @@
+EXPORT DEF SAME EQU 2

test/link/symbols/conflict/out.err

@@ -0,0 +1 @@
+error: "SAME" is defined as 2 at symbols/conflict/b.asm(1), but as 1 at symbols/conflict/a.asm(1)

test/link/test.sh

@@ -86,17 +86,6 @@ evaluateTest () {
 	fi
 }
 
-substPath () {
-	# Replace the file name with a different one to match changed output, escaping regex metacharacters
-	subst="$(printf '%s\n' "$1" | sed 's:[][\/.^$*]:\\&:g')"
-	sed -i'' -e "s|$subst|$2|g" "$3"
-	if which cygpath &>/dev/null; then
-		# Replace the MinGW path with a Windows path, escaping regex metacharacters
-		subst="$(cygpath -m "$1" | sed 's:[][\/.^$*]:\\&:g')"
-		sed -i'' -e "s|$subst|$2|g" "$3"
-	fi
-}
-
 for i in *.asm; do
 	test=${i%.asm}
 	startTest
@@ -268,6 +257,15 @@ rgblinkQuiet -o "$gbtemp" "$otemp" "$gbtemp2"
 tryCmpRom "$test"/ref.out.bin
 evaluateTest
 
+test="section-normal/same-name"
+startTest
+"$RGBASM" -o "$otemp" "$test"/a.asm
+"$RGBASM" -o "$gbtemp" "$test"/b.asm
+continueTest
+rgblinkQuiet "$otemp" "$gbtemp" 2>"$outtemp"
+tryDiff "$test"/out.err "$outtemp"
+evaluateTest
+
 test="section-union/good"
 startTest
 "$RGBASM" -o "$otemp" "$test"/a.asm
@@ -283,8 +281,6 @@ startTest
 "$RGBASM" -o "$gbtemp2" "$test"/b.asm
 continueTest
 rgblinkQuiet "$gbtemp2" "$otemp" 2>"$outtemp"
-substPath "$otemp" "$test/a.o" "$outtemp"
-substPath "$gbtemp2" "$test/b.o" "$outtemp"
 tryDiff "$test"/out.err "$outtemp"
 evaluateTest
 
@@ -294,8 +290,6 @@ startTest
 "$RGBASM" -o "$gbtemp2" "$test"/b.asm
 continueTest
 rgblinkQuiet -o "$gbtemp" "$gbtemp2" "$otemp" 2>"$outtemp"
-substPath "$otemp" "$test/a.o" "$outtemp"
-substPath "$gbtemp2" "$test/b.o" "$outtemp"
 tryDiff "$test"/out.err "$outtemp"
 tryCmpRom "$test"/ref.out.bin
 evaluateTest
@@ -317,6 +311,15 @@ for i in section-union/*.asm; do
 	evaluateTest
 done
 
+test="symbols/conflict"
+startTest
+"$RGBASM" -o "$otemp" "$test"/a.asm
+"$RGBASM" -o "$gbtemp" "$test"/b.asm
+continueTest
+rgblinkQuiet "$otemp" "$gbtemp" 2>"$outtemp"
+tryDiff "$test"/out.err "$outtemp"
+evaluateTest
+
 test="symbols/good"
 startTest
 "$RGBASM" -o "$otemp" "$test"/a.asm