From 447c561aaa8cc9cbe5fec16923e5f3237e11cc64 Mon Sep 17 00:00:00 2001
From: Sylvie <35663410+Rangi42@users.noreply.github.com>
Date: Sun, 3 Mar 2024 21:16:36 -0500
Subject: [PATCH] Use `std::variant` for symbol values (#1330)

---
 include/link/symbol.hpp | 20 +++++++++-------
 src/link/object.cpp     | 38 ++++++++++++++++++------------
 src/link/output.cpp     |  8 +++++--
 src/link/patch.cpp      | 16 ++++++++-----
 src/link/sdas_obj.cpp   | 52 ++++++++++++++++++++++++++++++-----------
 src/link/symbol.cpp     |  3 +++
 6 files changed, 92 insertions(+), 45 deletions(-)

diff --git a/include/link/symbol.hpp b/include/link/symbol.hpp
index 20f4fab2..b625f228 100644
--- a/include/link/symbol.hpp
+++ b/include/link/symbol.hpp
@@ -8,12 +8,20 @@
 
 #include <stdint.h>
 #include <string>
+#include <variant>
 
 #include "linkdefs.hpp"
 
 struct FileStackNode;
 struct Section;
 
+struct Label {
+	int32_t sectionID;
+	int32_t offset;
+	// Extra info computed during linking
+	Section *section;
+};
+
 struct Symbol {
 	// Info contained in the object files
 	std::string name;
@@ -21,14 +29,10 @@ struct Symbol {
 	char const *objFileName;
 	FileStackNode const *src;
 	int32_t lineNo;
-	int32_t sectionID;
-	union {
-		// Both types must be identical
-		int32_t offset;
-		int32_t value;
-	};
-	// Extra info computed during linking
-	Section *section;
+	std::variant<
+		int32_t, // Constants just have a numeric value
+		Label // Label values refer to an offset within a specific section
+	> data;
 };
 
 void sym_AddSymbol(Symbol &symbol);
diff --git a/src/link/object.cpp b/src/link/object.cpp
index 864b85ac..c24f55da 100644
--- a/src/link/object.cpp
+++ b/src/link/object.cpp
@@ -178,18 +178,28 @@ static void readSymbol(FILE *file, Symbol &symbol, char const *fileName,
 	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());
 		symbol.src = &fileNodes[nodeID];
 		tryReadlong(symbol.lineNo, file, "%s: Cannot read \"%s\"'s line number: %s",
 			    fileName, symbol.name.c_str());
-		tryReadlong(symbol.sectionID, file, "%s: Cannot read \"%s\"'s section ID: %s",
+		int32_t sectionID, value;
+		tryReadlong(sectionID, file, "%s: Cannot read \"%s\"'s section ID: %s",
 			    fileName, symbol.name.c_str());
-		tryReadlong(symbol.offset, file, "%s: Cannot read \"%s\"'s value: %s",
+		tryReadlong(value, file, "%s: Cannot read \"%s\"'s value: %s",
 			    fileName, symbol.name.c_str());
+		if (sectionID == -1) {
+			symbol.data = value;
+		} else {
+			symbol.data = Label{
+				.sectionID = sectionID,
+				.offset = value,
+				// Set the `.section` later based on the `.sectionID`
+				.section = nullptr,
+			};
+		}
 	} else {
-		symbol.sectionID = -1;
+		symbol.data = -1;
 	}
 }
 
@@ -330,11 +340,13 @@ static void readSection(FILE *file, Section &section, char const *fileName,
 static void linkSymToSect(Symbol &symbol, Section &section)
 {
 	uint32_t a = 0, b = section.symbols.size();
+	int32_t symbolOffset = std::get<Label>(symbol.data).offset;
 
 	while (a != b) {
 		uint32_t c = (a + b) / 2;
+		int32_t otherOffset = std::get<Label>(section.symbols[c]->data).offset;
 
-		if (section.symbols[c]->offset > symbol.offset)
+		if (otherOffset > symbolOffset)
 			b = c;
 		else
 			a = c + 1;
@@ -452,8 +464,8 @@ void obj_ReadFile(char const *fileName, unsigned int fileID)
 
 		if (symbol.type == SYMTYPE_EXPORT)
 			sym_AddSymbol(symbol);
-		if (symbol.sectionID != -1)
-			nbSymPerSect[symbol.sectionID]++;
+		if (Label *label = std::get_if<Label>(&symbol.data); label)
+			nbSymPerSect[label->sectionID]++;
 	}
 
 	// This file's sections, stored in a table to link symbols to them
@@ -484,12 +496,8 @@ void obj_ReadFile(char const *fileName, unsigned int fileID)
 
 	// Give symbols' section pointers to their sections
 	for (uint32_t i = 0; i < nbSymbols; i++) {
-		int32_t sectionID = fileSymbols[i].sectionID;
-
-		if (sectionID == -1) {
-			fileSymbols[i].section = nullptr;
-		} else {
-			Section *section = fileSections[sectionID];
+		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);
@@ -497,10 +505,10 @@ void obj_ReadFile(char const *fileName, unsigned int fileID)
 			if (section->modifier != SECTION_NORMAL) {
 				if (section->modifier == SECTION_FRAGMENT)
 					// Add the fragment's offset to the symbol's
-					fileSymbols[i].offset += section->offset;
+					label->offset += section->offset;
 				section = getMainSection(*section);
 			}
-			fileSymbols[i].section = section;
+			label->section = section;
 		}
 	}
 
diff --git a/src/link/output.cpp b/src/link/output.cpp
index 2ad57c88..c9e0cb7b 100644
--- a/src/link/output.cpp
+++ b/src/link/output.cpp
@@ -345,7 +345,10 @@ static void writeSymBank(SortedSections const &bankSections, enum SectionType ty
 		for (Symbol const *sym : sect->symbols) {
 			// Don't output symbols that begin with an illegal character
 			if (!sym->name.empty() && canStartSymName(sym->name[0]))
-				symList.push_back({ .sym = sym, .addr = (uint16_t)(sym->offset + sect->org) });
+				symList.push_back({
+					.sym = sym,
+					.addr = (uint16_t)(std::get<Label>(sym->data).offset + sect->org)
+				});
 		}
 	});
 
@@ -414,7 +417,8 @@ static void writeMapBank(SortedSections const &sectList, enum SectionType type,
 				for (Symbol *sym : sect->symbols)
 					// Space matches "\tSECTION: $xxxx ..."
 					fprintf(mapFile, "\t         $%04" PRIx32 " = %s\n",
-						sym->offset + org, sym->name.c_str());
+						std::get<Label>(sym->data).offset + org,
+						sym->name.c_str());
 
 				if (sect->nextu) {
 					// Announce the following "piece"
diff --git a/src/link/patch.cpp b/src/link/patch.cpp
index 489d5732..86c76360 100644
--- a/src/link/patch.cpp
+++ b/src/link/patch.cpp
@@ -6,6 +6,7 @@
 #include <limits.h>
 #include <stdlib.h>
 #include <string.h>
+#include <variant>
 
 #include "link/object.hpp"
 #include "link/patch.hpp"
@@ -13,6 +14,7 @@
 #include "link/symbol.hpp"
 
 #include "error.hpp"
+#include "helpers.hpp"
 #include "linkdefs.hpp"
 #include "opmath.hpp"
 #include "platform.hpp"
@@ -226,14 +228,14 @@ static int32_t computeRPNExpr(Patch const &patch, std::vector<Symbol> const &fil
 				      fileSymbols[value].name.c_str());
 				isError = true;
 				value = 1;
-			} else if (!symbol->section) {
+			} else if (!std::holds_alternative<Label>(symbol->data)) {
 				error(patch.src, patch.lineNo,
 				      "Requested BANK() of non-label symbol \"%s\"",
 				      fileSymbols[value].name.c_str());
 				isError = true;
 				value = 1;
 			} else {
-				value = symbol->section->bank;
+				value = std::get<Label>(symbol->data).section->bank;
 			}
 			break;
 
@@ -386,10 +388,12 @@ static int32_t computeRPNExpr(Patch const &patch, std::vector<Symbol> const &fil
 					      "Unknown symbol \"%s\"", fileSymbols[value].name.c_str());
 					isError = true;
 				} else {
-					value = symbol->value;
-					// Symbols attached to sections have offsets
-					if (symbol->section)
-						value += symbol->section->org;
+					value = std::visit(Visitor{
+						[](int32_t val) -> int32_t { return val; },
+						[](Label label) -> int32_t {
+							return label.section->org + label.offset;
+						}
+					}, symbol->data);
 				}
 			}
 			break;
diff --git a/src/link/sdas_obj.cpp b/src/link/sdas_obj.cpp
index 3abc78ea..f481a167 100644
--- a/src/link/sdas_obj.cpp
+++ b/src/link/sdas_obj.cpp
@@ -8,9 +8,12 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <tuple>
+#include <variant>
 #include <vector>
 
 #include "linkdefs.hpp"
+#include "helpers.hpp"
 #include "platform.hpp"
 
 #include "link/assign.hpp"
@@ -334,19 +337,29 @@ void sdobj_ReadFile(FileStackNode const &where, FILE *file, std::vector<Symbol>
 			symbol.src = &where;
 			symbol.lineNo = lineNo;
 
-			// No need to set the `sectionID`, since we can directly set the pointer
-			symbol.section = !fileSections.empty() ? fileSections.back().section : nullptr;
-
 			getToken(line.data(), "'S' line is too short");
 			symbol.name = token;
 
 			getToken(nullptr, "'S' line is too short");
-			// It might be an `offset`, but both types are the same so type punning is fine
-			symbol.value = parseNumber(where, lineNo, &token[3], numberType);
-			if (symbol.section && symbol.section->isAddressFixed) {
-				assert(symbol.offset >= symbol.section->org);
-				symbol.offset -= symbol.section->org;
-				assert(symbol.offset <= symbol.section->size);
+
+			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;
+				if (section->isAddressFixed) {
+					assert(value >= section->org &&
+					       value <= section->org + section->size);
+					value -= section->org;
+				}
+				symbol.data = Label{
+					// No need to set the `sectionID`, since we set the pointer
+					.sectionID = 0,
+					.offset = value,
+					.section = section
+				};
+			} else {
+				// Symbols without sections are just constants
+				symbol.data = value;
 			}
 
 			// Expected format: /[DR]ef[0-9A-F]+/i
@@ -363,13 +376,24 @@ void sdobj_ReadFile(FileStackNode const &where, FILE *file, std::vector<Symbol>
 				if (other) {
 					// The same symbol can only be defined twice if neither
 					// definition is in a floating section
-					if ((other->section && !other->section->isAddressFixed)
-					 || (symbol.section && !symbol.section->isAddressFixed)) {
+					auto visitor = Visitor{
+						[](int32_t value) -> std::tuple<Section *, int32_t> {
+							return {nullptr, value};
+						},
+						[](Label label) -> std::tuple<Section *, int32_t> {
+							return {label.section, label.offset};
+						}
+					};
+					auto [symbolSection, symbolValue] = std::visit(visitor, symbol.data);
+					auto [otherSection, otherValue] = std::visit(visitor, other->data);
+					
+					if ((otherSection && !otherSection->isAddressFixed)
+					    || (symbolSection && !symbolSection->isAddressFixed)) {
 						sym_AddSymbol(symbol); // This will error out
-					} else if (other->value != symbol.value) {
+					} else if (otherValue != symbolValue) {
 						error(&where, lineNo,
 						      "Definition of \"%s\" conflicts with definition in %s (%" PRId32 " != %" PRId32 ")",
-						      symbol.name.c_str(), other->objFileName, symbol.value, other->value);
+						      symbol.name.c_str(), other->objFileName, symbolValue, otherValue);
 					}
 				} else {
 					// Add a new definition
@@ -691,7 +715,7 @@ void sdobj_ReadFile(FileStackNode const &where, FILE *file, std::vector<Symbol>
 		if (section->modifier == SECTION_FRAGMENT) {
 			// Add the fragment's offset to all of its symbols
 			for (Symbol *symbol : section->symbols)
-				symbol->offset += section->offset;
+				std::get<Label>(symbol->data).offset += section->offset;
 		}
 	}
 
diff --git a/src/link/symbol.cpp b/src/link/symbol.cpp
index 2819e24a..0e7adeab 100644
--- a/src/link/symbol.cpp
+++ b/src/link/symbol.cpp
@@ -4,12 +4,15 @@
 #include <map>
 #include <stdlib.h>
 #include <string>
+#include <variant>
 
 #include "link/object.hpp"
+#include "link/section.hpp"
 #include "link/symbol.hpp"
 #include "link/main.hpp"
 
 #include "error.hpp"
+#include "helpers.hpp"
 
 std::map<std::string, Symbol *> symbols;