-
Notifications
You must be signed in to change notification settings - Fork 46
fix(rtFunc): resolve go:linkname directives for runtime package #1508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @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 resolves an issue where calls to Highlights
🧠 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. Using Gemini Code AssistThe 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
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 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
|
There was a problem hiding this 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 addresses an ordering issue with go:linkname resolution for the runtime package. The solution, which involves pre-loading linknames from the runtime's AST before compiling user code, is well-implemented. The changes effectively simplify the sigsetjmp handling by centralizing the logic and properly enabling linkname resolution in rtFunc. I have one suggestion to improve the robustness of parsing //go:linkname directives.
cl/import.go
Outdated
| text := strings.TrimSpace(line[len(linkname):]) | ||
| if idx := strings.IndexByte(text, ' '); idx > 0 { | ||
| name := text[:idx] | ||
| if name == inPkgName { | ||
| link := strings.TrimLeft(text[idx+1:], " ") | ||
| prog.SetLinkname(fullName, link) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current parsing of the //go:linkname directive using strings.IndexByte and strings.TrimLeft is a bit fragile. It could incorrectly parse directives with extra fields. For instance, //go:linkname localname linkname extrastuff would result in linkname extrastuff being treated as the link target, which is likely not the intended behavior.
Using strings.Fields would make the parsing more robust by ensuring that the directive has exactly two arguments, which aligns with how the Go toolchain handles these directives.
| text := strings.TrimSpace(line[len(linkname):]) | |
| if idx := strings.IndexByte(text, ' '); idx > 0 { | |
| name := text[:idx] | |
| if name == inPkgName { | |
| link := strings.TrimLeft(text[idx+1:], " ") | |
| prog.SetLinkname(fullName, link) | |
| } | |
| } | |
| fields := strings.Fields(line[len(linkname):]) | |
| if len(fields) == 2 { | |
| name := fields[0] | |
| if name == inPkgName { | |
| link := fields[1] | |
| prog.SetLinkname(fullName, link) | |
| } | |
| } |
Code Review SummaryThis PR successfully fixes the linkname resolution issue for runtime functions. The implementation is architecturally sound and follows existing patterns. I've reviewed the changes across code quality, performance, documentation, and security dimensions. Key Findings:
See inline comments for specific recommendations. |
Detailed Review FindingsCode Quality Issues1. Code Duplication in
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1508 +/- ##
==========================================
- Coverage 90.93% 90.76% -0.18%
==========================================
Files 44 44
Lines 11525 11495 -30
==========================================
- Hits 10480 10433 -47
- Misses 883 901 +18
+ Partials 162 161 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4290ded to
847110d
Compare
Extend ParsePkgSyntax to collect //go:linkname and //llgo:link directives during package loading. This reuses the existing dedup.SetPreload mechanism to ensure all linknames (including runtime package) are collected before compilation begins. Changes: - Add function and variable linkname collection to ParsePkgSyntax - Add collectLinknameFromDoc helper function for linkname parsing - Use sync.Map for linkname storage to support concurrent package loading - Keep fnlink resolution in rtFunc for linkname substitution The ParsePkgSyntax function is already called during package loading via dedup.SetPreload, so this change naturally integrates with the existing build flow without requiring special handling for the runtime package. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
847110d to
c078e08
Compare
Move linkname and export collection logic from initFiles to ParsePkgSyntax, which runs during package loading phase. Changes: - Add exports sync.Map to Program for storing export declarations - Add SetExportName, ExportName, CopyExportsTo methods to Program - Extend collectLinknameFromDoc to handle //export directives - Move package C auto-export logic to ParsePkgSyntax - Simplify initFiles to only collect skip names This consolidates all AST comment parsing in one place and allows cPkg to be determined directly from pkg.Name() == "C". 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Alt packages (e.g. github.com/goplus/llgo/runtime/internal/lib/os) should register linknames using the original package path (e.g. os), not their own path. Otherwise linkname lookups fail because symbols are stored under the wrong namespace. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
When //export directive has a different name than the function and enableExportRename is false (non-embedded target), the compiler should panic with "export comment has wrong name" error. This was missing in the refactored collectLinknameFromDoc function. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove the initLinknameByDoc function which is no longer needed after moving linkname/export collection to ParsePkgSyntax. The initLinkname and initLink functions are retained as they are still used by pkgSymInfo.initLinknames for processing decl-only packages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
After moving linkname collection to ParsePkgSyntax (which runs during package loading), the syms.initLinknames(p) call in importPkg became redundant. ParsePkgSyntax already processes all packages' linknames at load time via dedup.SetPreload, so there's no need to re-process them during compilation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
After refactoring linkname collection to ParsePkgSyntax during package loading, the following code is no longer needed: - pkgSymInfo type and its methods (addSym, initLinknames) - context.initLinkname and context.initLink methods - Related constants (noDirective, hasLinkname, unknownDirective) - Related test functions (TestErrInitLinkname, TestHandleExportDiffName, TestInitLinkExportDiffNames) - Simplified importPkg to only set pkgInfo.kind The linkname/export collection is now handled entirely by ParsePkgSyntax which runs during package loading via dedup.SetPreload. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
8e1c797 to
b21cbaf
Compare
- Rename functions to match original naming convention (init* prefix) - Move initLinknameByDoc after initFiles function - Simplify implementation to return hasLinkname directly when directive found - Update all tests to use new function names 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
b21cbaf to
30db70d
Compare
Add isVar parameter to the closure signature for future use in distinguishing function and variable linknames. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Pass isVar to distinguish function linknames (false) from variable linknames (true) when collecting linkname directives. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Update TestErrInitLinkname to use package-level initLinkname with nil prog - Update TestHandleExportDiffName to use package-level initLinkname - Update TestInitLinknameByDocExportDiffNames to use package-level initLinknameByDoc - Update TestInitLinkExportDiffNames to properly simulate enableExportRename behavior - Use prog.ExportName() instead of pkg.ExportFuncs() for consistency 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Minimize diff by keeping variable declarations and assignments the same as main branch, only removing the goto and linkname collection logic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Keep the warning output when linkname symbol is not found, consistent with main branch behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Reorganize code structure by placing related skip collection functions (collectSkipNames, collectSkipNamesByDoc, collectSkip) before the linkname collection functions for better readability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove dead commented code that was left over from previous implementation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary
PreloadLinknamesto collect linknames from runtime AST before user code compilationfnlinkresolution inrtFuncto properly resolve linkname targetssigsetjmpfor non-linux,__sigsetjmpfor linux)Problem
When
rtFunc("Sigsetjmp")was called, it generated calls toruntime.Sigsetjmpinstead of the C symbolsigsetjmp. This was because:rtFunc()didn't callfnlinkto resolve linknameSolution
Preload runtime linknames from AST right after
SetRuntime, before user code compilation.Test plan
_demo/go/defer/runs correctly with output:error,10,5Fixes #1483
🤖 Generated with Claude Code