Skip to content

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Aug 2, 2025

Consider the following snippet:

struct S {
  void foo() const;
  void bar();
};

void test() {
  S s;
  s.foo();
  s.bar();
}

Because foo is a const member function there will be a GlvalueConversion which converts the qualifier from a glval<S> to a glval<const S> before it's passed to foo. This conversion will generate a Convert instruction.

However, since bar isn't a const member function there won't be a Convert instruction, 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.

@MathiasVP MathiasVP marked this pull request as ready for review August 2, 2025 15:37
@MathiasVP MathiasVP requested a review from a team as a code owner August 2, 2025 15:37
@Copilot Copilot AI review requested due to automatic review settings August 2, 2025 15:37
Copilot

This comment was marked as outdated.

@Copilot Copilot AI review requested due to automatic review settings August 2, 2025 15:39
Copilot

This comment was marked as outdated.

@Copilot Copilot AI review requested due to automatic review settings August 2, 2025 16:52
Copy link
Contributor

@Copilot Copilot AI left a 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 TypePreservingConvertInstruction class 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

@MathiasVP MathiasVP force-pushed the value-numbering-for-noop-casts branch from 10762d7 to 65b1b7f Compare August 3, 2025 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant