From 4a7d333891c69bb77fa7eecced1c3ec924961e18 Mon Sep 17 00:00:00 2001
From: Sylvie <35663410+Rangi42@users.noreply.github.com>
Date: Sat, 9 Mar 2024 11:12:01 -0500
Subject: [PATCH] Use `std::unique_ptr` for rgblink sections (#1337)

---
 include/link/section.hpp |  5 ++-
 src/link/assign.cpp      |  2 +-
 src/link/main.cpp        | 17 --------
 src/link/object.cpp      | 85 +++++++++++++++++++++-------------------
 src/link/output.cpp      |  6 +--
 src/link/patch.cpp       |  2 +-
 src/link/script.y        |  2 +-
 src/link/sdas_obj.cpp    | 24 ++++++------
 src/link/section.cpp     | 73 +++++++++++++++++-----------------
 9 files changed, 102 insertions(+), 114 deletions(-)

diff --git a/include/link/section.hpp b/include/link/section.hpp
index e134743a..5e817e62 100644
--- a/include/link/section.hpp
+++ b/include/link/section.hpp
@@ -6,6 +6,7 @@
 
 // GUIDELINE: external code MUST NOT BE AWARE of the data structure used!
 
+#include <memory>
 #include <stdint.h>
 #include <string>
 #include <vector>
@@ -50,7 +51,7 @@ struct Section {
 	// Extra info computed during linking
 	std::vector<Symbol> *fileSymbols;
 	std::vector<Symbol *> symbols;
-	Section *nextu; // The next "component" of this unionized sect
+	std::unique_ptr<Section> nextu; // The next "component" of this unionized sect
 };
 
 /*
@@ -64,7 +65,7 @@ void sect_ForEach(void (*callback)(Section &));
  * Registers a section to be processed.
  * @param section The section to register.
  */
-void sect_AddSection(Section &section);
+void sect_AddSection(std::unique_ptr<Section> &&section);
 
 /*
  * Finds a section by its name.
diff --git a/src/link/assign.cpp b/src/link/assign.cpp
index b72fa3e7..2006c502 100644
--- a/src/link/assign.cpp
+++ b/src/link/assign.cpp
@@ -57,7 +57,7 @@ static void initFreeSpace() {
 static void assignSection(Section &section, MemoryLocation const &location) {
 	// Propagate the assigned location to all UNIONs/FRAGMENTs
 	// so `jr` patches in them will have the correct offset
-	for (Section *next = &section; next != nullptr; next = next->nextu) {
+	for (Section *next = &section; next != nullptr; next = next->nextu.get()) {
 		next->org = location.address;
 		next->bank = location.bank;
 	}
diff --git a/src/link/main.cpp b/src/link/main.cpp
index 1d3353bf..4f0d733c 100644
--- a/src/link/main.cpp
+++ b/src/link/main.cpp
@@ -346,19 +346,6 @@ next:
 	exit(1);
 }
 
-static void freeSection(Section &section) {
-	Section *next = &section;
-
-	for (Section *nextu; next; next = nextu) {
-		nextu = next->nextu;
-		delete next;
-	};
-}
-
-static void freeSections() {
-	sect_ForEach(freeSection);
-}
-
 int main(int argc, char *argv[]) {
 	// Parse options
 	for (int ch; (ch = musl_getopt_long_only(argc, argv, optstring, longopts, nullptr)) != -1;) {
@@ -462,10 +449,6 @@ int main(int argc, char *argv[]) {
 	if (isDmgMode)
 		sectionTypeInfo[SECTTYPE_VRAM].lastBank = 0;
 
-	// Do cleanup before quitting, though.
-	// Mostly here to please tools such as `valgrind` so actual errors can be seen
-	atexit(freeSections);
-
 	// Read all object files first,
 	for (obj_Setup(argc - curArgIndex); curArgIndex < argc; curArgIndex++)
 		obj_ReadFile(argv[curArgIndex], argc - curArgIndex - 1);
diff --git a/src/link/object.cpp b/src/link/object.cpp
index afb6045d..7b752999 100644
--- a/src/link/object.cpp
+++ b/src/link/object.cpp
@@ -7,7 +7,7 @@
 #include <errno.h>
 #include <inttypes.h>
 #include <limits.h>
-#include <new>
+#include <memory>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -329,8 +329,10 @@ static void readPatch(
  * Sets a patch's pcSection from its pcSectionID.
  * @param patch The patch to fix
  */
-static void linkPatchToPCSect(Patch &patch, std::vector<Section *> const &fileSections) {
-	patch.pcSection = patch.pcSectionID != (uint32_t)-1 ? fileSections[patch.pcSectionID] : nullptr;
+static void
+    linkPatchToPCSect(Patch &patch, std::vector<std::unique_ptr<Section>> const &fileSections) {
+	patch.pcSection =
+	    patch.pcSectionID != (uint32_t)-1 ? fileSections[patch.pcSectionID].get() : nullptr;
 }
 
 /*
@@ -469,10 +471,6 @@ static void readAssertion(
 	tryReadstring(assert.message, file, "%s: Cannot read assertion's message: %s", fileName);
 }
 
-static Section *getMainSection(Section &section) {
-	return section.modifier != SECTION_NORMAL ? sect_GetSection(section.name) : &section;
-}
-
 void obj_ReadFile(char const *fileName, unsigned int fileID) {
 	FILE *file;
 
@@ -571,47 +569,16 @@ void obj_ReadFile(char const *fileName, unsigned int fileID) {
 	}
 
 	// This file's sections, stored in a table to link symbols to them
-	std::vector<Section *> fileSections(nbSections, nullptr);
+	std::vector<std::unique_ptr<Section>> fileSections(nbSections);
 
 	verbosePrint("Reading %" PRIu32 " sections...\n", nbSections);
 	for (uint32_t i = 0; i < nbSections; i++) {
 		// Read section
-		fileSections[i] = new (std::nothrow) Section();
-		if (!fileSections[i])
-			err("%s: Failed to create new section", fileName);
-
+		fileSections[i] = std::make_unique<Section>();
 		fileSections[i]->nextu = nullptr;
 		readSection(file, *fileSections[i], fileName, nodes[fileID]);
 		fileSections[i]->fileSymbols = &fileSymbols;
 		fileSections[i]->symbols.reserve(nbSymPerSect[i]);
-
-		sect_AddSection(*fileSections[i]);
-	}
-
-	// Give patches' PC section pointers to their sections
-	for (uint32_t i = 0; i < nbSections; i++) {
-		if (sect_HasData(fileSections[i]->type)) {
-			for (Patch &patch : fileSections[i]->patches)
-				linkPatchToPCSect(patch, fileSections);
-		}
-	}
-
-	// Give symbols' section pointers to their sections
-	for (uint32_t i = 0; i < nbSymbols; i++) {
-		if (Label *label = std::get_if<Label>(&fileSymbols[i].data); label) {
-			Section *section = fileSections[label->sectionID];
-
-			// Give the section a pointer to the symbol as well
-			linkSymToSect(fileSymbols[i], *section);
-
-			if (section->modifier != SECTION_NORMAL) {
-				if (section->modifier == SECTION_FRAGMENT)
-					// Add the fragment's offset to the symbol's
-					label->offset += section->offset;
-				section = getMainSection(*section);
-			}
-			label->section = section;
-		}
 	}
 
 	uint32_t nbAsserts;
@@ -626,6 +593,44 @@ void obj_ReadFile(char const *fileName, unsigned int fileID) {
 		assertion.fileSymbols = &fileSymbols;
 	}
 
+	// Give patches' PC section pointers to their sections
+	for (uint32_t i = 0; i < nbSections; i++) {
+		if (sect_HasData(fileSections[i]->type)) {
+			for (Patch &patch : fileSections[i]->patches)
+				linkPatchToPCSect(patch, fileSections);
+		}
+	}
+
+	// Give symbols' section pointers to their sections
+	for (uint32_t i = 0; i < nbSymbols; i++) {
+		if (Label *label = std::get_if<Label>(&fileSymbols[i].data); label) {
+			Section *section = fileSections[label->sectionID].get();
+
+			label->section = section;
+			// Give the section a pointer to the symbol as well
+			linkSymToSect(fileSymbols[i], *section);
+		}
+	}
+
+	// Calling `sect_AddSection` invalidates the contents of `fileSections`!
+	for (uint32_t i = 0; i < nbSections; i++)
+		sect_AddSection(std::move(fileSections[i]));
+
+	// Fix symbols' section pointers to component sections
+	// This has to run **after** all the `sect_AddSection()` calls,
+	// so that `sect_GetSection()` will work
+	for (uint32_t i = 0; i < nbSymbols; i++) {
+		if (Label *label = std::get_if<Label>(&fileSymbols[i].data); label) {
+			if (Section *section = label->section; section->modifier != SECTION_NORMAL) {
+				if (section->modifier == SECTION_FRAGMENT)
+					// Add the fragment's offset to the symbol's
+					label->offset += section->offset;
+				// Associate the symbol with the main section, not the "component" one
+				label->section = sect_GetSection(section->name);
+			}
+		}
+	}
+
 	fclose(file);
 }
 
diff --git a/src/link/output.cpp b/src/link/output.cpp
index ef253d10..dd32ba9e 100644
--- a/src/link/output.cpp
+++ b/src/link/output.cpp
@@ -321,11 +321,11 @@ static void writeSymBank(SortedSections const &bankSections, SectionType type, u
 		for (auto it = bankSections.zeroLenSections.begin(); \
 		     it != bankSections.zeroLenSections.end(); \
 		     it++) { \
-			for (Section const *sect = *it; sect; sect = sect->nextu) \
+			for (Section const *sect = *it; sect; sect = sect->nextu.get()) \
 				__VA_ARGS__ \
 		} \
 		for (auto it = bankSections.sections.begin(); it != bankSections.sections.end(); it++) { \
-			for (Section const *sect = *it; sect; sect = sect->nextu) \
+			for (Section const *sect = *it; sect; sect = sect->nextu.get()) \
 				__VA_ARGS__ \
 		} \
 	} while (0)
@@ -431,7 +431,7 @@ static void writeMapBank(SortedSections const &sectList, SectionType type, uint3
 
 		if (!noSymInMap) {
 			// Also print symbols in the following "pieces"
-			for (uint16_t org = sect->org; sect; sect = sect->nextu) {
+			for (uint16_t org = sect->org; sect; sect = sect->nextu.get()) {
 				for (Symbol *sym : sect->symbols)
 					// Space matches "\tSECTION: $xxxx ..."
 					fprintf(
diff --git a/src/link/patch.cpp b/src/link/patch.cpp
index 87a757ef..61ce84d3 100644
--- a/src/link/patch.cpp
+++ b/src/link/patch.cpp
@@ -519,7 +519,7 @@ static void applyPatches(Section &section) {
 	if (!sect_HasData(section.type))
 		return;
 
-	for (Section *component = &section; component; component = component->nextu)
+	for (Section *component = &section; component; component = component->nextu.get())
 		applyFilePatches(*component, section);
 }
 
diff --git a/src/link/script.y b/src/link/script.y
index 38bfe4d3..88975a80 100644
--- a/src/link/script.y
+++ b/src/link/script.y
@@ -579,7 +579,7 @@ static void placeSection(std::string const &name, bool isOptional) {
 	// Check that the linker script doesn't contradict what the code says.
 	if (section->type == SECTTYPE_INVALID) {
 		// SDCC areas don't have a type assigned yet, so the linker script is used to give them one.
-		for (Section *fragment = section; fragment; fragment = fragment->nextu) {
+		for (Section *fragment = section; fragment; fragment = fragment->nextu.get()) {
 			fragment->type = activeType;
 		}
 	} else if (section->type != activeType) {
diff --git a/src/link/sdas_obj.cpp b/src/link/sdas_obj.cpp
index 3f343d37..5fcc9045 100644
--- a/src/link/sdas_obj.cpp
+++ b/src/link/sdas_obj.cpp
@@ -6,7 +6,7 @@
 #include <ctype.h>
 #include <errno.h>
 #include <inttypes.h>
-#include <new>
+#include <memory>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -236,7 +236,7 @@ void sdobj_ReadFile(FileStackNode const &where, FILE *file, std::vector<Symbol>
 	// Now, let's parse the rest of the lines as they come!
 
 	struct FileSection {
-		Section *section;
+		std::unique_ptr<Section> section;
 		uint16_t writeIndex;
 	};
 	std::vector<FileSection> fileSections;
@@ -257,10 +257,7 @@ void sdobj_ReadFile(FileStackNode const &where, FILE *file, std::vector<Symbol>
 				warning(
 				    &where, lineNo, "Got more 'A' lines than the expected %" PRIu32, expectedNbAreas
 				);
-			Section *curSection = new (std::nothrow) Section();
-
-			if (!curSection)
-				fatal(&where, lineNo, "Failed to alloc new area: %s", strerror(errno));
+			std::unique_ptr<Section> curSection = std::make_unique<Section>();
 
 			getToken(line.data(), "'A' line is too short");
 			assert(strlen(token) != 0); // This should be impossible, tokens are non-empty
@@ -271,8 +268,6 @@ void sdobj_ReadFile(FileStackNode const &where, FILE *file, std::vector<Symbol>
 			}
 			char const *sectionName = token; // We'll deal with the section's name depending on type
 
-			fileSections.push_back({.section = curSection, .writeIndex = 0});
-
 			expectToken("size", 'A');
 
 			getToken(nullptr, "'A' line is too short");
@@ -345,6 +340,8 @@ void sdobj_ReadFile(FileStackNode const &where, FILE *file, std::vector<Symbol>
 			curSection->isAlignFixed = false;       // No such concept!
 			curSection->fileSymbols = &fileSymbols; // IDs are instead per-section
 			curSection->nextu = nullptr;
+
+			fileSections.push_back({.section = std::move(curSection), .writeIndex = 0});
 			break;
 		}
 
@@ -371,7 +368,7 @@ void sdobj_ReadFile(FileStackNode const &where, FILE *file, std::vector<Symbol>
 			if (int32_t value = parseNumber(where, lineNo, &token[3], numberType);
 			    !fileSections.empty()) {
 				// Symbols in sections are labels; their value is an offset
-				Section *section = fileSections.back().section;
+				Section *section = fileSections.back().section.get();
 				if (section->isAddressFixed) {
 					assert(value >= section->org && value <= section->org + section->size);
 					value -= section->org;
@@ -479,7 +476,7 @@ void sdobj_ReadFile(FileStackNode const &where, FILE *file, std::vector<Symbol>
 				    fileSections.size()
 				);
 			assert(!fileSections.empty()); // There should be at least one, from the above check
-			Section *section = fileSections[areaIdx].section;
+			Section *section = fileSections[areaIdx].section.get();
 			uint16_t *writeIndex = &fileSections[areaIdx].writeIndex;
 			uint8_t writtenOfs = ADDR_SIZE; // Bytes before this have been written to `->data`
 			uint16_t addr = data[0] | data[1] << 8;
@@ -826,7 +823,7 @@ void sdobj_ReadFile(FileStackNode const &where, FILE *file, std::vector<Symbol>
 	nbSectionsToAssign += fileSections.size();
 
 	for (FileSection &entry : fileSections) {
-		Section *section = entry.section;
+		std::unique_ptr<Section> &section = entry.section;
 
 		// RAM sections can have a size, but don't get any data (they shouldn't have any)
 		if (entry.writeIndex != section->size && entry.writeIndex != 0)
@@ -839,13 +836,14 @@ void sdobj_ReadFile(FileStackNode const &where, FILE *file, std::vector<Symbol>
 			    section->size
 			);
 
-		sect_AddSection(*section);
-
 		if (section->modifier == SECTION_FRAGMENT) {
 			// Add the fragment's offset to all of its symbols
 			for (Symbol *symbol : section->symbols)
 				symbol->label().offset += section->offset;
 		}
+
+		// Calling `sect_AddSection` invalidates the contents of `fileSections`!
+		sect_AddSection(std::move(section));
 	}
 
 #undef expectEol
diff --git a/src/link/section.cpp b/src/link/section.cpp
index 3a1056dd..a04e886c 100644
--- a/src/link/section.cpp
+++ b/src/link/section.cpp
@@ -5,6 +5,7 @@
 #include <assert.h>
 #include <inttypes.h>
 #include <map>
+#include <memory>
 #include <stdlib.h>
 #include <string.h>
 #include <string>
@@ -14,7 +15,7 @@
 
 #include "link/main.hpp"
 
-std::map<std::string, Section *> sections;
+std::map<std::string, std::unique_ptr<Section>> sections;
 
 void sect_ForEach(void (*callback)(Section &)) {
 	for (auto &it : sections)
@@ -139,53 +140,53 @@ static void checkFragmentCompat(Section &target, Section &other) {
 	}
 }
 
-static void mergeSections(Section &target, Section &other, SectionModifier mod) {
+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(
 		    "Section \"%s\" is defined with conflicting types %s and %s",
-		    other.name.c_str(),
+		    other->name.c_str(),
 		    sectionTypeInfo[target.type].name.c_str(),
-		    sectionTypeInfo[other.type].name.c_str()
+		    sectionTypeInfo[other->type].name.c_str()
 		);
 
-	if (other.isBankFixed) {
+	if (other->isBankFixed) {
 		if (!target.isBankFixed) {
 			target.isBankFixed = true;
-			target.bank = other.bank;
-		} else if (target.bank != other.bank) {
+			target.bank = other->bank;
+		} else if (target.bank != other->bank) {
 			errx(
 			    "Section \"%s\" is defined with conflicting banks %" PRIu32 " and %" PRIu32,
-			    other.name.c_str(),
+			    other->name.c_str(),
 			    target.bank,
-			    other.bank
+			    other->bank
 			);
 		}
 	}
 
 	switch (mod) {
 	case SECTION_UNION:
-		checkSectUnionCompat(target, other);
-		if (other.size > target.size)
-			target.size = other.size;
+		checkSectUnionCompat(target, *other);
+		if (other->size > target.size)
+			target.size = other->size;
 		break;
 
 	case SECTION_FRAGMENT:
-		checkFragmentCompat(target, other);
+		checkFragmentCompat(target, *other);
 		// Append `other` to `target`
 		// Note that the order in which fragments are stored in the `nextu` list does not
 		// really matter, only that offsets are properly computed
-		other.offset = target.size;
-		target.size += other.size;
+		other->offset = target.size;
+		target.size += other->size;
 		// Normally we'd check that `sect_HasData`, but SDCC areas may be `_INVALID` here
-		if (!other.data.empty()) {
-			target.data.insert(target.data.end(), RANGE(other.data));
+		if (!other->data.empty()) {
+			target.data.insert(target.data.end(), RANGE(other->data));
 			// Adjust patches' PC offsets
-			for (Patch &patch : other.patches)
-				patch.pcOffset += other.offset;
+			for (Patch &patch : other->patches)
+				patch.pcOffset += other->offset;
 		} else if (!target.data.empty()) {
-			assert(other.size == 0);
+			assert(other->size == 0);
 		}
 		break;
 
@@ -193,39 +194,39 @@ static void mergeSections(Section &target, Section &other, SectionModifier mod)
 		unreachable_();
 	}
 
-	other.nextu = target.nextu;
-	target.nextu = &other;
+	other->nextu = std::move(target.nextu);
+	target.nextu = std::move(other);
 }
 
-void sect_AddSection(Section &section) {
+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 *other = sect_GetSection(section->name); other) {
+		if (section->modifier != other->modifier)
 			errx(
 			    "Section \"%s\" defined as %s and %s",
-			    section.name.c_str(),
-			    sectionModNames[section.modifier],
+			    section->name.c_str(),
+			    sectionModNames[section->modifier],
 			    sectionModNames[other->modifier]
 			);
-		else if (section.modifier == SECTION_NORMAL)
-			errx("Section name \"%s\" is already in use", section.name.c_str());
+		else if (section->modifier == SECTION_NORMAL)
+			errx("Section name \"%s\" is already in use", section->name.c_str());
 		else
-			mergeSections(*other, section, section.modifier);
-	} else if (section.modifier == SECTION_UNION && sect_HasData(section.type)) {
+			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",
-		    section.name.c_str(),
-		    sectionTypeInfo[section.type].name.c_str()
+		    section->name.c_str(),
+		    sectionTypeInfo[section->type].name.c_str()
 		);
 	} else {
 		// If not, add it
-		sections[section.name] = &section;
+		sections.emplace(section->name, std::move(section));
 	}
 }
 
 Section *sect_GetSection(std::string const &name) {
 	auto search = sections.find(name);
-	return search != sections.end() ? search->second : nullptr;
+	return search != sections.end() ? search->second.get() : nullptr;
 }
 
 static void doSanityChecks(Section &section) {