-
Notifications
You must be signed in to change notification settings - Fork 1.9k
C++: Value numbering for casts that only modify specifiers #20156
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
C++: Value numbering for casts that only modify specifiers #20156
Conversation
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.
Pull Request Overview
This PR improves global value numbering (GVN) for C++ by treating conversions that only modify type specifiers (like const qualifiers) as preserving the same value number. This fixes false positives in static analysis by recognizing that s.foo() and s.bar() operate on the same underlying object value, even when foo requires a const conversion.
- Introduces
TypePreservingConvertInstructionclass to identify conversions that only change specifiers - Updates GVN logic to assign the same value number to type-preserving conversions as their operands
- Adds test coverage for the new functionality
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| StrncpyFlippedArgs.expected | Removes a false positive test result that is now correctly handled |
| test.cpp | Adds test case for const member function calls to verify GVN behavior |
| ir_gvn.expected | Updates expected test output showing improved value numbering |
| ValueNumberingInternal.qll (3 files) | Implements the core logic for type-preserving conversion handling |
| 2025-08-02-gvn.md | Documents the improvement in change notes |
.../lib/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/internal/ValueNumberingInternal.qll
Show resolved
Hide resolved
10762d7 to
65b1b7f
Compare
jketema
left a comment
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.
LGTM
Consider the following snippet:
Because
foois aconstmember function there will be aGlvalueConversionwhich converts the qualifier from aglval<S>to aglval<const S>before it's passed tofoo. This conversion will generate aConvertinstruction.However, since
barisn't aconstmember function there won't be aConvertinstruction, and so global value numbering (GVN) concludes that the two qualifiers won't necessarily evaluate to the same value at runtime (although, they obviously will).This PR modifies GVN so that conversions that only modify specifiers receive the same global value number.
Commit-by-commit review recommended.
There was 1 query test change (see 099e007) which fixes a false positive. I went digging a bit into when that FP was introduced, and it turns out this was all the way back in #2851 when we put the IR into production (i.e., when we replaced the AST-based GVN with the IR-based GVN)!
DCA was uneventful.