Repository navigation
feat(structs): add explicit storage alignment markers - #3029
Closed
zhouguangyuan0718 wants to merge 7 commits into
Closed
zhouguangyuan0718 wants to merge 7 commits into
zhouguangyuan0718 wants to merge 7 commits into
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: explicit alignment markers (structs.Align8/16/32/64)
This is a well-engineered, carefully-tested change. It generalizes the existing SIMD/vector layout machinery to also serve user-requested explicit alignment, and the soundness-critical paths are all covered:
- Storage alignment is propagated through stack, global, heap (
new()→ aligned allocator), slice growth, interface boxes, channel buffers, and reflection allocations, with direct test coverage inssa/alignment_test.go,internal/build/alignment_test.go, andtest/go/explicit_alignment_llgo_test.go. requiredExplicitAlignmentrecursion terminates: it follows only by-value storage (array/struct/tuple), never pointers/slices/maps/chans, so recursive Go types cannot loop.- The
panic/recoverincl/compile.goconverts only*llssa.AlignmentErrorto a returned error and re-panics everything else, so unrelated panics are not masked. - Integer casts for DWARF (
debugAlignof→uint32(... * 8)) are bounded by 64 bytes (512 bits), well within range. - The
NeedExportFilemode change (build.go/load.go) is sound: llgo type-checks the patched source itself, so skipping cmd/go export-data compilation for packages with added public declarations does not weaken type safety, and the cache-key fallback keeps output identity correct.
I reviewed across code quality, performance, security/soundness, and documentation accuracy. No correctness, security, performance, or documentation defects were found. The documentation (README + package doc + updated comments) matches the implemented behavior and test expectations.
Below are a few small, non-blocking suggestions. Everything here is optional.
| // before our initial check. | ||
| // Explicit export-data requests and ordinary loads keep their contract. | ||
| if dedup == nil || dedup.rewrite == nil { | ||
| if dedup == nil || dedup.rewrite == nil && len(dedup.llgoFiles) == 0 { |
Contributor
There was a problem hiding this comment.
Nit (readability): dedup == nil || dedup.rewrite == nil && len(dedup.llgoFiles) == 0 mixes || and && without parentheses. The precedence is correct (&& binds tighter, so it reads as dedup == nil || (dedup.rewrite == nil && len(dedup.llgoFiles) == 0)), but this is the only unparenthesized mix of the two operators in the diff. Adding explicit parentheses would guard against a future editing mistake:
if dedup == nil || (dedup.rewrite == nil && len(dedup.llgoFiles) == 0) {| if ll.IsFunctionVarArg() { | ||
| for _, arg := range args { | ||
| if requiredExplicitAlignment(arg.raw.Type) != 0 { | ||
| panic(&AlignmentError{Pos: sig.Params().At(sig.Params().Len() - 1).Pos(), Type: arg.raw.Type}) |
Contributor
There was a problem hiding this comment.
The vararg path reports the __llgo_va_list parameter's position rather than the offending argument's position (the fixed param/result path in type.go uses the exact tuple.At(i).Pos()). This is a reasonable fallback since ellipsis actuals have no declaration slot / token.Pos, but the choice isn't obvious at the call site. Consider a one-line comment noting the va_list parameter position is used deliberately because the offending actual has no source position — the existing comment above explains why the check is here but not why this Pos.
| @@ -713,6 +717,19 @@ func (p Program) toNamed(raw *types.Named) Type { | |||
| break | |||
| } | |||
Contributor
There was a problem hiding this comment.
This explicit-alignment carrier block is near-identical to the SIMD block just below (both do StructCreateNamed → StructSetBody([]llvm.Type{carrier}, false) → &aType{ll, rawType{raw}, vkStruct} → p.named[name] = typ, differing only in the alignment source). A small shared helper (e.g. p.alignmentCarrierStruct(name, raw, n)) would collapse the duplicated construction to a one-line difference and reduce the chance the two paths drift. Optional.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
String equality 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 |
xushiwei
added
llgo-only
Contributor
|
For llgo-specific extensions, the use of |
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.
Add LLGo extensions
structs.Align8,Align16,Align32, andAlign64for zero-sized alignment fields. For example,struct { _ structs.Align16; Data [24]byte }has alignment 16, size 32, and data offset 0. Requirements propagate through value fields and arrays, compose withHostLayout, and remain independent of SIMD representation.Connect source layout, LLVM storage, allocation alignment, reflection metadata, and DWARF. Migrate 32-bit memory-profile counter storage to
structs.Align8, retaining the existing fallback for other Go compilers and older toolchains. Load source-patched packages without requesting unused gc export data, so the metadata driver does not reject LLGo-only public declarations before LLGo type checking.Validate native signatures, callbacks, exports, and concrete C varargs before LLVM lowering, returning positioned compilation errors without package-level panic recovery.
The initial implementation rejects map keys/values aligned above 8 bytes and explicitly aligned native by-value arguments/results, including C varargs. Pointers remain supported; libffi reflection calls still reject alignment above 8 bytes. These markers are LLGo extensions, not APIs provided by other Go compilers.
Validation:
structstests, and C pointer layout/read-write checks.Other targets received layout/IR checks, not native runtime qualification.