From 04405fb4442a23f91d1a28959f6652963a469fad Mon Sep 17 00:00:00 2001 From: Sylvie <35663410+Rangi42@users.noreply.github.com> Date: Fri, 22 Mar 2024 13:27:21 -0400 Subject: [PATCH] Use `std::shared_ptr` for fstack nodes (#1371) --- include/asm/fstack.hpp | 7 ++-- include/asm/output.hpp | 4 +-- include/asm/section.hpp | 7 ++-- include/asm/symbol.hpp | 5 +-- src/asm/fstack.cpp | 80 ++++++++++++----------------------------- src/asm/output.cpp | 33 +++++------------ src/asm/rpn.cpp | 1 - src/asm/symbol.cpp | 8 ++--- 8 files changed, 45 insertions(+), 100 deletions(-) diff --git a/include/asm/fstack.hpp b/include/asm/fstack.hpp index 96b0b1b9..4b58e022 100644 --- a/include/asm/fstack.hpp +++ b/include/asm/fstack.hpp @@ -5,6 +5,7 @@ #ifndef RGBDS_ASM_FSTACK_H #define RGBDS_ASM_FSTACK_H +#include #include #include #include @@ -22,12 +23,10 @@ struct FileStackNode { > data; - FileStackNode *parent; // Pointer to parent node, for error reporting + std::shared_ptr parent; // Pointer to parent node, for error reporting // Line at which the parent context was exited; meaningless for the root level uint32_t lineNo; - // If referenced by a Symbol, Section, or Patch's `src`, don't `delete`! - bool referenced = false; // Set only if referenced: ID within the object file, -1 if not output yet uint32_t ID = -1; @@ -50,7 +49,7 @@ extern size_t maxRecursionDepth; struct MacroArgs; void fstk_DumpCurrent(); -FileStackNode *fstk_GetFileStack(); +std::shared_ptr fstk_GetFileStack(); void fstk_AddIncludePath(std::string const &path); void fstk_SetPreIncludeFile(std::string const &path); diff --git a/include/asm/output.hpp b/include/asm/output.hpp index 683d7d81..7db45a32 100644 --- a/include/asm/output.hpp +++ b/include/asm/output.hpp @@ -3,6 +3,7 @@ #ifndef RGBDS_ASM_OUTPUT_H #define RGBDS_ASM_OUTPUT_H +#include #include #include @@ -13,8 +14,7 @@ struct FileStackNode; extern std::string objectName; -void out_RegisterNode(FileStackNode *node); -void out_ReplaceNode(FileStackNode *node); +void out_RegisterNode(std::shared_ptr node); void out_SetFileName(std::string const &name); void out_CreatePatch(uint32_t type, Expression const &expr, uint32_t ofs, uint32_t pcShift); void out_CreateAssert( diff --git a/include/asm/section.hpp b/include/asm/section.hpp index fe670c21..9348836c 100644 --- a/include/asm/section.hpp +++ b/include/asm/section.hpp @@ -4,6 +4,7 @@ #define RGBDS_SECTION_H #include +#include #include #include #include @@ -18,7 +19,7 @@ struct FileStackNode; struct Section; struct Patch { - FileStackNode const *src; + std::shared_ptr src; uint32_t lineNo; uint32_t offset; Section *pcSection; @@ -31,8 +32,8 @@ struct Section { std::string name; SectionType type; SectionModifier modifier; - FileStackNode const *src; // Where the section was defined - uint32_t fileLine; // Line where the section was defined + std::shared_ptr src; // Where the section was defined + uint32_t fileLine; // Line where the section was defined uint32_t size; uint32_t org; uint32_t bank; diff --git a/include/asm/symbol.hpp b/include/asm/symbol.hpp index 6ddb77a2..0ceaed6c 100644 --- a/include/asm/symbol.hpp +++ b/include/asm/symbol.hpp @@ -3,6 +3,7 @@ #ifndef RGBDS_SYMBOL_H #define RGBDS_SYMBOL_H +#include #include #include #include @@ -31,8 +32,8 @@ struct Symbol { bool isExported; // Whether the symbol is to be exported bool isBuiltin; // Whether the symbol is a built-in Section *section; - FileStackNode *src; // Where the symbol was defined - uint32_t fileLine; // Line where the symbol was defined + std::shared_ptr src; // Where the symbol was defined + uint32_t fileLine; // Line where the symbol was defined std::variant< int32_t, // If isNumeric() diff --git a/src/asm/fstack.cpp b/src/asm/fstack.cpp index 64199324..70c389f2 100644 --- a/src/asm/fstack.cpp +++ b/src/asm/fstack.cpp @@ -7,7 +7,6 @@ #include #include #include -#include #include #include #include @@ -25,7 +24,7 @@ #include "asm/warning.hpp" struct Context { - FileStackNode *fileInfo; + std::shared_ptr fileInfo; LexerState lexerState{}; uint32_t uniqueID = 0; MacroArgs *macroArgs = nullptr; // Macro args are *saved* here @@ -96,17 +95,8 @@ void fstk_DumpCurrent() { contextStack.top().fileInfo->dump(lexer_GetLineNo()); } -FileStackNode *fstk_GetFileStack() { - if (contextStack.empty()) - return nullptr; - - FileStackNode *topNode = contextStack.top().fileInfo; - - // Mark node and all of its parents as referenced if not already so they don't get freed - for (FileStackNode *node = topNode; node && !node->referenced; node = node->parent) - node->referenced = true; - - return topNode; +std::shared_ptr fstk_GetFileStack() { + return contextStack.empty() ? nullptr : contextStack.top().fileInfo; } void fstk_AddIncludePath(std::string const &path) { @@ -173,14 +163,10 @@ bool yywrap() { if (Context &context = contextStack.top(); context.fileInfo->type == NODE_REPT) { // The context is a REPT or FOR block, which may loop - // If the node is referenced, we can't edit it; duplicate it - if (context.fileInfo->referenced) { - context.fileInfo = new (std::nothrow) FileStackNode(*context.fileInfo); - if (!context.fileInfo) - fatalerror("Failed to duplicate REPT file node: %s\n", strerror(errno)); - // Copy all info but the referencing - context.fileInfo->referenced = false; - context.fileInfo->ID = -1; + // If the node is referenced outside this context, we can't edit it, so duplicate it + if (context.fileInfo.use_count() > 1) { + context.fileInfo = std::make_shared(*context.fileInfo); + context.fileInfo->ID = -1; // The copy is not yet registered } std::vector &fileInfoIters = context.fileInfo->iters(); @@ -212,6 +198,7 @@ bool yywrap() { contextStack.pop(); lexer_CleanupState(oldContext.lexerState); + // Restore args if a macro (not REPT) saved them if (oldContext.fileInfo->type == NODE_MACRO) macro_UseNewArgs(contextStack.top().macroArgs); @@ -220,9 +207,6 @@ bool yywrap() { if (oldContext.fileInfo->type == NODE_REPT || oldContext.fileInfo->type == NODE_MACRO || contextStack.top().uniqueID > 0) macro_SetUniqueID(contextStack.top().uniqueID); - // Free the file stack node - if (!oldContext.fileInfo->referenced) - delete oldContext.fileInfo; lexer_SetState(&contextStack.top().lexerState); @@ -232,17 +216,17 @@ bool yywrap() { // Make sure not to switch the lexer state before calling this, so the saved line no is correct. // BE CAREFUL! This modifies the file stack directly, you should have set up the file info first. // Callers should set `contextStack.top().lexerState` after this so it is not `nullptr`. -static Context &newContext(FileStackNode &fileInfo) { +static Context &newContext(std::shared_ptr fileInfo) { if (contextStack.size() > maxRecursionDepth) fatalerror("Recursion limit (%zu) exceeded\n", maxRecursionDepth); // Save the current `\@` value, to be restored when this context ends contextStack.top().uniqueID = macro_GetUniqueID(); - fileInfo.parent = contextStack.top().fileInfo; - fileInfo.lineNo = lexer_GetLineNo(); + fileInfo->parent = contextStack.top().fileInfo; + fileInfo->lineNo = lexer_GetLineNo(); - return contextStack.emplace(Context{.fileInfo = &fileInfo}); + return contextStack.emplace(Context{.fileInfo = fileInfo}); } void fstk_RunInclude(std::string const &path) { @@ -258,15 +242,10 @@ void fstk_RunInclude(std::string const &path) { return; } - FileStackNode *fileInfo = new (std::nothrow) FileStackNode(NODE_FILE, *fullPath); - if (!fileInfo) { - error("Failed to alloc file info for INCLUDE: %s\n", strerror(errno)); - return; - } - uint32_t uniqueID = contextStack.top().uniqueID; - Context &context = newContext(*fileInfo); + auto fileInfo = std::make_shared(NODE_FILE, *fullPath); + Context &context = newContext(fileInfo); if (!lexer_OpenFile(context.lexerState, fileInfo->name())) fatalerror("Failed to set up lexer for file include\n"); lexer_SetStateAtEOL(&context.lexerState); @@ -288,13 +267,8 @@ static void runPreIncludeFile() { return; } - FileStackNode *fileInfo = new (std::nothrow) FileStackNode(NODE_FILE, *fullPath); - if (!fileInfo) { - error("Failed to alloc file info for pre-include: %s\n", strerror(errno)); - return; - } - - Context &context = newContext(*fileInfo); + auto fileInfo = std::make_shared(NODE_FILE, *fullPath); + Context &context = newContext(fileInfo); if (!lexer_OpenFile(context.lexerState, fileInfo->name())) fatalerror("Failed to set up lexer for file include\n"); lexer_SetState(&context.lexerState); @@ -315,15 +289,11 @@ void fstk_RunMacro(std::string const ¯oName, MacroArgs &args) { } contextStack.top().macroArgs = macro_GetCurrentArgs(); - FileStackNode *fileInfo = new (std::nothrow) FileStackNode(NODE_MACRO, ""); - if (!fileInfo) { - error("Failed to alloc file info for \"%s\": %s\n", macro->name.c_str(), strerror(errno)); - return; - } + auto fileInfo = std::make_shared(NODE_MACRO, ""); // Print the name... std::string &fileInfoName = fileInfo->name(); - for (FileStackNode const *node = macro->src; node; node = node->parent) { + for (FileStackNode const *node = macro->src.get(); node; node = node->parent.get()) { if (node->type != NODE_REPT) { fileInfoName.append(node->name()); break; @@ -340,7 +310,7 @@ void fstk_RunMacro(std::string const ¯oName, MacroArgs &args) { fileInfoName.append("::"); fileInfoName.append(macro->name); - Context &context = newContext(*fileInfo); + Context &context = newContext(fileInfo); std::string_view *macroView = macro->getMacro(); lexer_OpenFileView( context.lexerState, "MACRO", macroView->data(), macroView->size(), macro->fileLine @@ -351,11 +321,7 @@ void fstk_RunMacro(std::string const ¯oName, MacroArgs &args) { } static bool newReptContext(int32_t reptLineNo, char const *body, size_t size) { - FileStackNode *fileInfo = new (std::nothrow) FileStackNode(NODE_REPT, std::vector{1}); - if (!fileInfo) { - error("Failed to alloc file info for REPT: %s\n", strerror(errno)); - return false; - } + auto fileInfo = std::make_shared(NODE_REPT, std::vector{1}); if (contextStack.top().fileInfo->type == NODE_REPT && !contextStack.top().fileInfo->iters().empty()) { @@ -365,7 +331,7 @@ static bool newReptContext(int32_t reptLineNo, char const *body, size_t size) { ); } - Context &context = newContext(*fileInfo); + Context &context = newContext(fileInfo); // Correct our line number, which currently points to the `ENDR` line context.fileInfo->lineNo = reptLineNo; lexer_OpenFileView(context.lexerState, "REPT", body, size, reptLineNo); @@ -453,9 +419,7 @@ void fstk_Init(std::string const &mainPath, size_t maxDepth) { fatalerror("Failed to open main file\n"); lexer_SetState(&context.lexerState); - context.fileInfo = new (std::nothrow) FileStackNode(NODE_FILE, context.lexerState.path); - if (!context.fileInfo) - fatalerror("Failed to allocate memory for main file info: %s\n", strerror(errno)); + context.fileInfo = std::make_shared(NODE_FILE, context.lexerState.path); // lineNo and nbReptIters are unused on the top-level context context.fileInfo->parent = nullptr; context.fileInfo->lineNo = 0; // This still gets written to the object file, so init it diff --git a/src/asm/output.cpp b/src/asm/output.cpp index fa82f95c..618dfc5a 100644 --- a/src/asm/output.cpp +++ b/src/asm/output.cpp @@ -34,7 +34,7 @@ static std::vector objectSymbols; static std::deque assertions; -static std::deque fileStackNodes; +static std::deque> fileStackNodes; // Write a long to a file (little-endian) static void putlong(uint32_t n, FILE *file) { @@ -53,7 +53,7 @@ static void putstring(std::string const &s, FILE *file) { putc('\0', file); } -void out_RegisterNode(FileStackNode *node) { +void out_RegisterNode(std::shared_ptr node) { // If node is not already registered, register it (and parents), and give it a unique ID for (; node && node->ID == (uint32_t)-1; node = node->parent) { node->ID = fileStackNodes.size(); @@ -61,21 +61,6 @@ void out_RegisterNode(FileStackNode *node) { } } -void out_ReplaceNode(FileStackNode * /* node */) { -#if 0 -This is code intended to replace a node, which is pretty useless until ref counting is added... - - auto search = std::find(RANGE(fileStackNodes), node); - assert(search != fileStackNodes.end()); - // The list is supposed to have decrementing IDs; catch inconsistencies early - assert(search->ID == node->ID); - assert(search + 1 == fileStackNodes.end() || (search + 1)->ID == node->ID - 1); - - // TODO: unreference the node - *search = node; -#endif -} - // Return a section's ID, or -1 if the section is not in the list static uint32_t getSectIDIfAny(Section *sect) { if (!sect) @@ -249,12 +234,10 @@ static void writerpn(std::vector &rpnexpr, std::vector const & } static void initpatch(Patch &patch, uint32_t type, Expression const &expr, uint32_t ofs) { - FileStackNode *node = fstk_GetFileStack(); - patch.type = type; - patch.src = node; + patch.src = fstk_GetFileStack(); // All patches are assumed to eventually be written, so the file stack node is registered - out_RegisterNode(node); + out_RegisterNode(patch.src); patch.lineNo = lexer_GetLineNo(); patch.offset = ofs; patch.pcSection = sect_GetSymbolSection(); @@ -342,17 +325,17 @@ void out_WriteObject() { putlong(fileStackNodes.size(), file); for (auto it = fileStackNodes.begin(); it != fileStackNodes.end(); it++) { - FileStackNode const *node = *it; + FileStackNode const &node = **it; - writeFileStackNode(*node, file); + writeFileStackNode(node, file); // The list is supposed to have decrementing IDs - if (it + 1 != fileStackNodes.end() && it[1]->ID != node->ID - 1) + if (it + 1 != fileStackNodes.end() && it[1]->ID != node.ID - 1) fatalerror( "Internal error: fstack node #%" PRIu32 " follows #%" PRIu32 ". Please report this to the developers!\n", it[1]->ID, - node->ID + node.ID ); } diff --git a/src/asm/rpn.cpp b/src/asm/rpn.cpp index 5ad88522..7f9e781d 100644 --- a/src/asm/rpn.cpp +++ b/src/asm/rpn.cpp @@ -6,7 +6,6 @@ #include #include #include -#include #include #include #include diff --git a/src/asm/symbol.cpp b/src/asm/symbol.cpp index a62f0c73..49f2e068 100644 --- a/src/asm/symbol.cpp +++ b/src/asm/symbol.cpp @@ -97,15 +97,13 @@ static void dumpFilename(Symbol const &sym) { // Update a symbol's definition filename and line static void updateSymbolFilename(Symbol &sym) { - FileStackNode *oldSrc = sym.src; - + std::shared_ptr oldSrc = std::move(sym.src); sym.src = fstk_GetFileStack(); sym.fileLine = sym.src ? lexer_GetLineNo() : 0; - // If the old node was referenced, ensure the new one is - if (oldSrc && oldSrc->referenced && oldSrc->ID != (uint32_t)-1) + // If the old node was registered, ensure the new one is too + if (oldSrc && oldSrc->ID != (uint32_t)-1) out_RegisterNode(sym.src); - // TODO: unref the old node, and use `out_ReplaceNode` instead of deleting it } // Create a new symbol by name