Skip to content

C++: Fix global variable dataflow FP #20040

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

Merged
merged 4 commits into from
Jul 14, 2025

Conversation

MathiasVP
Copy link
Contributor

This PR fixes a very odd corner case of global dataflow through global variables.

Consider this snippet (which I've also added as a test):

int recursion = (sink(recursion), source());

The IR we generate for this snippet is roughly equivalent to:

void __initialize_recursion() {
  sink(recursion);
  recursion = source();
}

where __recursion represents the actual global variable. Since this is an IRFunction just like any other Function, it gets an initial SSA definition of the global variables that it reads, and it gets a final SSA use of the global variables that it defines. So for the point-of-view of dataflow the function ends up looking like:

void __initialize_recursion() {
  recursion = __initialize_global; // (3)
  sink(recursion); // (4)
  recursion = source(); // (1)
  __final_use(recursion); // (2)
}

where __initialize_global represents the initial SSA definition (a InitialGlobalValue node), and __final_use(recursion) represents the final SSA use (a FinalGlobalValue node).

And now global variable dataflow does its things and this happens:

  1. A write to recursion at // (1) flows to the final SSA use at // (2)
  2. A jump step causes flow from // (2) to the global dataflow node (a GlobalLikeVariableNode) for recursion
  3. Another jump step from the global dataflow node for recursion to the initial SSA definition for recursion at // (3)
  4. A normal flow step from // (3) to the use at // (4).

But clearly we should not have flow in this case! So this PR fixes the FP by removing the InitialGlobalValue node for some global variable v at the entry for the IRFunction that initializes v.

I don't think this will ever happen on any real codebase. But since it was so easy to fix we might as well get this fix in 😄

@Copilot Copilot AI review requested due to automatic review settings July 14, 2025 14:34
@MathiasVP MathiasVP requested a review from a team as a code owner July 14, 2025 14:34
@github-actions github-actions bot added the C++ label Jul 14, 2025
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 fixes a false positive in global variable dataflow analysis for C++ code. The issue occurs in an edge case where a global variable is used within its own initialization expression, causing incorrect dataflow through SSA definitions.

  • Adds a test case demonstrating the problematic scenario with self-referential global variable initialization
  • Modifies the SSA internals to exclude InitialGlobalValue nodes from IRFunctions that initialize the same global variable
  • Updates expected test results to reflect the new test case

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
test.cpp Adds test case for self-referential global variable initialization
dataflow-consistency.expected Updates expected test results for the new test case
SsaInternals.qll Implements fix by filtering out problematic SSA definitions

@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Jul 14, 2025
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if DCA is happy.

@jketema jketema merged commit 2ed54d5 into github:main Jul 14, 2025
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants