Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
FennoAI review — PR #3048 (WASM ABI closeout)
Reviewed the full diff across code quality, performance, security, and documentation. Overall this is a well-engineered change: the per-profile Wasm layout logic is carefully reasoned, the struct/field/array recursion matches the LLVM storage path for the covered shapes, the source-layout arithmetic has explicit overflow guards, and the ABI-boundary narrowing is guarded by runtime/compile-time range checks. Docs (doc/runtime-source-reuse.md, doc/wasm-proposal.md) and the apt-mirror CI change (ubuntu_apt.py) check out — the size table and prose match the test assertions, and the added Azure mirror is an official Canonical HTTPS endpoint that only reorders mirror lists without touching the signing keyring.
One correctness edge case is worth addressing; the rest are minor, wasm-path-only maintainability notes.
Findings
- (correctness) Top-level named C-tagged array of scalars is not narrowed — see inline comment on
ssa/native_wasm_layout.go. The*types.Arraycase only recurses into the element and never consultshasLegacyNativeTypeLayout(t)the way the*types.Structcase does, so a//llgo:cdecl type Words [2]uintptrreturns(_, false)andunsafe.Sizeofreports the Go-word size (16), while the storage path (hasLegacyNativeTypeLayout→nativeStorageLLVMType) narrows it to[2 x i32](8). All tests exercise arrays only as struct fields/elements, so this shape is uncovered. Either handle the narrowing in the array case or document/assert the invariant that such a type can never reachwithNativeStoragestandalone. - (maintainability, low) Redundant re-traversal on the wasm path — see inline comment on
ssa/native_wasm_layout.go. This only affects wasm-profile compilation (the ordinary path is correctly gated behindusesWideGoStorage()+ thehasNativeTypeMetadataatomic, so there's no regression off-wasm).
Minor / optional (no inline)
Offsetsof(ssa/type.go) evaluatesprog.nativeFields(fields)up to three times (each takes an RLock); the result is invariant for a givenfields, so hoisting it into one local would be cheaper and clearer.- Style nit: the functions touched in
ssa/type.gostill useProgram(unsafe.Pointer(p)), whereas the new file uses the equivalent safe conversionProgram(p)— optional to align while you're here.
Note: I could not compile the ssa/internal/build packages in this environment (CGo requires llvm-c/Core.h, which isn't present), so these findings are from static analysis of the diff and surrounding code.
cpunion
changed the title
zhouguangyuan0718
left a comment
zhouguangyuan0718
left a comment
There was a problem hiding this comment.
One non-blocking maintainability suggestion for keeping source size and offset calculations consistent.
b47da08 to
5dcf127
Compare
|
Confirmed. The secret scan matched only All four review dimensions are now covered:
Most importantly, both of my prior review findings are resolved and now have regression tests. This is a managed-mode task, so I'll return the result as markdown in my final message without publishing a comment. Re-review complete — PR #3048 (WASM ABI closeout)I re-reviewed the updated PR ( Prior findings — resolved ✓
Verification highlights
Residual (low-severity, non-blocking; sharing for context only)
One note: I couldn't compile the |
5dcf127 to
a179320
Compare
|
@fennoai Rebased onto current main 2681caa to resolve the merge conflicts. The Ubuntu mirror fallback is now in main through #3044, so its duplicate CI commit was dropped. All three Wasm implementation/test commits are patch-identical according to git range-diff. Full SSA, the cross-target native-record layout test, runtime ABI audit, and Ubuntu helper tests pass. Please re-review the rebase at a179320. |
|
I re-reviewed the rebase at
I found no blocking issues. The four parallel reviewer agents failed with an API error, so I reviewed the diff myself. I couldn't build or run the packages that need LLVM here, so the test results are the ones you reported, not ones I reproduced. |
There was a problem hiding this comment.
FennoAI re-review: rebase onto 2681caa07 (head a17932084)
I found no blocking issues.
Rebase check
- I ran
git range-diff 7d66e2d8f..5dcf12765 2681caa07..a17932084. All three commits match (=). The dropped commit7d66e2d8fis the Ubuntu mirror CI commit, which is now in main through #3044. - Since the previous base, main changed only the PCLN function-value fallback (
internal/build/funcinfo_table.goand its tests) and the Ubuntu mirror script. Neither touchesssa/type.go, the Wasm layout helpers, the runtime ABI audit, or thewasm-profilefixture, so the rebase does not create a semantic conflict. - The earlier feedback is still addressed in the rebased code:
- Named C-tagged arrays:
wasmNativeLayoutsizes them from their own directive, andtoNamedgives them native storage. - Shared field walk: Sizeof, Alignof and Offsetsof all go through
wasmGoFieldsLayout. - Repeated lookups:
OffsetsofcallsnativeFieldsonly once.
- Named C-tagged arrays:
One optional doc nit (inline): in doc/wasm-proposal.md, the new paragraph has no blank line before the next one, so the two render as a single paragraph.
Note: subagent reviewers were unavailable in this run, so I reviewed the diff directly. I could not build or run the CGo/LLVM-backed packages here. The test results above are the ones you reported.
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
zhouguangyuan0718
left a comment
zhouguangyuan0718
left a comment
There was a problem hiding this comment.
Found a Memory32 correctness regression in conversions between C-tagged arrays and ordinary Go arrays; the inline comment includes a reproducer and exact-base comparison.
1ad8aa4 to
91d488b
Compare
|
@fennoai Please re-review 91d488b, rebased onto current main 166dafa. This addresses the new P1 array-conversion stack overflow with element-wise Go/native representation conversion and existing range checks. Regressions cover conversions, assignments, nested arrays, pointers, callbacks and overflow in the existing Wasm profile jobs. J32/J64/W32 execution and the focused compiler test pass; the broader CL/SSA run is still finishing locally. |
|
Confirmed both minor items. The Both are genuinely minor polish. I'm confident in the overall assessment: the P1 fix is correct and well-tested, with comprehensive regression coverage (value round-trips, aliasing checks, nested arrays, pointers with GC, callbacks, empty arrays, and overflow panics across j32/w32/j64). No blocking issues across any dimension. This is managed mode — I'll deliver the result as markdown in my final message. Re-review complete — PR #3048 (WASM ABI closeout), head
|
|
Validation follow-up for 91d488b: SSA passes, and all CL tests are now covered. The CL run hit its default 10-minute limit after 525 top-level tests with no failures; the remaining 33 top-level tests passed in a separate run with a longer limit. J32/J64/W32 profile execution and official Go js/wasip1 oracles pass. |
|
@fennoai Addressed both optional nits in 33218e1: hoisted the logical source type and array-length queries out of the conversion loops, and clarified the aggregate-tracing comment. The targeted J32/W32 conversion and array alignment/volatile tests pass; vet still matches the baseline. No other implementation changes. Please re-review this small follow-up. |
|
This is exactly the minimal follow-up described — nothing more. Let me verify both nits are correctly addressed:
All three changes are correct, minimal, and behavior-preserving. There are no functional/logic changes — this is pure cleanup of exactly the two optional nits I raised. The diff is self-evidently safe (hoisting loop-invariants out of loops with no side effects, and a comment reword), so a full four-subagent fan-out would be disproportionate for a 3-line-of-logic no-op refactor. I've verified the semantics directly. One small sanity check: confirm The diff confirms it: This is managed mode — I'll return the result as markdown without publishing. Re-review complete — PR #3048, head
|
Fix Memory32 C record and explicitly C-tagged array layout so
unsafe.Sizeof,AlignofandOffsetofagree with physical native storage. Ordinary Go aggregates retain Go64 storage. Go/native array conversions and assignments now convert elements with checked integer narrowing, including nested arrays, pointers and callbacks.Update the J32/J64/W32 runtime audit and document the Go/native function-value difference. Share layout calculations without caching incomplete checker types.
Validation: focused CL and SSA tests, J32/J64/W32 execution checks, O0 array regressions, and O3/full-LTO checks on J32 and native ARM64. Regressions use the existing Wasm profile jobs.
Fixes #2447.