Repository navigation
fix(cabi): classify unaligned SysV aggregates as memory - #2941
Merged
cpunion merged 5 commits intoNov 11, 2026
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
Collaborator
Author
|
Investigated the failed workers job: Local validation on macOS arm64:
The original failed job has been rerun. It is currently queued for the |
cpunion
marked this pull request as ready for review
November 2, 2026 00:45
cpunion
commented
Nov 2, 2026
cpunion
left a comment
cpunion
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed 12b66e968. The ordinary packed, nested, array-stride, and aligned-control cases behave as intended. Full go test -count=1 ./internal/cabi ./ssa ./cl passed on macOS arm64 / LLVM 22.1.8. I also checked complete signature lowering and LLVM verification for 11 layouts on each of Linux/Darwin AMD64, compared their MEMORY/register classification with Clang, and verified the caller's byval storage alignment.
One remaining compatibility case is documented inline: an unaligned zero-length array member. It also fails on the PR's base, so this is an uncovered case in the fix, not a newly introduced regression.
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
FennoAI Review
This PR correctly fixes a real SysV AMD64 ABI bug: aggregates with unaligned (packed) fields are now classified as MEMORY (byval pointer) instead of being flattened into registers, which would otherwise lose the packed member offsets. I independently verified the correctness of the change:
- The
hasUnalignedFieldrecursion anduint64offset arithmetic are sound. Recursion terminates (by-value self-reference is impossible in LLVM; pointers end the descent), the zero-size guard is correct, and the modulo atarch.go:186cannot divide by zero (ABITypeAlignmentis always ≥ 1 for sized types). - The array branch probing only element 0 and element 1 is provably sufficient, not just a heuristic: for a fixed internal field, the per-element alignment residues form an arithmetic progression modulo a power of two, so if elements 0 and 1 are both aligned, every element is. The
"array stride"test case ([2 x <{i32, i8}>]) specifically exercises this path. - The comments accurately describe the SysV ABI behavior, and no public-facing docs need updating.
The notes below are minor/optional — none block the change.
Minor (not inline):
hasUnalignedField's array branch makes two recursive calls per array level (element 0 and element 1), so deeply nested arrays-of-arrays cost2^Dcalls with no memoization. This is on the compile-time ABI path and bounded by array nesting depth, so practical impact is small, but it is an avoidable asymptotic cost relative to the linear walkers (elementTypesCount/elementTypes) already in this file.- Optional test coverage: every MEMORY fixture has alignment 1, so the
max(8, info.Align)upper branch is never exercised; and no case assertsinfo.Type1 == PointerType(typ, 0). Adding an over-aligned packed fixture and a pointer-shape assertion would lock in the full MEMORY classification.
cpunion
force-pushed
the
codex/sysv-unaligned-layout-20261029
branch
from
November 2, 2026 01:07
12b66e9 to
9a847ea
Compare
Collaborator
Author
|
Also addressed the non-inline notes in the review, in
Full cabi/ssa/cl tests and cabi vet pass. The changed alignment helpers and MEMORY helper have 100% local statement coverage in the focused AMD64/SysV tests. |
cpunion
marked this pull request as draft
November 2, 2026 02:07
LLGo baseline benchmarks
String equality benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
cpunion
force-pushed
the
codex/sysv-unaligned-layout-20261029
branch
from
November 10, 2026 05:56
965f9d1 to
724423b
Compare
cpunion
marked this pull request as ready for review
November 10, 2026 13:15
Contributor
There was a problem hiding this comment.
Review summary
Reviewed the SysV AMD64 ABI lowering for unaligned/packed aggregates and zero-sized members. This is a well-engineered, focused change: the MEMORY classification of packed/unaligned aggregates is correct, the checkFieldAlignment recursion threads offsets correctly through nested structs and arrays, and the array-stride optimization keeps nested-array classification at O(depth). Test coverage is excellent — unit tests, a benchmark guarding the O(depth) path, and a host interop test that differentially compares the lowered IR (and, on matching hosts, runtime behavior) against real Clang output.
No correctness, security, or documentation-accuracy issues were found. The azure.archive.ubuntu.com CI mirror fallback is benign (an official Canonical mirror; APT trust still rests on the preserved signing keys), and the clang-driven host test uses only test-controlled temp paths with no shell injection surface.
The two inline notes below are minor, non-blocking suggestions.
Minor (no inline anchor):
internal/cabi/arch.go—checkFieldAlignment's doc comment says "For an aligned layout, it also returns the maximum member alignment," but on the unaligned path it returns a partial max (struct iteration returns early). It's safe because the only caller (hasUnalignedField) discards the alignment whenunalignedis true, but a one-line note that the alignment is only meaningful when the second result isfalsewould prevent future misuse.
cpunion
force-pushed
the
codex/sysv-unaligned-layout-20261029
branch
from
November 10, 2026 23:21
724423b to
2959cb0
Compare
zhouguangyuan0718
approved these changes
Nov 11, 2026
cpunion
force-pushed
the
codex/sysv-unaligned-layout-20261029
branch
from
November 11, 2026 09:20
2959cb0 to
9da449e
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Classify SysV AMD64 aggregates with unaligned members as MEMORY (
sretresults andbyvalarguments), including nested records, array strides and zero-sized members. Aligned controls retain register passing.Cache aggregate classifications within each lowering pass. A 32-function shared-type benchmark improves from about 245µs to 79µs, adding approximately 336 bytes and four allocations per module.
Rebased onto main
2681caa07. Cabi/SSA suites, focused CL ABI tests and Linux/Darwin Clang signature comparisons pass. Native amd64 C execution remains CI qualification. Source-level packing directives and cgo layout recovery remain in #2937.