feat(ssa): implement lazy loading for type initialization to fix #1389 #1390
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 addresses a critical bug in overlay packages that caused segmentation faults due to an incorrect initialization order. Specifically, the Highlights
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a critical segmentation fault caused by an incorrect initialization order for overlay packages. The fix in ssa/stmt_builder.go is direct and effective. I have one suggestion to improve the long-term maintainability of the notInit function by making the suffix check more extensible. Overall, this is a good fix.
ssa/stmt_builder.go
Outdated
| name := fn.Name() | ||
| // Check if this is an init call (.init suffix) or hasPatch call ($hasPatch suffix) | ||
| // Both should be considered as initialization calls that need to run before init$after | ||
| return !strings.HasSuffix(name, ".init") && !strings.HasSuffix(name, "$hasPatch") |
There was a problem hiding this comment.
This change is correct and fixes the bug. For better long-term maintainability, you could consider making this check more extensible. Instead of adding && !strings.HasSuffix(...) for each new init-like suffix, iterating over a slice of suffixes would make it easier to add more in the future.
name := fn.Name()
// Check for init-like suffixes. These calls must run before init$after.
for _, suffix := range []string{".init", "$hasPatch"} {
if strings.HasSuffix(name, suffix) {
return false
}
}
return true
Code Review SummaryThis PR effectively resolves a critical initialization order bug in overlay packages. The fix is minimal, precise, and well-documented with inline comments. All security, performance, and code quality reviews indicate this is a solid implementation. ✅ Approved - The change maintains high code quality standards while fixing the segmentation fault issue in Key strengths:
|
luoliwoshang
force-pushed
the
fix-overlay-init-order
branch
3 times, most recently
from
b6eca18 to
c485e2c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1390 +/- ##
==========================================
- Coverage 90.98% 90.27% -0.71%
==========================================
Files 43 43
Lines 11287 11404 +117
==========================================
+ Hits 10269 10295 +26
- Misses 859 949 +90
- Partials 159 160 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add a minimal test case that reproduces the conditional variable reuse bug in type initialization. The test demonstrates: - A named type (MyType) that is initialized early - An interface (MyInterface) with a method returning MyType - The compiler incorrectly reuses the parent type's null check condition to control method type initialization In the generated IR (out.ll): - Line 163-164: Defines %9 = (MyType == null) - Line 182: Reuses the same %9 to control InitNamed call - When MyType already exists, method initialization is skipped This is the same bug pattern as io/fs.FileMode in the reported issue, but in a much simpler form for easier understanding and testing. Related: goplus#1389 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
luoliwoshang
force-pushed
the
fix-overlay-init-order
branch
from
c485e2c to
113aa06
Compare
Fixes goplus#1389 by introducing lazy loader functions that guarantee type initialization regardless of control flow. Previously, nested type initializations (e.g., `func() MyType`) were incorrectly placed inside conditional blocks (Phase2 of parent types). This caused segfaults when the parent type was already initialized, as the nested type's initialization would be skipped. Changes: - Add `createTypeLoader()` to generate `__llgo_load_<typename>` functions - Each loader performs null check and initializes the type on first call - Modify `loadType()` to return call expression instead of global variable - All type references now become `call @__llgo_load_xxx()` instead of `load @xxx` - Add test cases for SetBlockEx behavior to document the temporary block switching pattern 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Regenerated all test IR files using gentests after implementing lazy type loading mechanism. Key changes in IR: - All type references now use `call @__llgo_load_<typename>()` instead of `load @<typename>` - Each type has a dedicated loader function with null check - Removed `init$after` function as type initialization is now lazy - Type initialization happens on-demand in loader functions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Fixes goplus#1389 by introducing lazy loader functions that guarantee type initialization regardless of control flow. Changes: - Delete redundant abiTypeInitInline function - Rename typeLoaderName → abiTypeLoaderName - Rename createTypeLoader → abiTypeInitLoader - Modify loadType() to call loaders in both init$after and current function - Each type gets independent __llgo_load_<typename> function - Loader functions perform null check and initialize on first call - Update all test outputs to reflect new IR generation This fixes the bug where nested type initializations were incorrectly placed in conditional blocks and could be skipped.
luoliwoshang
changed the title
1 participant
Issue #1389 Root Cause Analysis Report
Problem Overview
When importing the
go/buildpackage (an overlay patch package), the program crashes at runtime with the error:The crash occurs during
io/fs.init$afterwhen initializing the runtime metadata for theio/fs.DirEntryinterface type.Crash Stack Analysis
Key Points:
io/fs.init$afterexecutionInterface()→interfaceStr()→(*Type).String()0x14, which is a null pointer dereference when accessing theTFlagfield (offset 0x14) of theTypestructRuntime Debugging Findings
1. Added Debugging Code
Added debug output in
runtime/internal/runtime/z_face.go:2. Debug Output
Key Finding:
io/fs.DirEntryinterface has 4 methods:Info(),IsDir(),Name(),Type()Type()has anilabi.Imethod.Typ_field3. DirEntry Interface Definition
The
Type()method has signaturefunc() FileMode, but this function type was not properly initialized.Overlay Comparison Experiment
Experiment Method
Remove
go/buildfromhasAltPkginruntime/build.go:Experiment Result
After removing overlay: The program runs normally!
This proves the issue is related to the
go/buildoverlay patch mechanism.Initialization Order Comparison
1. Correct Initialization Order Without Overlay
Order: Initialize all dependency packages → then execute package's
init$after2. Incorrect Initialization Order With Overlay
Order: Execute
init$afterfirst → then initialize dependency packages viainit$hasPatchProblem:
go/build.init$afteruses theio/fs.FileModetypeio/fs.FileModeto be prematurely initialized (only creating type shell, incomplete)go/build.init$hasPatchcallsio/fs.init()→io/fs.init$afterio/fs.FileModeis already non-null, triggering the compiler's incorrect logicLLVM IR Code Analysis
1. The Only Initialization Location for
func() FileModeType_llgo_121Basic Block:Key Points:
@"_llgo_func$s8U8kwnQmuJamqzoKKCmURes5qoxCGgMrAgxIKBHdmE"is thefunc() FileModetypefunc() FileModewill never be initialized2. Control Flow Analysis
Precondition for
_llgo_121:%443 == truewill it jump to_llgo_119_llgo_119checks iffunc() FileModeis null, then jumps to_llgo_121for initialization%443 == false, it jumps to_llgo_120, skipping the entire initialization block3. Definition of
%443_llgo_46Basic Block:Key Logic:
%443represents whether@"_llgo_io/fs.FileMode"is nullFileModeis null (%443 = true) will method type initialization be executedFileModeis not null (%443 = false), method type initialization is skipped4. Control Flow Graph
Core Problem:
%443 = whether FileMode is nullin_llgo_46_llgo_118, it reuses the same%443variable to control method type initializationRoot Cause Identification
IR Generation Error
The compiler has a logic error when generating LLVM IR for type initialization:
Incorrect Logic:
FileMode):%443 = (FileMode == nil)%443variable to control method type initializationGenerated IR Code:
Correct Logic Should Be:
Correct IR Should Be:
Two Necessary Conditions for the Bug
This bug requires two errors to exist simultaneously to be triggered:
Error 1: Incorrect Init Order
Problem: Overlay package's
init$afterexecutes beforeinit$hasPatchConsequence:
go/build.init$afterexecutes prematurelyio/fs.FileMode, causing it to be prematurely initialized (only creating shell)io/fs.init$afterexecutes later,FileModeis already non-nullError 2: IR Generation Reuses Condition Variable
Problem: Method type initialization is bound to the same null check condition as parent type creation
Consequence:
func() FileModenever being createdDirEntry.Type()method'sImethod.Typ_isnilProblem Trigger Chain
Solutions
Solution 1: Fix Init Order
Change: Adjust compiler logic to ensure
init$hasPatchexecutes beforeinit$afterEffect: Prevents
init$afterfrom prematurely using incompletely initialized typesSolution 2: Fix IR Generation Logic
Change: Allow method type initialization to have independent null check, not dependent on parent type condition
Reason:
Summary
This is a bug that requires two compiler errors to exist simultaneously to trigger:
FileModeto be prematurely initializedFixing either one alone can resolve the surface problem, but both need to be fixed to completely resolve the root issue.
Report Generated: 2025-01
LLGo Version: main branch