Repository navigation
fix: pad trailing zero-sized Go struct fields on amd64 and arm64 - #2860
Open
MeteorsLiu wants to merge 2 commits into
Open
MeteorsLiu wants to merge 2 commits into
MeteorsLiu wants to merge 2 commits into
Conversation
MeteorsLiu
changed the title
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
This PR fixes storage/ABI handling for structs with a trailing zero-sized field. The change is well-reasoned and the two paths (SSA storage layout via padZeroSizedStructTail and the runtime FFI call type via padFFIZeroSizedStructTail) agree on when to add tail storage.
Verified correct
- Build tags are exact complements —
windows && 386is the precise negation of!windows || !386(De Morgan): no overlap, no gap. - No out-of-bounds indexing —
padZeroSizedStructTailis only reached from thedefaultbranch, after thecase native || len(fields) == 0: returnguard, sofields[len(fields)-1]is always valid.padFFIZeroSizedStructTailshort-circuits onlen(st.Fields) != 0before indexing. - No uintptr underflow —
typ.Size_ - offis only reached whenalign(off, Align_) < typ.Size_, which (sincealign(x,n) >= x) impliesoff < typ.Size_, so the result is strictly positive. - SSA/FFI layout consistency — both sides use equivalent predicates:
{int64; struct{}}gains a trailing word on both;{float64; float32; struct{}}gains nothing on both (preserving call classification). The no-op Windows/386 FFI variant is consistent with the 386 SSA path, which routes through the field-layout conversion rather thanpadZeroSizedStructTail.
Note
The main actionable item is missing test coverage for the new default-target path (see inline). No performance concerns and no memory-safety defects were found in the diff.
| // the object. Existing tail padding, as in { float64; float32; struct{} }, | ||
| // already provides that space; adding a field there could change call ABI | ||
| // classification even though the size remains 16. | ||
| func (p Program) padZeroSizedStructTail(fields []llvm.Type) []llvm.Type { |
Contributor
There was a problem hiding this comment.
[P2] No test covers the new default-target zero-sized-tail path
This is the actual bug fix (the default, non-386/non-wasm-profile path), yet it has no test. All existing struct-layout tests in ssa/abitype_layout_test.go construct NewProgram(&Target{GOOS: "windows", GOARCH: "386"}), which takes the case goarch == "386" branch and never exercises padZeroSizedStructTail. The subtest "final zero-sized field has a distinct address" checks the same property, but only through the 386 path.
Suggest adding a test on e.g. linux/amd64 asserting: (a) {int64; struct{}} has size 16 with the trailing field addressable inside the object, and (b) {float64; float32; struct{}} stays size 16 with no added tail byte — locking in the "do not change call classification" invariant. CLAUDE.md states "All bug fixes and features MUST include tests."
| return p.padZeroSizedStructTail(fields), nil | ||
| } | ||
|
|
||
| // Both cases above continue through the existing field-layout conversion. |
Contributor
There was a problem hiding this comment.
[P3] Switch fall-through comment is subtle
"Both cases above continue through the existing field-layout conversion" is correct but relies on the reader knowing that the empty case goarch == "386": and case goarch == "wasm" ...: bodies simply exit the switch (Go cases don't fall through) and reach the code below. Consider making this explicit, e.g. "The 386 and wasm-profile cases have empty bodies and continue below through the existing field-layout conversion." Readability nit only — the control flow is correct.
| ) | ||
|
|
||
| // padFFIZeroSizedStructTail accounts for storage beyond natural alignment: | ||
| // two pointers followed by struct{} need a third word. Ordinary tail padding, |
Contributor
There was a problem hiding this comment.
[P3] "third word" example is implicitly 64-bit
This file also builds for 32-bit non-Windows targets (e.g. linux/386, linux/arm), where two pointers are 8 bytes and the struct is bumped to 12 — not a 24-byte "third word". The example is correct for 64-bit but target-specific; consider qualifying it as a 64-bit example. Not a correctness issue — the code (align(off, Align_) < Size_) is size-driven and handles both widths.
| "github.com/xgo-dev/llgo/runtime/internal/ffi" | ||
| ) | ||
|
|
||
| // Windows/386 C ABI removes Go field wrappers and their tail padding: |
Contributor
There was a problem hiding this comment.
[P3] "removes Go field wrappers" has no operation in this file
The function returns fields unchanged, relying on toFFIStructType having already skipped the zero-sized field; wrapper handling lives in the ssa layout path, not here. The following sentence "Keep the original field description" accurately states what this code does, so the pairing is understandable — but "removes ... wrappers" describes ABI rationale with no corresponding action in this file. Minor wording only.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
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
1 participant
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.
Fixes #2797 by reserving the physical storage required for ordinary Go structs with trailing zero-sized fields, so compiled access and reflection agree on element sizes and nested field offsets.
The implementation includes:
toLLVMStructBody, with comments distinguishing tail-only correction from the existing 386 and profiled Wasm field-layout conversion.runtime/_test/codegen/trailingzero:expect.txtcovers physical strides, reflection reads/writes, copy guards, nested fields, zero-sized objects, and reflected calls; sourceCHECKdirectives cover LLVM layouts on Linux, macOS, and Windows for amd64/arm64 through the existing runner.MakeFunctests, and LLDB-based layoutlint checks also passed.Reflective indexing, writes, and copying now agree with physical storage without changing the Windows/386 call convention; native execution on other platforms remains for CI.