Skip to content

Split unit-regression2.cpp so the MinGW linker can relocate it - #5511

Open
nlohmann wants to merge 1 commit into
ubjson-valueless-count-capfrom
split-unit-regression2
Open

Split unit-regression2.cpp so the MinGW linker can relocate it#5511
nlohmann wants to merge 1 commit into
ubjson-valueless-count-capfrom
split-unit-regression2

Conversation

@nlohmann

@nlohmann nlohmann commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Prerequisite for the binary-reader series (#5104, PRs #5501#5508); it does not change library code.

What

Linking test-regression2 with clang and MinGW fails once the translation unit grows past a certain size:

relocation truncated to fit: IMAGE_REL_AMD64_REL32 against `.rdata'

The code can no longer reach the read-only data it references within the range of a 32-bit relocation. unit-regression2.cpp is one of the largest files in the test suite and had been sitting just under that limit, so a change anywhere in the library is enough to tip it over — the iterative binary readers in this series do exactly that, on 9 of 9 clang versions.

Two things worth noting:

  • This is the second time this file has hit a size limit: it exists because unit-regression1.cpp was split for size.
  • windows.yml already carries a workaround for the same limit hitting the same target-DCMAKE_CXX_FLAGS_DEBUG="-g0", because linking it failed with IMAGE_REL_AMD64_SECREL against '.debug_line'. Dropping debug info fixed that relocation. This one is against .rdata, which no compiler flag avoids.

How

Move the second half of the regression tests, and the helper types only they use, into unit-regression3.cpp. The sections are independent — every statement in TEST_CASE("regression tests 2") was already inside a SECTION — so they move unchanged.

Nothing is lost: 168 assertions before the split, 50 + 118 after.

Both files end up comfortably smaller than the one that used to link (clang, -O1, C++20):

read-only data text object
before 58,233 1,287,764 3,158,120
unit-regression2.cpp 48,161 1,012,988 2,522,296
unit-regression3.cpp 41,710 772,704 1,878,880

No CMake change is needed: tests/CMakeLists.txt globs src/unit-*.cpp, so the new file is built for every standard like its siblings.

CONTRIBUTING.md pointed contributors at unit-regression2.cpp for new bug tests; it now points at the smaller file and explains why the two exist, so the split doesn't quietly undo itself.

API impact

No breaking changes. Test-only change; no library code is touched.

Checklist

  • The changes are described in detail, both the what and why.
  • If applicable, an existing issue is referenced.
  • The Code coverage remained at 100%. A test case for every new line of code.
  • If applicable, the documentation is updated.
  • The source code is amalgamated by running make amalgamate.

🤖 Generated with Claude Code

Comment thread .github/CONTRIBUTING.md Outdated
When fixing a bug, edit `unit-regression2.cpp` and add a section referencing the fixed issue.
When fixing a bug, edit `unit-regression3.cpp` and add a section referencing the fixed issue.
`unit-regression2.cpp` holds the older tests; the two files exist because a single one grew large enough for the
MinGW linker to fail relocating it, so please keep adding to the smaller file rather than growing one of them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"growing the larger one"?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix - once the running CI jobs finish.

@nlohmann
nlohmann force-pushed the split-unit-regression2 branch from 3d89ac5 to 408ad8c Compare September 7, 2026 18:40
@nlohmann nlohmann added the review needed It would be great if someone could review the proposed changes. label Sep 7, 2026
@nlohmann
nlohmann force-pushed the split-unit-regression2 branch from 408ad8c to 81f55c5 Compare September 8, 2026 11:13
@nlohmann
nlohmann force-pushed the split-unit-regression2 branch from 81f55c5 to d91dff7 Compare September 9, 2026 08:21
Linking test-regression2 with clang and MinGW fails with

    relocation truncated to fit: IMAGE_REL_AMD64_REL32 against `.rdata'

once the translation unit grows past a certain size: the code can no longer
reach the read-only data it references within the range of a 32-bit
relocation. The file is one of the largest in the test suite and had been
sitting just under that limit, so an unrelated change elsewhere in the
library is enough to tip it over. It is already the second such file --
unit-regression1.cpp was split for size before -- and windows.yml already
carries a workaround for the same limit hitting the debug sections of this
same target, where -g0 was enough because that relocation was against
`.debug_line'. This one is against `.rdata', which no compiler flag avoids.

Move the second half of the regression tests, and the helper types only they
use, into unit-regression3.cpp. The sections are independent -- every
statement in "regression tests 2" was already inside a SECTION -- so they
move unchanged, and the counts confirm nothing was lost: 168 assertions
before the split, 50 plus 118 after.

The result is that both files are comfortably smaller than the one that used
to link, measured with clang at -O1 for C++20:

                        read-only data        text     object
    before                      58,233   1,287,764  3,158,120
    unit-regression2.cpp        48,161   1,012,988  2,522,296
    unit-regression3.cpp        41,710     772,704  1,878,880

No CMake change is needed: tests/CMakeLists.txt globs src/unit-*.cpp, so the
new file is picked up and built for every standard like its siblings.

CONTRIBUTING.md pointed contributors at unit-regression2.cpp for new bug
tests; it now points at the smaller file and says why the two exist, so the
split does not quietly undo itself.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
@nlohmann
nlohmann force-pushed the split-unit-regression2 branch from d91dff7 to a4d9c29 Compare September 10, 2026 15:15
@nlohmann nlohmann added 🚀 ready to merge Ready to merge - just waiting for CI to complete. and removed review needed It would be great if someone could review the proposed changes. labels Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L 🚀 ready to merge Ready to merge - just waiting for CI to complete. tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants