From ca4b890273e09395cd98df67a25e55972f90077f Mon Sep 17 00:00:00 2001 From: Rangi42 Date: Tue, 23 Sep 2025 11:53:05 -0400 Subject: [PATCH] Consistently do `& alignMask`, not `% alignSize` Also add more unrelated tests for coverage --- src/asm/section.cpp | 38 +++++++++---------- src/link/layout.cpp | 8 ++-- src/link/section.cpp | 7 +--- test/asm/deprecated-functions.asm | 29 ++++++++++---- test/asm/deprecated-functions.err | 23 +++++++++++ test/asm/incbin-start-bad.asm | 4 ++ test/asm/incbin-start-bad.err | 5 +++ test/link/section-fragment/align-conflict.asm | 10 +++++ test/link/section-fragment/align-conflict.out | 10 +++++ test/link/test.sh | 2 +- 10 files changed, 98 insertions(+), 38 deletions(-) create mode 100644 test/asm/incbin-start-bad.asm create mode 100644 test/asm/incbin-start-bad.err create mode 100644 test/link/section-fragment/align-conflict.asm create mode 100644 test/link/section-fragment/align-conflict.out diff --git a/src/asm/section.cpp b/src/asm/section.cpp index 4a167f9a..42123425 100644 --- a/src/asm/section.cpp +++ b/src/asm/section.cpp @@ -218,14 +218,8 @@ static unsigned int } } else if (alignment != 0) { - int32_t curOfs = (alignOffset - sect.size) % alignSize; - - if (curOfs < 0) { - curOfs += alignSize; - } - // Make sure any fixed address given is compatible - if (sect.org != UINT32_MAX) { + if (uint32_t curOfs = (alignOffset - sect.size) & alignMask; sect.org != UINT32_MAX) { if ((sect.org - curOfs) & alignMask) { sectError( "Section already declared as fixed at incompatible address $%04" PRIx32, @@ -624,10 +618,10 @@ uint32_t sect_GetAlignBytes(uint8_t alignment, uint16_t offset) { return 0; } - // We need `(pcValue + curOffset + return value) % (1 << alignment) == offset` + // We need `(pcValue + curOffset + return value) & minAlignMask == offset` uint16_t pcValue = isFixed ? sect->org : sect->alignOfs; - return static_cast(offset - curOffset - pcValue) - % (1u << std::min(alignment, curAlignment)); + uint32_t minAlignMask = (1u << std::min(alignment, curAlignment)) - 1; + return static_cast(offset - curOffset - pcValue) & minAlignMask; } void sect_AlignPC(uint8_t alignment, uint16_t offset) { @@ -637,13 +631,15 @@ void sect_AlignPC(uint8_t alignment, uint16_t offset) { assume(alignment <= 16); // Should be ensured by the caller uint32_t alignSize = 1u << alignment; + uint32_t alignMask = alignSize - 1; Section *sect = sect_GetSymbolSection(); assume(sect->align <= 16); // Left-shifting by 32 or more would be UB uint32_t sectAlignSize = 1u << sect->align; + uint32_t sectAlignMask = sectAlignSize - 1; if (sect->org != UINT32_MAX) { - if (uint32_t actualOffset = (sect->org + curOffset) % alignSize; actualOffset != offset) { + if (uint32_t actualOffset = (sect->org + curOffset) & alignMask; actualOffset != offset) { error( "Section is misaligned (at PC = $%04" PRIx32 ", expected ALIGN[%" PRIu32 ", %" PRIu32 "], got ALIGN[%" PRIu32 ", %" PRIu32 "])", @@ -654,8 +650,8 @@ void sect_AlignPC(uint8_t alignment, uint16_t offset) { actualOffset ); } - } else if (uint32_t actualOffset = (sect->alignOfs + curOffset) % alignSize; - sect->align != 0 && actualOffset % sectAlignSize != offset % sectAlignSize) { + } else if (uint32_t actualOffset = (sect->alignOfs + curOffset) & alignMask; + sect->align != 0 && (actualOffset & sectAlignMask) != (offset & sectAlignMask)) { error( "Section is misaligned ($%04" PRIx32 " bytes into the section, expected ALIGN[%" PRIu32 ", %" PRIu32 "], got ALIGN[%" PRIu32 ", %" PRIu32 "])", @@ -672,8 +668,8 @@ void sect_AlignPC(uint8_t alignment, uint16_t offset) { sect->org = offset - curOffset; } else if (alignment > sect->align) { sect->align = alignment; - // We need `(sect->alignOfs + curOffset) % alignSize == offset` - sect->alignOfs = (offset - curOffset) % alignSize; + // We need `(sect->alignOfs + curOffset) & alignMask == offset` + sect->alignOfs = (offset - curOffset) & alignMask; } } @@ -948,12 +944,11 @@ bool sect_BinaryFile(std::string const &name, uint32_t startPos) { // The file is seekable; skip to the specified start position fseek(file, startPos, SEEK_SET); } else { + // LCOV_EXCL_START if (errno != ESPIPE) { - // LCOV_EXCL_START error( "Error determining size of `INCBIN` file \"%s\": %s", name.c_str(), strerror(errno) ); - // LCOV_EXCL_STOP } // The file isn't seekable, so we'll just skip bytes one at a time while (startPos--) { @@ -964,6 +959,7 @@ bool sect_BinaryFile(std::string const &name, uint32_t startPos) { return false; } } + // LCOV_EXCL_STOP } for (int byte; (byte = fgetc(file)) != EOF;) { @@ -1013,12 +1009,11 @@ bool sect_BinaryFileSlice(std::string const &name, uint32_t startPos, uint32_t l // The file is seekable; skip to the specified start position fseek(file, startPos, SEEK_SET); } else { + // LCOV_EXCL_START if (errno != ESPIPE) { - // LCOV_EXCL_START error( "Error determining size of `INCBIN` file \"%s\": %s", name.c_str(), strerror(errno) ); - // LCOV_EXCL_STOP } // The file isn't seekable, so we'll just skip bytes one at a time while (startPos--) { @@ -1029,21 +1024,22 @@ bool sect_BinaryFileSlice(std::string const &name, uint32_t startPos, uint32_t l return false; } } + // LCOV_EXCL_STOP } while (length--) { if (int byte = fgetc(file); byte != EOF) { writeByte(byte); - } else if (ferror(file)) { // LCOV_EXCL_START + } else if (ferror(file)) { error("Error reading `INCBIN` file \"%s\": %s", name.c_str(), strerror(errno)); - // LCOV_EXCL_STOP } else { error( "Premature end of `INCBIN` file \"%s\" (%" PRId32 " bytes left to read)", name.c_str(), length + 1 ); + // LCOV_EXCL_STOP } } return false; diff --git a/src/link/layout.cpp b/src/link/layout.cpp index 07f141c8..1e66c05b 100644 --- a/src/link/layout.cpp +++ b/src/link/layout.cpp @@ -128,6 +128,7 @@ void layout_AlignTo(uint32_t alignment, uint32_t alignOfs) { layout_SetAddr(floatingAlignOffset); } else { uint32_t alignSize = 1u << alignment; + uint32_t alignMask = alignSize - 1; if (alignOfs >= alignSize) { scriptError( @@ -139,8 +140,8 @@ void layout_AlignTo(uint32_t alignment, uint32_t alignOfs) { return; } - floatingAlignMask = alignSize - 1; - floatingAlignOffset = alignOfs % alignSize; + floatingAlignMask = alignMask; + floatingAlignOffset = alignOfs & alignMask; } return; } @@ -158,6 +159,7 @@ void layout_AlignTo(uint32_t alignment, uint32_t alignOfs) { if (alignment < 16) { uint32_t alignSize = 1u << alignment; + uint32_t alignMask = alignSize - 1; if (alignOfs >= alignSize) { scriptError( @@ -170,7 +172,7 @@ void layout_AlignTo(uint32_t alignment, uint32_t alignOfs) { } assume(pc >= typeInfo.startAddr); - length %= alignSize; + length &= alignMask; } if (uint16_t offset = pc - typeInfo.startAddr; length > typeInfo.size - offset) { diff --git a/src/link/section.cpp b/src/link/section.cpp index 5a116675..10146714 100644 --- a/src/link/section.cpp +++ b/src/link/section.cpp @@ -54,7 +54,7 @@ static void checkAgainstFixedAddress(Section const &target, Section const &other } } -static bool checkAgainstFixedAlign(Section const &target, Section const &other, int32_t ofs) { +static bool checkAgainstFixedAlign(Section const &target, Section const &other, uint32_t ofs) { if (target.isAddressFixed) { if ((target.org - ofs) & other.alignMask) { fatalTwoAt( @@ -107,10 +107,7 @@ static void checkFragmentCompat(Section &target, Section &other) { target.isAddressFixed = true; target.org = org; } else if (other.isAlignFixed) { - int32_t ofs = (other.alignOfs - target.size) % (other.alignMask + 1); - if (ofs < 0) { - ofs += other.alignMask + 1; - } + uint32_t ofs = (other.alignOfs - target.size) & other.alignMask; if (checkAgainstFixedAlign(target, other, ofs)) { target.isAlignFixed = true; target.alignMask = other.alignMask; diff --git a/test/asm/deprecated-functions.asm b/test/asm/deprecated-functions.asm index bd574431..143dd859 100644 --- a/test/asm/deprecated-functions.asm +++ b/test/asm/deprecated-functions.asm @@ -4,12 +4,25 @@ def s equs "Hello world!" assert strin(#s, "l") == strfind(#s, "l") + 1 assert strrin(#s, "l") == strrfind(#s, "l") + 1 -assert !strcmp(strsub(#s, 7), strslice(#s, 6)) -assert !strcmp(strsub(#s, 7, 5), strslice(#s, 6, 11)) -assert !strcmp(strsub(#s, strlen(#s), 1), strslice(#s, strlen(#s) - 1, strlen(#s))) -assert !strcmp(strsub(#s, 7, 999), strslice(#s, 6, 999)) +assert strsub(#s, 7) === strslice(#s, 6) +assert strsub(#s, 7, 5) === strslice(#s, 6, 11) +assert strsub(#s, strlen(#s), 1) === strslice(#s, strlen(#s) - 1, strlen(#s)) +assert strsub(#s, 7, 999) === strslice(#s, 6, 999) -assert !strcmp(charsub(#s, 12), strchar(#s, 11)) -assert !strcmp(charsub(#s, -1), strchar(#s, -1)) -assert !strcmp(charsub(#s, -999), strchar(#s, -999)) -assert !strcmp(charsub(#s, 999), strchar(#s, 999)) +assert charsub(#s, 12) === strchar(#s, 11) +assert charsub(#s, -1) === strchar(#s, -1) +assert charsub(#s, -999) === strchar(#s, -999) +assert charsub(#s, 999) === strchar(#s, 999) + +; characters: +; 1: U+0061 a +; 2: U+00E4 a with diaresis (0xC3 0xA4) +; 3: U+0062 b +; 4: invalid byte 0xA3 +; 5: U+0063 c +; 6: incomplete U+6F22 kanji (0xE6 0xBC without 0xA2) +def invalid equs "aäb£cæ¼" + +def copy equs strsub(#invalid, 1) +def past equs strsub(#invalid, 9, 1) +def incomplete equs strsub(#invalid, 12, 1) diff --git a/test/asm/deprecated-functions.err b/test/asm/deprecated-functions.err index 425f6488..6835a0e7 100644 --- a/test/asm/deprecated-functions.err +++ b/test/asm/deprecated-functions.err @@ -30,3 +30,26 @@ warning: CHARSUB: Position 999 is past the end of the string [-Wbuiltin-args] at deprecated-functions.asm(15) warning: STRCHAR: Index 999 is past the end of the string [-Wbuiltin-args] at deprecated-functions.asm(15) +warning: `STRSUB` is deprecated; use 0-indexed `STRSLICE` instead [-Wobsolete] + at deprecated-functions.asm(26) +error: STRSUB: Invalid UTF-8 byte 0xA3 + at deprecated-functions.asm(26) +error: STRSUB: Incomplete UTF-8 character + at deprecated-functions.asm(26) +warning: `STRSUB` is deprecated; use 0-indexed `STRSLICE` instead [-Wobsolete] + at deprecated-functions.asm(27) +error: STRSUB: Invalid UTF-8 byte 0xA3 + at deprecated-functions.asm(27) +warning: STRSUB: Position 9 is past the end of the string [-Wbuiltin-args] + at deprecated-functions.asm(27) +error: STRSUB: Incomplete UTF-8 character + at deprecated-functions.asm(27) +warning: `STRSUB` is deprecated; use 0-indexed `STRSLICE` instead [-Wobsolete] + at deprecated-functions.asm(28) +error: STRSUB: Invalid UTF-8 byte 0xA3 + at deprecated-functions.asm(28) +warning: STRSUB: Position 12 is past the end of the string [-Wbuiltin-args] + at deprecated-functions.asm(28) +error: STRSUB: Incomplete UTF-8 character + at deprecated-functions.asm(28) +Assembly aborted with 6 errors! diff --git a/test/asm/incbin-start-bad.asm b/test/asm/incbin-start-bad.asm new file mode 100644 index 00000000..724f3c88 --- /dev/null +++ b/test/asm/incbin-start-bad.asm @@ -0,0 +1,4 @@ +SECTION "Bad", ROM0 + +INCBIN "data.bin", 999 +INCBIN "data.bin", 999, 1 diff --git a/test/asm/incbin-start-bad.err b/test/asm/incbin-start-bad.err new file mode 100644 index 00000000..7ab6332b --- /dev/null +++ b/test/asm/incbin-start-bad.err @@ -0,0 +1,5 @@ +error: Specified start position is greater than length of file "data.bin" + at incbin-start-bad.asm(3) +error: Specified start position is greater than length of file "data.bin" + at incbin-start-bad.asm(4) +Assembly aborted with 2 errors! diff --git a/test/link/section-fragment/align-conflict.asm b/test/link/section-fragment/align-conflict.asm new file mode 100644 index 00000000..0c44f088 --- /dev/null +++ b/test/link/section-fragment/align-conflict.asm @@ -0,0 +1,10 @@ +IF !DEF(SECOND) + def ATTRS equs ",ALIGN[2]" +ELSE + def ATTRS equs "[$1337]" +ENDC + +SECTION FRAGMENT "conflicting alignment", ROM0 {ATTRS} + db + + PURGE ATTRS diff --git a/test/link/section-fragment/align-conflict.out b/test/link/section-fragment/align-conflict.out new file mode 100644 index 00000000..707a4a5f --- /dev/null +++ b/test/link/section-fragment/align-conflict.out @@ -0,0 +1,10 @@ +FATAL: Section "conflicting alignment" is defined with 4-byte alignment (offset 0), but also with address $1337 + at section-fragment/align-conflict.asm(7) + and also: + at section-fragment/align-conflict.asm(7) +Linking aborted with 1 error +--- +error: Section already declared as aligned to 4 bytes (offset 0) + at (18) +FATAL: Cannot create section "conflicting alignment" (1 error) + at (18) diff --git a/test/link/test.sh b/test/link/test.sh index 0d2ca49f..782176a2 100755 --- a/test/link/test.sh +++ b/test/link/test.sh @@ -401,7 +401,7 @@ tryDiff "$test"/out.err "$outtemp" tryCmpRom "$test"/ref.out.bin evaluateTest -for i in section-union/*.asm; do +for i in section-union/*.asm section-fragment/*.asm; do test=${i%.asm} startTest "$RGBASM" -o "$otemp" "${test}.asm"