Refactor section creation errors, and simplify output for single errors #1964
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| #include <iterator> | ||
| #include <optional> | ||
| #include <stack> | ||
| #include <stdarg.h> | ||
| #include <stdint.h> | ||
| #include <stdio.h> | ||
| #include <stdlib.h> | ||
|
|
@@ -114,16 +115,29 @@ Section *sect_FindSectionByName(std::string const &name) { | |
| return index ? §ions[*index] : nullptr; | ||
| } | ||
|
|
||
| static std::vector<std::string> sectErrors; | ||
|
|
||
| void sectError(char const *fmt, ...) { | ||
| std::string result; | ||
| va_list args1, args2; | ||
| va_start(args1, fmt); | ||
| va_copy(args2, args1); | ||
| int len = vsnprintf(nullptr, 0, fmt, args1); | ||
| va_end(args1); | ||
| if (len < 0) { | ||
| // LCOV_EXCL_START | ||
| va_end(args2); | ||
| fatal("Error describing the error that occurred when merging a section"); | ||
| // LCOV_EXCL_STOP | ||
| } else if (len > 0) { | ||
| result.resize(len); | ||
| vsnprintf(result.data(), len + 1, fmt, args2); | ||
| } | ||
| va_end(args2); | ||
| sectErrors.push_back(result); | ||
| } | ||
|
Comment on lines
+120
to
+138
Member
There was a problem hiding this comment. I really hate the logic of this function, and would rather use
Contributor
Author
There was a problem hiding this comment. Can we rely on Even without
Member
There was a problem hiding this comment. Please. As for availability, I have no idea.
Contributor
Author
There was a problem hiding this comment. We could use a template<typename... Args>
void sectError(char const *fmt, Args... args) {
std::string result;
int len = snprintf(nullptr, 0, fmt, std::forward<Args>(args)...);
if (len < 0) {
fatal("Error describing the error that occurred when merging a section"); // LCOV_EXCL_LINE
} else if (len > 0) {
result.resize(len);
snprintf(result.data(), len + 1, fmt, std::forward<Args>(args)...);
}
sectErrors.push_back(result);
}...however, Switching to |
||
|
|
||
| static void mergeSectUnion(Section §, uint32_t org, uint8_t alignment, uint16_t alignOffset) { | ||
| assume(alignment < 16); // Should be ensured by the caller | ||
| uint32_t alignSize = 1u << alignment; | ||
| uint32_t alignMask = alignSize - 1; | ||
|
|
@@ -136,11 +150,15 @@ static unsigned int | |
| // If both are fixed, they must be the same | ||
| if (sect.org != UINT32_MAX && sect.org != org) { | ||
| sectError( | ||
| "Section \"%s\" already declared as fixed at different address $%04" PRIx32, | ||
| sect.name.c_str(), | ||
| sect.org | ||
| ); | ||
| } else if (sect.align != 0 && ((org - sect.alignOfs) & sectAlignMask)) { | ||
| sectError( | ||
| "Section \"%s\" already declared as aligned to %" PRIu32 " bytes (offset %" PRIu16 | ||
| ")", | ||
| sect.name.c_str(), | ||
| sectAlignSize, | ||
| sect.alignOfs | ||
| ); | ||
|
|
@@ -154,15 +172,17 @@ static unsigned int | |
| if (sect.org != UINT32_MAX) { | ||
| if ((sect.org - alignOffset) & alignMask) { | ||
| sectError( | ||
| "Section \"%s\" already declared as fixed at incompatible address $%04" PRIx32, | ||
| sect.name.c_str(), | ||
| sect.org | ||
| ); | ||
| } | ||
| // Check if alignment offsets are compatible | ||
| } else if ((alignOffset & sectAlignMask) != (sect.alignOfs & alignMask)) { | ||
| sectError( | ||
| "Section \"%s\" already declared with incompatible %" PRIu32 | ||
| "-byte alignment (offset %" PRIu16 ")", | ||
| sect.name.c_str(), | ||
| sectAlignSize, | ||
| sect.alignOfs | ||
| ); | ||
|
|
@@ -172,14 +192,9 @@ static unsigned int | |
| sect.alignOfs = alignOffset; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| static void mergeFragments(Section §, uint32_t org, uint8_t alignment, uint16_t alignOffset) { | ||
| assume(alignment < 16); // Should be ensured by the caller | ||
| uint32_t alignSize = 1u << alignment; | ||
| uint32_t alignMask = alignSize - 1; | ||
|
|
@@ -197,11 +212,15 @@ static unsigned int | |
| // If both are fixed, they must be the same | ||
| if (sect.org != UINT32_MAX && sect.org != curOrg) { | ||
| sectError( | ||
| "Section \"%s\" already declared as fixed at incompatible address $%04" PRIx32, | ||
| sect.name.c_str(), | ||
| sect.org | ||
| ); | ||
| } else if (sect.align != 0 && ((curOrg - sect.alignOfs) & sectAlignMask)) { | ||
| sectError( | ||
| "Section \"%s\" already declared as aligned to %" PRIu32 " bytes (offset %" PRIu16 | ||
| ")", | ||
| sect.name.c_str(), | ||
| sectAlignSize, | ||
| sect.alignOfs | ||
| ); | ||
|
|
@@ -215,15 +234,17 @@ static unsigned int | |
| if (uint32_t curOfs = (alignOffset - sect.size) & alignMask; sect.org != UINT32_MAX) { | ||
| if ((sect.org - curOfs) & alignMask) { | ||
| sectError( | ||
| "Section \"%s\" already declared as fixed at incompatible address $%04" PRIx32, | ||
| sect.name.c_str(), | ||
| sect.org | ||
| ); | ||
| } | ||
| // Check if alignment offsets are compatible | ||
| } else if ((curOfs & sectAlignMask) != (sect.alignOfs & alignMask)) { | ||
| sectError( | ||
| "Section \"%s\" already declared with incompatible %" PRIu32 | ||
| "-byte alignment (offset %" PRIu16 ")", | ||
| sect.name.c_str(), | ||
| sectAlignSize, | ||
| sect.alignOfs | ||
| ); | ||
|
|
@@ -233,8 +254,6 @@ static unsigned int | |
| sect.alignOfs = curOfs; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| static void mergeSections( | ||
|
|
@@ -246,59 +265,88 @@ static void mergeSections( | |
| uint16_t alignOffset, | ||
| SectionModifier mod | ||
| ) { | ||
| sectErrors.clear(); | ||
|
|
||
| auto sectAlreadyDefinedCallback = [§]() { | ||
| fprintf(stderr, "Section \"%s\" already defined\n", sect.name.c_str()); | ||
| fstk_TraceCurrent(); | ||
| fputs(" and also:\n", stderr); | ||
| sect.src->printBacktrace(sect.fileLine); | ||
| }; | ||
| auto sectAlreadyDefinedError = []() { | ||
| // The empty string is a sentinel for the `sectAlreadyDefinedCallback` error, | ||
| // since it cannot be preformatted as a string | ||
| sectErrors.push_back(""); | ||
| }; | ||
|
|
||
| if (type != sect.type) { | ||
| sectError( | ||
| "Section \"%s\" already exists but with type `%s`", | ||
| sect.name.c_str(), | ||
| sectionTypeInfo[sect.type].name.c_str() | ||
| ); | ||
| } | ||
|
|
||
| if (sect.modifier != mod) { | ||
| sectError( | ||
| "Section \"%s\" already declared as `SECTION %s`", | ||
| sect.name.c_str(), | ||
| sectionModNames[sect.modifier] | ||
| ); | ||
| } else { | ||
| switch (mod) { | ||
| case SECTION_UNION: | ||
| case SECTION_FRAGMENT: { | ||
| void (*merge)(Section &, uint32_t, uint8_t, uint16_t) = | ||
| mod == SECTION_UNION ? mergeSectUnion : mergeFragments; | ||
| merge(sect, org, alignment, alignOffset); | ||
|
|
||
| // If the section's bank is unspecified, override it | ||
| if (sect.bank == UINT32_MAX) { | ||
| sect.bank = bank; | ||
| } | ||
| // If both specify a bank, it must be the same one | ||
| else if (bank != UINT32_MAX && sect.bank != bank) { | ||
| sectError( | ||
| "Section \"%s\" already declared with different bank %" PRIu32, | ||
| sect.name.c_str(), | ||
| sect.bank | ||
| ); | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| case SECTION_NORMAL: | ||
| sectAlreadyDefinedError(); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (size_t nbSectErrors = sectErrors.size(); nbSectErrors == 1) { | ||
| // If there was only one error, print it as a fatal error | ||
| if (std::string const &message = sectErrors[0]; message.empty()) { | ||
| fatalNoTrace(sectAlreadyDefinedCallback); | ||
| } else { | ||
| fatal("%s", message.c_str()); | ||
| } | ||
| } else if (nbSectErrors > 1) { | ||
| // If there were multiple errors, print each of them, followed by a fatal summary error | ||
| for (std::string const &message : sectErrors) { | ||
| if (message.empty()) { | ||
| errorNoTrace(sectAlreadyDefinedCallback); | ||
| } else { | ||
| error("%s", message.c_str()); | ||
| } | ||
| } | ||
| fatal( | ||
| "Cannot create section \"%s\" (%zu error%s)", | ||
| sect.name.c_str(), | ||
| nbSectErrors, | ||
| nbSectErrors == 1 ? "" : "s" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| static Section *createSection( | ||
| std::string const &name, | ||
| SectionType type, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,4 @@ | ||
| FATAL: Section "dup" already defined | ||
| at duplicate-section-after-literal.asm(7) | ||
| and also: | ||
| at duplicate-section-after-literal.asm(5) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,4 @@ | ||
| FATAL: Section "sec" already defined | ||
| at duplicate-section.asm(4) | ||
| and also: | ||
| at duplicate-section.asm(2) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,2 @@ | ||
| FATAL: Section "aligned" already declared as fixed at incompatible address $c002 | ||
| at fragment-align-mismatch.asm(2) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,2 @@ | ||
| FATAL: Section "test" already declared as fixed at incompatible address $0000 | ||
| at fragment-mismatch.asm(2) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,2 @@ | ||
| FATAL: Section "Test" already declared with incompatible 256-byte alignment (offset 0) | ||
| at incompatible-alignment.asm(8) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| SECTION FRAGMENT "test", ROM0[$0000] | ||
|
|
||
| SECTION FRAGMENT "test", ROMX[$4000] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| error: Section "test" already exists but with type `ROM0` | ||
| at multiple-section-errors.asm(3) | ||
| error: Section "test" already declared as fixed at incompatible address $0000 | ||
| at multiple-section-errors.asm(3) | ||
| FATAL: Cannot create section "test" (2 errors) | ||
| at multiple-section-errors.asm(3) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,2 @@ | ||
| FATAL: Section "test" already declared as fixed at different address $c000 | ||
| at section-union-mismatch.asm(2) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,2 @@ | ||
| FATAL: Section "test" already declared as `SECTION UNION` | ||
| at section-union.asm(37) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,2 @@ | ||
| FATAL: Section "fixed" already declared as fixed at incompatible address $c001 | ||
| at union-mismatch.asm(2) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use
std::functionunless you have to. Its indirection has a lot of overhead, including in codegen, due to the type-erasure that it performs.If you want a callable that can hold onto state (thus, not just a function pointer), please template this function. (Using concepts to document the expected parameters and return type, of course.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a fatal error; the overhead of a memory allocation compared to passing a function pointer really won't matter. Regardless, I'd be fine with changing the implementation, if we can do so without moving the whole implementation into the .hpp file as a template (and thereby also having to include the appropriate headers, helper functions, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🦀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remain concerned about how much extra C++ standard library this pulls in, the extra headers involved, and all the extra codegen regardless of the runtime cost.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if it can be done another way, but I suspect I chose
std::functionin the first place forerrorNoTracebecause the callbacks need to capture variables, and the function can't/shouldn't be defined as a template in theasm/warning.hppheader file. If the#include <functional>has enough extra codegen to matter, it should be measurable, not just "avoidstd::functionall the time".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would this be solved in Rust? Can
format!args be bundled into aVecof deferred arg packs?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, using
format_args!(); but rather, you don't need to provide declaration separately from implementation and thus there are no header woes anymore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we move
fatalNoTraceinto the warning.hpp header file, we should do the same for the already-existingerrorNoTracefunction, which also takes astd::function<void()> callback. However,errorNoTracecalls thestatic void incrementErrors()function, so then that whole function would have to be non-staticto be visible in the header file. Also the header would have to have#include <stdio.h>and#include "style.hpp"for the functions' implementations. I just doing think avoidingstd::functionis worth that cost.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::functionis likely to have a "small buffer optimization", and sincesectAlreadyDefinedCallbackonly captures one reference-pointer, it's likely to fit in a small buffer and not require an extra heap allocation. Unlike atemplateparameter, it won't be inlined, but that's just the time+space cost of one vtable'd function call. It's not done in some tight loop, it's only done in error conditions, andfatalNoTraceexits immediately so it will only happen at most once. So the runtime+space cost ofstd::functionis low, and the code quality benefit is worthwhile.