Skip to content

feat: implement llgo.stackSave#1304

Merged
xushiwei merged 4 commits intogoplus:mainfrom
MeteorsLiu:impl-stacksave
Nov 12, 2025
Merged

feat: implement llgo.stackSave#1304
xushiwei merged 4 commits intogoplus:mainfrom
MeteorsLiu:impl-stacksave

Conversation

@MeteorsLiu
Copy link
Contributor

@MeteorsLiu MeteorsLiu commented Sep 15, 2025

A part of #1278

@niupilot
Copy link

niupilot bot commented Sep 15, 2025

🔍 PR Review: llgo.stackSave Implementation

Todo List:

  • Read and analyze all 8 changed files
  • Check existing codebase patterns and conventions
  • Review for bugs, security issues, and performance problems
  • Evaluate architecture and design decisions
  • Verify test coverage and expected outputs
  • Check documentation consistency
  • Provide comprehensive review feedback

Summary

This PR implements the llgo.stackSave functionality, which provides access to LLVM's llvm.stacksave() intrinsic. The implementation follows established patterns in the codebase and includes proper test coverage.

Code Quality Assessment

Positive Aspects

  1. Consistent Architecture: The implementation follows the established pattern for adding llgo instructions:

    • Proper constant definition in cl/import.go:507
    • Registration in llgoInstrs map in cl/instr.go:429
    • Handler implementation in cl/instr.go:605-606
  2. LLVM Integration: Correctly uses LLVM's llvm.stacksave() intrinsic via b.Pkg.cFunc() and b.InlineCall(), consistent with other LLVM intrinsic wrappers.

  3. Test Coverage: Includes comprehensive testing:

    • Runtime test with expected LLVM IR output (cl/_testrt/stacksave/)
    • Architecture-specific demo implementations for amd64 and arm64
    • Cross-validation with inline assembly
  4. Type Safety: Proper Go type signature using unsafe.Pointer return type and //go:linkname directives.

🔍 Issues Found

Critical Bug in Type Signature (ssa/eh.go:95-101)

func (p Program) tyStacksave() *types.Signature {
    if p.stackSaveTy == nil {
        paramPtr := types.NewParam(token.NoPos, nil, "", p.VoidPtr().raw.Type)  // ❌ WRONG
        params := types.NewTuple(paramPtr)                                      // ❌ WRONG
        p.stackSaveTy = types.NewSignatureType(nil, nil, nil, nil, params, false)
    }
    return p.stackSaveTy
}

Problems:

  1. stacksave should have no parameters but returns unsafe.Pointer
  2. Currently defines parameters instead of return types
  3. The signature should be: func() unsafe.Pointer

Fix:

func (p Program) tyStacksave() *types.Signature {
    if p.stackSaveTy == nil {
        paramPtr := types.NewParam(token.NoPos, nil, "", p.VoidPtr().raw.Type)
        results := types.NewTuple(paramPtr)  // This should be results, not params
        p.stackSaveTy = types.NewSignatureType(nil, nil, nil, nil, results, false)
    }
    return p.stackSaveTy
}

Inconsistent Demo Code Quality

AMD64 Demo Issues (_demo/c/stacksave/stacksave_amd64.go:19):

asmFull("movq sp, {{addr}}", map[string]any{
    "addr": unsafe.Pointer(&spPtr),
})
  • Uses non-standard {{addr}} syntax instead of standard {addr}
  • Inline assembly should use %rsp instead of sp on x86-64

ARM64 Demo Issues (_demo/c/stacksave/stacksave_arm64.go:17):

sp := asmFull("mov {}, sp", nil)
  • Unnamed register placeholder {} may be unclear
  • Should specify register constraint more explicitly

🏗️ Architecture & Design

Strengths:

  • Clean separation between high-level Go API and low-level LLVM implementation
  • Proper integration with existing instruction dispatch mechanism
  • Follows established naming conventions (llgoStackSave, llgoInstrBase + 0x41)

Concerns:

  • No stackrestore complement function (LLVM provides both stacksave and stackrestore)
  • Demo code tests equivalence but doesn't demonstrate practical usage scenarios

📊 Test Coverage Analysis

Runtime Test (cl/_testrt/stacksave/):

  • ✅ Correct LLVM IR generation verification
  • ✅ Proper linkname usage
  • ✅ Expected output format matches patterns

Demo Tests:

  • ✅ Cross-platform architecture support
  • ✅ Integration with inline assembly
  • ⚠️ Could benefit from more comprehensive validation

📚 Documentation

Missing Documentation:

  • No inline documentation explaining when/why to use stackSave
  • No usage examples in comments
  • Missing safety considerations (stack pointer validity, restoration needs)

Recommendations

Must Fix:

  1. Correct the type signature bug in ssa/eh.go:95-101
  2. Fix inline assembly syntax in demo files

Should Fix:

  1. Add inline documentation explaining the function's purpose and safety considerations
  2. Consider implementing stackrestore for completeness
  3. Improve demo code with better register specifications

Could Improve:

  1. Add more comprehensive usage examples
  2. Consider adding integration tests with actual stack manipulation scenarios

Overall Assessment

This is a solid implementation that follows established patterns well, but contains a critical type signature bug that must be fixed before merging. The test coverage is good and the integration is clean. Once the signature issue is resolved, this will be a valuable addition to the llgo instruction set.

Recommendation: Approve with required changes


👉 Want me to re-run this review? Just type /review in the comments! For more usage examples, visit CodeAgent GitHub Repository.

@MeteorsLiu MeteorsLiu mentioned this pull request Sep 15, 2025
11 tasks
@codecov
Copy link

codecov bot commented Sep 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.02%. Comparing base (86cafff) to head (bf6f785).
⚠️ Report is 21 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1304      +/-   ##
==========================================
+ Coverage   90.98%   91.02%   +0.04%     
==========================================
  Files          43       43              
  Lines       11287    11298      +11     
==========================================
+ Hits        10269    10284      +15     
+ Misses        859      855       -4     
  Partials      159      159              

☔ 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.

@MeteorsLiu MeteorsLiu marked this pull request as ready for review September 15, 2025 07:02
@MeteorsLiu
Copy link
Contributor Author

@xushiwei

@xushiwei xushiwei merged commit 0dbe528 into goplus:main Nov 12, 2025
42 checks passed
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.

4 participants