Skip to content

BlockRowType for block row indexing - #1417

Open
Aidan63 wants to merge 1 commit into
HaxeFoundation:masterfrom
Aidan63:block_row_type
Open

Aidan63 wants to merge 1 commit into
HaxeFoundation:masterfrom
Aidan63:block_row_type

Conversation

@Aidan63

@Aidan63 Aidan63 commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

I've introduced a new BlockRowType whose underlying type varies based on the big blocks define. This type is used for indexing into block rows, it is not guaranteed to be able to represent the 1 indexed "number" of lines in a block.

IMMIX_BLOCK_BYTE_COUNT and IMMIX_BLOCK_LINE_COUNT are size_t as they are only used in "size" or pointer offset related code which already uses other variables of this type so I think it's appropriate.

@dimensionscape

dimensionscape commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

This doesn’t compile with HXCPP_GC_BIG_BLOCKS and HX_GC_VERIFY_ALLOC_START: saturating_add(i, IMMIX_HEADER_LINES) in verifyAllocStart() mixes BlockRowType and uint8_t, while the other three call sites wrap the constant in BlockRowType{}.

And, fiixing that call site doesn’t fix the cause. With a type that changes with the define, a mismatch like this only fails in one configuration, so every use of BlockRowType (it appears 20 times in this PR) has to be right in both. A big-blocks CI job wouldn’t have caught this one, since it also needs the verify define, and catching that class reliably would mean building the GC defines at both block sizes. With one type, the row-count code compiles the same way in both configurations, so a mistake like this fails in any build that compiles it, with no extra CI.

It also doesn’t give a more accurate range either bwcause uint8_t still allows 255, and uint16_t allows 509-65535. The limit is IMMIX_USEFUL_LINES, which #1409 checks in debug builds. Narrowing checks also favour one type: with uint16_t everywhere, brace-initialising a uint8_t from a row count is flagged in every build, not just big blocks.

saturating_sub(IMMIX_USEFUL_LINES, destInfo->mUsedRows) returns 0 when mUsedRows is above the limit, so a corrupted count passes silently. #1409’s debug check traps it in debug builds.

One uint16_t costs 8 bytes per block (sizeof(BlockDataInfo) goes from 1568 to 1576 on x64). #1409 already makes the row counts uint16_t, fixes the #1413 build, adds big blocks to CI and checks the limit.

Edit:
@tobil4sk @Aidan63
I would just say that testing needs to get simpler, not harder. #1402 and #1413 both broke big blocks. Building big blocks in CI would have caught #1413, but not #1402: the test suite passes on #1402’s code with big blocks. Only a compile-time check catches that kind of bug, which is what the static_asserts in #1409 do. More define dependent code structure adds a configuration every change has to be tested in.

And this isn’t about one missing CI job. A big blocks job builds one configuration of one test suite but it doesn’t reach the verify paths, the other GC defines, or code the tests don’t run, which is where this error is. Every type that changes with a define adds code that’s only checked in that configuration, and CI can’t keep up with that as defines are added. What keeps testing simple is keeping the code the same in every configuration wherever that’s (essentially) FREE, so every build checks it. Here it costs 8 bytes per block, and it’s what #1409 does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants