Skip to content

ssa: fix loop defer draining#1642

Merged
xushiwei merged 5 commits intogoplus:mainfrom
cpunion:fix/defer-inloop-drain
Feb 17, 2026
Merged

ssa: fix loop defer draining#1642
xushiwei merged 5 commits intogoplus:mainfrom
cpunion:fix/defer-inloop-drain

Conversation

@cpunion
Copy link
Collaborator

@cpunion cpunion commented Feb 15, 2026

Fix llgo's defer lowering for defers executed inside loops.

  • Ensure loop defers are recorded even for non-closure + no-args forms.
  • Drain only loop-defer nodes (avoid popping unrelated defers), preventing panics like time: Stop called on uninitialized Timer.
  • Classify defers in cyclic CFG blocks as loop defers (SCC-based) so nested/complex loops execute all defers.

Tests:

  • Add regressions in test/defer_test.go.
  • Strengthen ssa/eh_loop_test.go.
  • Update cl/_testdefer/gobuild/out.txt to match new classification.

Rationale / Notes

  • blocks-only is not sufficient: on goplus/main, the loop-defer lowering in ssa/eh.go fails to record some loop defers (notably non-closure + no-args forms), so the drain loop has nothing to pop/run. In an isolated cherry-pick of only the SCC classification + tests, go test ./ssa -run TestDeferInLoopIR fails and the IR contains no FreeDeferNode for loop defers.
  • This PR does not change runtime/internal/runtime.Defer (its layout stays the same). The only "structure" change is the compiler-emitted internal linked-list node layout stored in Defer.Args (adding a small header field id uintptr). Runtime never decodes these nodes; it only frees them via FreeDeferNode.
  • The per-node id tag (or an equivalent discriminator) is required to preserve correct semantics when multiple defer statements inside the same loop share a single LIFO chain and their nodes are interleaved. Without a tag, the drainer cannot safely determine which static defer statement should decode/call a given node, and it risks decoding the wrong layout or draining unrelated defers.
  • When a deferred call panics while draining loop defers, rethrow must resume at the drainer entry so the remaining nodes in the same frame are drained before unwinding to the next defer statement.

Possible follow-up

  • The IR generation for loop-drainers could be refactored to generate one drainer per contiguous run of DeferInLoop statements during endDefer (instead of accumulating state on aDefer), but the semantic requirements above (recording nodes, per-node dispatch, and rethrow-to-drain) remain.

@gemini-code-assist
Copy link

Summary of Changes

Hello @cpunion, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refines how llgo handles defer statements within loops. It introduces a more robust classification of loop-related blocks using SCC analysis, ensuring that defer calls within complex or nested loops are correctly identified. Furthermore, it enhances the runtime behavior of loop defers by ensuring all instances are recorded and by implementing a precise draining mechanism that only processes the intended loop-specific defers, thereby preventing incorrect execution or panics.

Highlights

  • Improved Loop Defer Classification: Implemented a new mechanism to correctly identify and classify defer statements within cyclic control flow graph (CFG) blocks as 'loop defers' using Tarjan's Strongly Connected Components (SCC) algorithm. This ensures accurate handling of defers in complex or nested loops.
  • Enhanced Loop Defer Recording: Ensured that loop defer statements are properly recorded and tracked, even for non-closure functions without arguments, to guarantee correct execution count and behavior.
  • Precise Loop Defer Draining: Revised the defer draining logic for loop defers to specifically process only the relevant loop-defer nodes, preventing unintended draining of unrelated defer statements and potential panics.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • cl/_testdefer/gobuild/out.txt
    • Updated defer classification for numerous basic blocks from 'cond' to 'loop', reflecting the new SCC-based loop detection.
  • cl/blocks/block.go
    • Added inLoop boolean field to blockState to indicate participation in a CFG cycle.
    • Modified kind() method to use the new inLoop field for DeferInLoop classification.
    • Introduced markInLoop function, implementing Tarjan's SCC algorithm, to identify and mark blocks belonging to cycles.
  • ssa/eh.go
    • Added nextID (uintptr) and loopCases ([]loopDeferCase) fields to the aDefer struct.
    • Defined loopDeferCase struct to store id, typ, fn, and args for loop defers.
    • Modified Defer function to assign a unique nextID to each defer statement and record loopDeferCase entries for DeferInLoop kind.
    • Updated saveDeferArgs to accept kind and id, ensuring loop defers are always recorded and storing the id in the defer node.
    • Refactored DeferInLoop handling in Defer to use a dispatch chain based on nodeID to call specific loop defer statements, replacing the previous generic call.
    • Adjusted callDefer to account for the new id field when accessing defer node data.
  • ssa/eh_loop_test.go
    • Added an assertion to TestDeferInLoopIR to verify that FreeDeferNode is present in the generated IR, confirming proper node management for loop defers.
  • test/defer_test.go
    • Imported bytes, sync/atomic, and time packages.
    • Added TestDeferInLoopTwoStatementsInterleaved to check the execution order of multiple defers within a loop.
    • Added TestDeferInLoopNoArgsNonClosureRunsPerIteration to confirm that non-closure, no-argument defers in loops are executed the correct number of times.
    • Added TestDeferInLoopDoesNotDrainOtherDefers to ensure that loop defer draining logic does not inadvertently execute unrelated outer defers.
Activity
  • No human activity (comments, reviews) was detected in the provided context.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly identifies a bug in defer handling within loops and proposes a robust solution using SCC for loop detection and a dispatch mechanism for executing the correct defers. The changes are well-tested. However, I've found a significant issue in the implementation where the defer-draining logic is generated multiple times, which needs to be addressed. I've also included a suggestion for a potential performance improvement in the dispatch logic.

@codecov
Copy link

codecov bot commented Feb 15, 2026

Codecov Report

❌ Patch coverage is 97.97980% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.18%. Comparing base (9c8b6b3) to head (fec2232).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
ssa/eh.go 95.83% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1642      +/-   ##
==========================================
+ Coverage   91.13%   91.18%   +0.04%     
==========================================
  Files          45       45              
  Lines       12117    12201      +84     
==========================================
+ Hits        11043    11125      +82     
- Misses        897      898       +1     
- Partials      177      178       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xgopilot
Copy link
Contributor

xgopilot bot commented Feb 15, 2026

Code Review Summary

This PR correctly fixes the defer classification for loop semantics by introducing Tarjan's SCC algorithm to identify all blocks participating in cycles, replacing the previous findLoop approach that only marked blocks on a single cycle path. The ID-based dispatch mechanism for draining multiple distinct defer statements within the same loop is well-designed.

Strengths:

  • SCC-based classification is the right architectural approach
  • Test coverage is thorough (interleaved defers, no-args non-closure, outer defer isolation)
  • Clear comments explaining the inLoop vs loop distinction in kind()

Minor suggestions: A few documentation updates would improve maintainability — see inline comments for the stale pseudo-code comment and the loopDeferCase struct.

@cpunion
Copy link
Collaborator Author

cpunion commented Feb 15, 2026

Addressed review notes:

  • ssa/eh.go: add loopDrainerGenerated and generate the loop-defer drainer once per contiguous run of DeferInLoop statements (reset when hitting DeferAlways/DeferInCond), plus guard against empty loopCases.
  • ssa/eh.go: update the pseudo-code node layout comment to include the new id field.
  • ssa/eh.go: replace the offset==2 proxy with an explicit fn.kind==vkClosure check.
  • ssa/eh.go: add doc comment for loopDeferCase.
  • cl/blocks/block.go: clarify loop (traversal marker) vs inLoop (SCC semantics).

Re dispatch perf: ssa.Builder doesn't currently expose a switch helper (it's commented out), so keeping the linear chain for now.

@cpunion
Copy link
Collaborator Author

cpunion commented Feb 15, 2026

Follow-up fix: loop-defer drainer must survive rethrow.

loopDrainerGenerated is fine for codegen dedup, but a deferred call can panic while draining (e.g. recover+re-panic). In that case we must keep draining remaining loop nodes in the same frame.

Fix: in the DeferInLoop drainer, store rethPtr to the current stmt block address before each callDefer, so a rethrow resumes at the drainer stmt (valid indirectbr dest) and continues draining with the next node already popped.

This makes TestPanicCrossTwoFunctionsRecover pass under llgo test.

@xushiwei xushiwei merged commit bd7594e into goplus:main Feb 17, 2026
37 checks passed
@cpunion cpunion deleted the fix/defer-inloop-drain branch March 6, 2026 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants