fix C++ this pointer for struct ret #1783
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modifies the getConstructorThisPtr function to handle edge cases where a constructor/destructor has the sret (struct return) attribute on parameter 0 but has fewer than 2 arguments. The change replaces strict assertions with a defensive fallback approach.
Key Changes:
- Replaces assertion that required at least 2 arguments when
sretis present with a conditional check - Implements fallback logic to use arg 0 when
sretis present but only 1 argument exists - Adds clarifying comments about the defensive approach
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
svf-llvm/lib/CppUtil.cpp
Outdated
| "not a constructor?"); | ||
| assert(fun->arg_size() >= 1 && "argument size >= 1?"); | ||
| // If param 0 is sret, 'this' is typically param 1, but be defensive. | ||
| const bool isStructRet = fun->hasParamAttribute(0, llvm::Attribute::StructRet); |
There was a problem hiding this comment.
Calling hasParamAttribute(0, ...) on line 533 occurs before verifying that the function has any arguments (checked on line 536). If a function somehow has zero arguments, hasParamAttribute(0, ...) would be querying an out-of-bounds parameter index. While LLVM's API may handle this gracefully, it's safer to check arg_size() first before querying parameter attributes to avoid potential undefined behavior.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| assert(fun->arg_size() >= 1 && "expected at least one argument"); | ||
|
|
||
| // Prefer arg1 when sret is present and available; otherwise fall back to arg0. | ||
| const u32_t thisIdx = (isStructRet && fun->arg_size() >= 2) ? 1 : 0; |
There was a problem hiding this comment.
The fallback logic is semantically incorrect. When sret is present on parameter 0 but there is only one argument, that single argument IS the struct return pointer, not the 'this' pointer. Falling back to arg 0 in this case would return the wrong pointer.
The correct behavior should be to either:
- Assert that when sret is present, there must be at least 2 arguments (restoring the original assertion), OR
- Return nullptr or handle this as an error case rather than silently returning an incorrect pointer
The defensive approach taken here masks a genuine error condition (a malformed constructor/destructor signature) which could lead to incorrect program analysis downstream.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| assert(fun->arg_size() >= 1 && "expected at least one argument"); | ||
|
|
||
| // Prefer arg1 when sret is present and available; otherwise fall back to arg0. | ||
| const u32_t thisIdx = (isStructRet && fun->arg_size() >= 2) ? 1 : 0; |
There was a problem hiding this comment.
This change introduces an inconsistency with the related function getVCallThisPtr (line 411-420), which unconditionally accesses arg 1 when sret is present without checking arg_size() >= 2. If the defensive approach is correct here, the same pattern should likely be applied to getVCallThisPtr for consistency. Conversely, if getVCallThisPtr's approach is correct (relying on well-formed inputs), then this defensive check may be unnecessary.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1783 +/- ##
=======================================
Coverage 64.15% 64.15%
=======================================
Files 243 243
Lines 24569 24569
Branches 4627 4627
=======================================
Hits 15762 15762
Misses 8807 8807
🚀 New features to boost your workflow:
|
1 participant
No description provided.