Skip to content

fix: generate valid function pointers for embedded type's unexported methods#1598

Open
luoliwoshang wants to merge 2 commits intogoplus:mainfrom
luoliwoshang:fix/embedded-unexported-methods
Open

fix: generate valid function pointers for embedded type's unexported methods#1598
luoliwoshang wants to merge 2 commits intogoplus:mainfrom
luoliwoshang:fix/embedded-unexported-methods

Conversation

@luoliwoshang
Copy link
Member

Summary

Fix runtime panic when using interfaces with unexported methods on types that embed other types from different packages.

Problem

When a type embeds another type from a different package to implement an interface that has unexported methods, the interface method table (itab) contained nil function pointers for those unexported methods. This caused a runtime panic when the interface method was called.

For example:

package main

import (
	"go/token"
	"go/types"
)

// wrappedFunc embeds *types.Func to implement types.Object
type wrappedFunc struct {
	*types.Func
}

func main() {
	pkg := types.NewPackage("test", "test")
	scope := pkg.Scope()

	sig := types.NewSignatureType(nil, nil, nil, nil, nil, false)
	fn := types.NewFunc(token.NoPos, pkg, "testFunc", sig)

	wrapped := &wrappedFunc{Func: fn}
	var obj types.Object = wrapped

	scope.Insert(obj) // panic: nil pointer dereference
}

Root Cause

In ssa/abitype.go, when generating the method table for a type, unexported methods from other packages had their Ifn_ (interface function pointer) set to nil:

if !token.IsExported(mName) {
    skipfn = PathOf(mPkg) != pkgPath  // true when method is from another package
}
if skipfn {
    tfn = prog.Nil(prog.VoidPtr()).impl
    ifn = tfn  // nil!
}

Fix

Remove the skipfn logic. Now all methods, including unexported ones from embedded types, get valid function pointers generated.

Test Plan

  • Existing tests pass
  • Regenerated out.ll files for affected test cases
  • Manual test with the reproduction case now works

🤖 Generated with Claude Code

@gemini-code-assist
Copy link

Summary of Changes

Hello @luoliwoshang, 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 primarily addresses a critical runtime panic related to how interface method tables are generated for types that embed other types with unexported methods. The fix ensures that the compiler correctly generates function pointers for these methods, preventing crashes. Additionally, it introduces a new command-line flag to enhance visibility into the build process by printing executed commands and significantly improves the robustness of defer and panic handling in baremetal environments.

Highlights

  • Fix for Interface Method Table (itab) Panic: Resolved a runtime panic that occurred when using interfaces with unexported methods on types embedding other types from different packages. Previously, the itab for such methods contained nil function pointers, leading to crashes.
  • Refined Method Table Generation: The logic in ssa/abitype.go for generating method tables has been updated to ensure that all methods, including unexported ones from embedded types originating from different packages, receive valid function pointers. The skipfn logic that previously nulled these pointers has been removed.
  • Enhanced Command Visibility (-x flag): Introduced a new PrintCommands configuration option and integrated it with the -x command-line flag. This allows users to see the underlying clang and ar commands executed during the build and run processes, improving debugging and transparency.
  • Improved Baremetal Defer/Panic Handling: Extended baremetal support for defer and panic mechanisms by integrating setjmp and longjmp for control flow. This includes new build tags and architecture-specific jmpbuf size definitions for various ARM, AVR, Cortex-M, RISC-V, and Xtensa targets.
  • Dependency Update: Updated the github.com/goplus/gogen dependency to version v1.20.8.

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

Ignored Files
  • Ignored by pattern: .github/workflows/** (1)
    • .github/workflows/targets.yml
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 fixes a runtime panic that occurred when using interfaces with unexported methods on embedded types from different packages. The core change in ssa/abitype.go properly generates valid function pointers in the interface method table for these methods. In addition to the bug fix, this PR introduces substantial improvements to the build system, including better command tracing via a new -x flag, and significantly enhances support for panic/defer on baremetal targets by implementing a setjmp/longjmp-based mechanism. The changes are well-structured, and the new test cases effectively validate the fix. I have one minor suggestion to improve the maintainability of a test script.

@luoliwoshang luoliwoshang force-pushed the fix/embedded-unexported-methods branch 3 times, most recently from 4f82652 to c17e1b3 Compare February 2, 2026 12:23
@xgopilot
Copy link
Contributor

xgopilot bot commented Feb 2, 2026

Code Review Summary

Overall Assessment: Approved

This PR correctly fixes the nil function pointer issue for unexported methods inherited from embedded types. The fix is minimal, focused, and includes a proper regression test.

Strengths:

  • Clean removal of the problematic skipfn logic that was incorrectly nullifying function pointers
  • Excellent regression test (cl/_testgo/embedunexport-1598/) reproducing the exact issue from the PR description
  • No security vulnerabilities or performance regressions introduced

Minor Suggestions:

The generated out.ll files correctly show the transition from ptr null to valid function pointers for unexported methods.

@codecov
Copy link

codecov bot commented Feb 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.05%. Comparing base (ba56141) to head (f315ea4).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1598      +/-   ##
==========================================
- Coverage   91.05%   91.05%   -0.01%     
==========================================
  Files          45       45              
  Lines       11933    11922      -11     
==========================================
- Hits        10866    10855      -11     
  Misses        892      892              
  Partials      175      175              

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

@luoliwoshang luoliwoshang force-pushed the fix/embedded-unexported-methods branch 7 times, most recently from dc23a5d to f2717a8 Compare February 3, 2026 02:43
@luoliwoshang
Copy link
Member Author

@xushiwei

@luoliwoshang luoliwoshang requested a review from xushiwei February 4, 2026 02:58
xgopilot bot pushed a commit to luoliwoshang/llgo that referenced this pull request Feb 5, 2026
…hods

This merges the fix for generating valid function pointers for embedded
type's unexported methods.

- Fix ssa/abitype.go: remove skipfn logic that was setting Ifn_ to nil
- Add regression tests for embedunexport-1598
- Update out.ll test files

Original PR: goplus#1598

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: luoliwoshang <51194195+luoliwoshang@users.noreply.github.com>
@luoliwoshang
Copy link
Member Author

@xgopilot resolve conflict

@xgopilot
Copy link
Contributor

xgopilot bot commented Feb 5, 2026

Merge Conflict Resolved

Merge conflicts have been successfully resolved.

Changes made:

  • Merged origin/main into the PR branch
  • ssa/abitype.go was auto-merged correctly (fix preserved)
  • Regenerated all out.ll files using gentests
  • Committed and pushed merge resolution

PR created in fork: luoliwoshang#195

@luoliwoshang Please merge this PR into your fix/embedded-unexported-methods branch to update the original PR #1598.

luoliwoshang and others added 2 commits February 5, 2026 23:26
…methods (goplus#1597)

Previously, when a type embedded another type from a different package,
the unexported methods inherited from the embedded type would have nil
function pointers (Ifn_) in the method table. This caused runtime panics
when interfaces with unexported methods (like types.Object) were used.

The fix removes the skipfn logic that was setting Ifn_ to nil for
methods from other packages. Now all methods, including unexported ones
from embedded types, get valid function pointers generated.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@luoliwoshang luoliwoshang force-pushed the fix/embedded-unexported-methods branch from f2717a8 to f315ea4 Compare February 5, 2026 15:27
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