Repository navigation
fix(ssa): stabilize local stack slots and synthetic debug locations - #2530
Merged
cpunion merged 10 commits intoSep 14, 2026
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: fix loop-local stack growth + synthetic-builder debug locations
This is a focused, well-executed change. Two independent fixes:
- Hoist stack-local allocas into the entry block (
ssa/memory.go) so alocaldeclared inside a loop reserves one slot per call instead of growing the stack every iteration. The reservation moves to the entry block while zero-initialization (memset) correctly stays at the declaration site inside the loop body — verified by the newTestLocalLoopAllocReservesOneSlotPerCall, including the GC-roots variant. - Track debug location on the
Builder(ssa/di.go,ssa/decl.go,ssa/eh.go,ssa/stmt_builder.go) so synthetic builders inherit a valid function scope, satisfying LLVM's requirement that inlinable calls in a debug-info function carry a!dbglocation.
Correctness / safety: No issues. Entry-block allocas dominate all uses and are always inserted before the first instruction (never past a terminator). Slot reuse is only taken for non-escaping (heap=false) locals — escaping loop-locals go through the heap AllocZ path — so per-call semantics are preserved. Comments accurately describe behavior. VerifyModule passes in the new tests.
Suggestions (non-blocking):
- One inline note below on the per-alloc builder allocation cost.
- Consider adding a debug-info-enabled case to
TestLocalLoopAllocReservesOneSlotPerCall(it currently runs withoutInitDebug), to lock in that the hoisted alloca's!dbginteraction verifies cleanly.
Nice work.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cpunion
force-pushed
the
codex/fix-loop-local-stack-20260908
branch
2 times, most recently
from
September 8, 2026 02:23
85e3775 to
ea25e9d
Compare
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
cpunion
force-pushed
the
codex/fix-loop-local-stack-20260908
branch
from
September 11, 2026 01:48
ea25e9d to
804d14f
Compare
cpunion
changed the title
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
cpunion
force-pushed
the
codex/fix-loop-local-stack-20260908
branch
2 times, most recently
from
September 13, 2026 02:29
0da6c73 to
0401ed9
Compare
Collaborator
Author
|
@fennoai Review again |
Contributor
|
FennoAI is processing your request, please wait a moment... |
Contributor
There was a problem hiding this comment.
Review: stabilize local stack slots and synthetic debug locations
Reviewed the locked range 410af8e9...826e07e5 with four subagents (code quality, performance, security, documentation accuracy). The change is well-constructed and thoroughly tested.
What holds up well
- Alloca hoisting is correct. Reserving each non-escaping local's slot once in the entry block while keeping
zeroinit/memsetat the declaration preserves per-iteration zeroing semantics and fixes unbounded per-iteration stack growth for loop-body locals. Escaping locals still route to the heap path (AllocZ), so shared-slot aliasing cannot occur. - GC-roots interaction is safe.
FirstBasicBlock()resolves to the gcroot entry block, so hoisted allocas land ahead of the terminator in a block dominating the body. Covered byroots=trueinmemory_local_test.go. - Debug-location mirroring is sound. The
diLocationshadow field routes all mutations throughsetDebugLocation/restoreDebugLocation, correctly handling non-debug builders and builders created beforeDebugFunctionattaches the DISubprogram.SetBlockExreapplies the location at the right choke point. - Builder disposal on the
Allocpath is leak-free (unconditionalDispose, no early return between create and dispose).
Minor findings (none blocking) are inline below.
cpunion
force-pushed
the
codex/fix-loop-local-stack-20260908
branch
from
September 14, 2026 05:30
826e07e to
726fc68
Compare
Collaborator
Author
|
@fennoai Review again after the latest main rebase and review-comment updates. |
Contributor
|
FennoAI is processing your request, please wait a moment... |
Contributor
There was a problem hiding this comment.
Re-review after main rebase + review-comment updates
Locked range 5c587435...726fc687. Re-ran all four subagents (code quality, performance, security, documentation).
What changed since my last review (826e07e5): only the two ssa/datastruct.go comments (channel Recv / Select) were reworded. The rest of the PR's own diff — ssa/memory.go, decl.go, di.go, eh.go, stmt_builder.go, tests, and FileCheck fixtures — is unchanged. The large delta versus the old head is the main rebase (overflow/cabi/caller_frame work), which is in both base and head and therefore outside this PR's diff.
Prior findings resolved
- ✅ The
RecvandSelectcomments now correctly state the receive buffers "use function-entry slots and remain valid across StackRestore" and describe the load-before-restore ordering as conservative/consistent rather than a correctness requirement. This matches the code: entry-block (static) allocas are not reclaimed byllvm.stackrestore. Both stale-comment findings from the previous review are addressed.
Re-verified clean
- Alloca hoisting preserves per-iteration zero-init semantics; escaping locals still route to the heap path, so no cross-iteration aliasing.
- StackSave/StackRestore lifetime for channel recv/select buffers is sound; the volatile load-before-restore is preserved, and
selectOpsSlicecorrectly keeps its dynamic alloca stack-save scoped. - Builder disposal is leak-free;
VerifyModulepasses in the debug-enabled tests.
One optional, non-blocking performance nit remains inline. LGTM.
Collaborator
Author
|
Included the independently reviewed #2562 fix after the rebased LTO job timed out in TestLTOPluginRejectsMalformedInterfaceAttributes waiting for opt. All three cases pass locally with the built LLVM 22 plugin and -tags=dev (no skips). The dependency and failed-job link are documented in the body. @fennoai please review the updated head, especially the dependency integration. |
Contributor
|
FennoAI is processing your request, please wait a moment... |
Contributor
There was a problem hiding this comment.
Re-review of the #2562 dependency integration
Locked range 5c587435...fc0ee507. Re-ran all four subagents (code quality, performance, security, documentation).
Delta since my last review (726fc687) is exactly the #2562 fix — two files:
ltoplugin/LLGOInterfaceMethodTypeIDPass.cpp(+3/-1)cl/compile_test.go(+14/-3)
Everything else in the PR is byte-identical to the previously reviewed, clean state.
Assessment of the integration — looks correct
-
invalidMetadata():report_fatal_error→reportFatalUsageError. This is the right fix for the hang.reportFatalUsageErroris LLVM's sanctioned API for invalid user input (it forwards toreport_fatal_error(reason, /*GenCrashDiag=*/false)): it prints the message andexit(1)s with no crash trace, no symbolizer, and no "report an LLVM bug" prompt — which is exactly the diagnostic path that could stalloptbefore. Available in LLVM 21+ (repo targets LLVM 22), andllvm/Support/ErrorHandling.his already included. The[[noreturn]]contract is preserved, so callers that rely on non-return after a validation branch (e.g. theDecl.Methods[*MethodIndex]index right after a bounds check) stay safe. The added comment accurately describes the behavior. -
TestLTOPluginRejectsMalformedInterfaceAttributeshardening. Wrappingoptincontext.WithTimeout(30s)+cmd.WaitDelay = time.Second, failing loudly onctx.Err(), and asserting a real*exec.ExitErrorwithExitCode() == 1(not a crash/signal, not success) is a meaningfully stronger and hang-proof encoding of the fixed contract. The exact exit-1 assertion is appropriate here: the child isoptinvoked directly (process-level exit code, not signal-translated), and the test is dev-gated behind the built LLVM plugin. The substring assertion matches the emitted"invalid interface type-id metadata:"prefix.
Safety / perf: The rejection path stays fail-closed and leak-free (no continued processing of invalid metadata, no attacker-controlled payload echoed in the message, deterministic termination). The change is off any compile hot path, and the timeout only costs anything on the failure path.
Non-blocking note (out of scope): the sibling ltoplugin/LLGOReflectMethodByNamePass.cpp still uses report_fatal_error at two sites, but those report internal invariant failures rather than malformed user input, so the crash-diag semantics are arguably correct there. Not part of this PR; flagging only for a possible future alignment.
No new inline findings. The dependency integration is clean and the earlier findings remain resolved. LGTM.
Collaborator
Author
|
The previously failing Dev LTO GlobalDCE and Go Method Drop job now passes on fc0ee50 after including #2562: https://github.com/xgo-dev/llgo/actions/runs/34812965081/job/103877868707 . The log shows the TestLTOPlugin* group completed successfully instead of hanging in malformed-metadata rejection. Local go test ./ssa -count=1 also passes (8.2s), and fennoai has reviewed the integrated head with no new findings. The rest of the CI matrix and ordinary coverage uploads are still running. |
Collaborator
Author
|
Final validation on fc0ee50: all 64 checks passed, no pending or failed checks, and Codecov confirms all modified coverable lines are covered. The current head has a completed fennoai review with no unresolved threads. The rebased LTO failure is fixed by the explicit #2562 dependency, with the formerly failing job passing: https://github.com/xgo-dev/llgo/actions/runs/34812965081/job/103877868707 . The complete LLGo workflow is https://github.com/xgo-dev/llgo/actions/runs/34812965111 and the benchmark workflow is https://github.com/xgo-dev/llgo/actions/runs/34812965149 . Note that the macOS coverage job passed in 59m45s, close to its 60-minute limit; this run establishes correctness, not that CI duration has comfortable headroom. No timeout increase or retry behavior was added. |
zhouguangyuan0718
approved these changes
Sep 14, 2026
cpunion
added a commit
that referenced
this pull request
Sep 17, 2026
Local Alloc hoists stack-slot reservations into the entry block so a loop-local slot is reserved once per call (issue #80196, PR #2530). The implementation created and disposed a fresh LLVM builder and re-derived the entry-block insertion point on every Alloc. For functions with many locals — e.g. the generated init in test/cmplxdivide.go, which builds a ~1000+-entry complex128 table — this turned local allocation into a severe compile-time regression (~7x slower, timing out the GOROOT daily Windows jobs; issue #2611). Reserve the slots through a single builder cached on the function, anchored at the entry block once and reused for every local Alloc, then disposed in EndBuild. Allocas still land in the entry block (one slot per call) with zeroing emitted at the declaration site each iteration, so the issue #80196 fix and ssa/memory_local_test.go invariants are preserved. Fixes #2611
cpunion
added a commit
that referenced
this pull request
Sep 18, 2026
- Add 90s timeouts and flake classification for fixedbugs/issue79186.go, fixedbugs/issue30041.go, and fixedbugs/issue39541.go on windows/386. - Remove version: go1.27 constraint from index0.go timeout and flake entries so Windows 386 uses the 90s budget across all supported Go releases. - Remove fixedbugs/issue80196.go from xfails: per-call stack-slot semantics for loops fixed the stack overflow in PR #2530 and PR #2613. - Remove fixedbugs/issue75764.go on windows/amd64 from xfails: the deep interface tail-call test now passes reliably.
4 of 8 tasks
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
2 participants
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.
Summary
Reserve each non-escaping Go local's stack slot once in the LLVM entry block while leaving zero-initialization at the declaration. A declaration inside a loop must reuse one slot per function call; previously, emitting
allocain the loop grew the stack on every iteration. The bounded WASI polling test exposed this after roughly 1,300 retries on a 128 KiB Fiber stack.This PR also contains the synthetic-builder debug-location prerequisite originally reviewed in #2529. It preserves source locations across generated blocks and prevents invalid or missing debug locations in synthetic defer paths. #2529 remains review history and is not a separate merge dependency.
Correctness and review follow-up
allocareserves storage;memsetinitializes that storage. Hoisting one does not remove the other. The current FileCheck fixtures assert the typed and aligned entry-block reservation, bind its address to declaration-time initialization, and follow subsequent value use. The loop regression uses two differently sized and aligned slots and verifies each initializer's destination and full byte count.The local-allocation hot path uses a raw LLVM builder rather than allocating a full Go builder and unused debug-scope cache. The builder is disposed immediately after the entry-block allocation.
Validation
The previously validated head
826e07e5e414was based on main410af8e93a1c, which includes the merged #2575 weak-cleanup reentrancy fix. The original seven compiler/test patches remain unchanged according togit range-diff. The follow-up only corrects four comment lines about channel scratch-slot scope and includes the real subprocess error in two failing compatibility assertions; it does not change compiler execution or add retries. That revision changed 20 files, +474/-215, with no workflow changes.The normal LLGo workflow for this head and all other normal checks completed with 64 successes, one expected tag-only release skip, and no failures. Patch coverage is 100%. All seven Windows LLGo jobs passed; the MinGW ARM64 test step completed in 4m22s (28m32s for the job), and the previously interrupted MSVC ARM64 shard-0 test step completed in 5m30s (30m07s for the job).
go test ./ssa -count=1 -timeout=5mpasses locally with Go 1.27 and LLVM 22 (6.2 seconds), including the loop-local allocation and synthetic debug-location regressions. An independent coverage run hits all 30 added coverable lines._testgocompiler/FileCheck group passed locally (123.4 seconds), covering all six changed fixtures. The five changed_testrtfixtures passed in a separately bounded invocation (170.5 seconds):go test ./cl -run '^TestRunAndTestFromTestrt$/^(float2any|gblarray|index|tpmap|unsafe)$' -count=1 -timeout=5m -v. These exercise the rebased compiler and runtime; they are not a claim of an unfiltered local./clpass.826e07e5e414,go test ./ssa ./test/cmd/llgo -count=1 -timeout=5mpasses locally (6.0s and 5.5s respectively).git diff --checkpasses.Previous Windows ARM64 failures
Two earlier Windows ARM64 jobs ended with GitHub's hosted-runner-lost-communication annotation: MSVC shard 0 on
0da6c73dc2be, then MinGW ARM64 on0401ed90c589. The failed-job archives are unavailable. The preserved MinGW live log showed empty compiler-subprocess output and native Gocgo.exe: exit status 2before the runner disappeared, but no panic, native stack, or OOM diagnostic. This establishes runner loss, not a specific test timeout, weak deadlock, or compiler fault.The first single-job fork diagnosis passed the full LLGo tests plus 20 TLS repetitions, ten isolated repetitions of the affected tool-compile tests, native Go controls, and
go list -export runtime/cgo. A second diagnostic used the same MinGW ARM64 job and 15-second resource sampler to run the full suite three times on the PR compiler changes and on main:Both diagnostic jobs passed, including the isolated LLGo repetitions and native Go controls. The measurements came from separate runner instances and a 15-second sampler, so they are observational rather than a precise allocation benchmark; they show no progressive or PR-specific memory exhaustion in these runs. The earlier runner losses were not reproduced, and their root cause remains unconfirmed. The temporary diagnostic workflow is not part of this contribution, and no retry or failure-suppression behavior was added.
Size and performance
The pre-rebase benchmark run passed all 9 jobs and compares
826e07e5e414with main410af8e93a1c. None of the 48 native program variants grew in file size;cprintfis unchanged with and without LTO on every platform.fmtprintf/ LTO size changeprintln/ LTO size changeThe Memory64
ec64module grows by 8 B; the other LLGo Wasm modules shrink by 228–350 B, and all generated glue sizes are unchanged.Build and run timings are single samples, not a statistically established speedup or regression. The six native-build aggregates stay within -1.35% to +1.60% on Windows and are -3.52% on Linux; macOS reports +25.42%, while unchanged Go controls and compiler benchmarks on that runner also slow down. These results do not establish a PR-caused broad slowdown, and they do not establish that every individual benchmark is regression-free. All benchmark jobs completed within 25 minutes (maximum 22m19s).
This contribution supersedes fork PR cpunion#243.
Latest main rebase (2026-09-14)
Rebased onto current main
5c5874359without conflicts. The implementation patches remain unchanged; commit726fc6875only updates the two receive/select comments requested in review so they accurately state that function-entry slots remain valid acrossStackRestore. The fullssapackage passes locally. Full localcltesting reached the macOS 10-minute package limit while executing generated binaries, with no assertion failure; fresh CI is running on the rebased head. All review threads were answered and resolved.LTO failure dependency
This branch now also includes the independently reviewed fix from #2562. The rebased CI LTO job https://github.com/xgo-dev/llgo/actions/runs/34809862200/job/103868886091 timed out in TestLTOPluginRejectsMalformedInterfaceAttributes while waiting for opt after invalid metadata triggered an abort. The dependency makes invalid metadata exit with status 1 and bounds the regression subprocess; it does not suppress errors or retry tests. With the LLVM 22 plugin built from the same fix and the dev build tag enabled, all three malformed-metadata regression cases pass locally (no skips). The combined head
fc0ee5071has now passed all 64 checks, including the formerly failing LTO job and all benchmark jobs. Codecov confirms all modified coverable lines are covered, and the current-head fennoai review has no unresolved findings. The macOS coverage job passed in 59m45s, close to the existing 60-minute limit; no timeout increase or retry behavior was added.