Skip to content
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++: Fix dataflow out of post update nodes #14171

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Sep 8, 2023

99cc417 prevented flow from an output argument from flowing into an argument to the same call. This revealed a problem that only became apparent on the QA run for 2.14.4. When we have a snippet such as:

void test(int* p) {
  write_to_first_argument_and_read_from_second_argument(
    p, // (1)
    p // (2)
  );
  sink(p); // (3)
}

we can't just block flow from a post-update node of a call (i.e., (1)) to a use in the same
call (i.e., (2)) since the use-use chain is (1) -> (2) -> (3).

So instead we have to skip the step from (1) to (2) and go directly from (1) to (3). This PR does this using a call to shortestDistances.

(Leaving this as a draft while DCA is running.)

@github-actions github-actions bot added the C++ label Sep 8, 2023
@MathiasVP MathiasVP force-pushed the fix-dataflow-out-of-post-update-nodes branch 2 times, most recently from 8d30964 to c3c1b4f Compare September 8, 2023 13:08
@MathiasVP
Copy link
Contributor Author

Ouch. Wireshark is not happy with this change 😭. Looking into it now.

@MathiasVP MathiasVP force-pushed the fix-dataflow-out-of-post-update-nodes branch from c3c1b4f to 9f89c63 Compare September 8, 2023 16:07
Copy link
Contributor

@alexet alexet left a comment

Choose a reason for hiding this comment

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

The logic LGTM.

@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Sep 8, 2023
@MathiasVP
Copy link
Contributor Author

The new results on vim and openttd are all TPs.

@MathiasVP MathiasVP marked this pull request as ready for review September 8, 2023 16:30
@MathiasVP MathiasVP requested a review from a team as a code owner September 8, 2023 16:30
@rdmarsh2
Copy link
Contributor

rdmarsh2 commented Sep 8, 2023

Performance LGTM

@rdmarsh2 rdmarsh2 merged commit e1ffc8d into github:codeql-cli-2.14.4 Sep 8, 2023
11 checks passed
@github github deleted a comment from Lizzyrobin4 Sep 8, 2023
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.

None yet

3 participants