cabi: remove unsafe load-to-memcpy optimization#1607
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 potential compiler optimization issue by adding a new test case. The test is designed to ensure that the compiler correctly handles scenarios involving Named Return Value Optimization (NRVO) and return-slot reuse, specifically when a slice is returned from a function and its underlying source data is modified afterwards. This prevents situations where the returned slice might unexpectedly reflect post-return modifications, thereby safeguarding data integrity and predictable program behavior. 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. Changelog
Activity
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 introduces a test case to address potential issues with Named Return Value Optimization (NRVO) and return-slot reuse in the compiler, particularly concerning slice return values. The test correctly verifies that a returned slice remains unchanged even if the source struct's slice field is modified after the return value is captured. My feedback includes suggestions to align the new Go test code with idiomatic language conventions for better readability and maintainability.
cl/_testgo/return-1605/main.go
Outdated
| } | ||
|
|
||
| func a() []int { | ||
| var t = T{data: []int{1, 2}} |
There was a problem hiding this comment.
In Go, it is more idiomatic to use the short variable declaration := when declaring and initializing a new variable inside a function. This improves code conciseness and readability.
| var t = T{data: []int{1, 2}} | |
| t := T{data: []int{1, 2}} |
cl/_testgo/return-1605/main.go
Outdated
| } | ||
|
|
||
| func b() ([]int, bool) { | ||
| var t = T{data: []int{1, 2}} |
There was a problem hiding this comment.
In Go, it is more idiomatic to use the short variable declaration := when declaring and initializing a new variable inside a function. This improves code conciseness and readability.
| var t = T{data: []int{1, 2}} | |
| t := T{data: []int{1, 2}} |
Code Review SummaryOverall, this is a solid regression test that correctly captures the NRVO/return-slot reuse bug scenario. The test structure follows project conventions with Suggestions:
No security or performance concerns identified. |
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
cl/_testgo/return-1605/main.go
Outdated
|
|
||
| func a() []int { | ||
| var t = T{data: []int{1, 2}} | ||
| a := t.data |
There was a problem hiding this comment.
The local variable a shadows the function name a. While Go allows this, consider renaming to something more descriptive like slice, result, or originalData for improved readability.
luoliwoshang
force-pushed
the
return/slot
branch
from
4d25691 to
0d41c21
Compare
Fixes goplus#1608 When transforming functions with sret (structure return) in CABI mode, the optimizer previously used memcpy from the load source address to the return address. However, if the source address content is modified between the load and ret instructions, the memcpy would copy the modified (wrong) value instead of the original loaded value. Example of the bug: ```go func problem() []int { var t = T{data: []int{1, 2}} a := t.data // load from &t.data t.data = []int{1, 2, 3} // modify &t.data return a // should return [1,2], not [1,2,3] } ``` The memcpy optimization assumed the source address content remains unchanged between load and ret, which is not always true. Solution: Disable the memcpy optimization for AttrPointer returns and always use store instruction to copy the loaded value directly. This ensures the correct value (from the load instruction) is returned. This also fixes the root cause of issue goplus#1604 (TypeSwitch initialization lost) which was caused by gogen's clearBlockStmt() being affected by this bug.
luoliwoshang
force-pushed
the
return/slot
branch
from
0d41c21 to
42218be
Compare
Move the regression test for issue goplus#1608/goplus#1604 from cl/_testgo to _demo/go/return-1605 with proper validation logic that panics if the returned slice has wrong length or elements.
luoliwoshang
force-pushed
the
return/slot
branch
from
45a4aea to
446b8b0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1607 +/- ##
==========================================
+ Coverage 91.00% 91.05% +0.05%
==========================================
Files 45 45
Lines 11927 11933 +6
==========================================
+ Hits 10854 10866 +12
+ Misses 898 892 -6
Partials 175 175 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add TestIssue1608_AttrPointerReturnNoMemcpy to verify CABI transformation uses store instead of memcpy for sret returns. The test creates IR where: 1. A slice is loaded from a struct field 2. The struct field is modified 3. The originally loaded slice is returned This ensures the fix prevents the memcpy optimization bug where the source address content could change between load and ret. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
luoliwoshang
changed the title
3 participants
Fixed #1604 #1606 #1608
Reproduction Code
Problem Analysis
IR Before CABI Transformation
IR After CABI Transformation (BUG)
Root Cause
During CABI transformation, when the optimizer sees
ret %Slice %9where%9is aloadinstruction, it previously usedmemcpy(sret_ptr, %9, size)to copy from the load source address.The bug:
%9and%18point to the same memory address (T.datafield). After theloadbut beforeret, this address content was modified bystore %17, ptr %18!So
memcpycopies the modified value{1,2,3}instead of the originally loaded value{1,2}.Fix
Remove the memcpy optimization, always use
storeinstruction:%10is an SSA value - once defined, it never changes. So usingstore %10is safe.