Skip to content

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Nov 15, 2023

TLDR

This PR adds a language-level hook for excluding steps from the flow-through calculation. It's a no-op PR for all other languages than C/C++.

Slightly longer explanation

We have a well-known source of FPs in C/C++ caused by the following pattern:

void modify(int* q) { *q = source(); }

void modify_copy(int* p) {
  int x = *p;
  modify(&x);
}

void test() {
  int x;
  modify_copy(&x);
  sink(x); // we report flow from source to sink here.
}

This happens because we model each indirection of a parameter as a separate SSA variable, so dataflow really sees the above program as

void modify(int* q, int deref_q) { deref_q = source(); }

void modify_copy(int* p, int deref_p) {
  int x = deref_p;
  modify(&x, x);
}

void test() {
  int x;
  modify_copy(&x, x);
  sink(x); // we report flow from source to sink here.
}

and in the above program it's easy to see that there's a ReturnNodeExt generated that updates the parameter deref_p in modify_copy.

The problem is the SSA step generated by int x = *p; which should be treated special when generating flow-throughs since a subsequent update of x should not affect *p (which is currently what the flow-through summaries give us).

@aschackmull I'd like to hear if you have any problems with 748625c? I hope it's not controversial since:

  • it's a no-op for all the other languages
  • it's a very simple fix for a problem that's been observed quite often in C/C++

DCA looks good. I've verified that all the removed results are instances of the test_modify_copy_of_pointer testcase.

@MathiasVP MathiasVP force-pushed the solve-modify-copy-problem branch from 816adfa to 339bf13 Compare November 28, 2023 14:32
@MathiasVP MathiasVP marked this pull request as ready for review December 5, 2023 13:40
@MathiasVP MathiasVP requested a review from a team as a code owner December 5, 2023 13:40
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Dec 5, 2023
@MathiasVP MathiasVP requested a review from aschackmull December 5, 2023 15:46
@MathiasVP MathiasVP merged commit 9fa20f5 into github:main Dec 6, 2023
@owen-mc
Copy link
Contributor

owen-mc commented Dec 6, 2023

I love that this didn't ping all the language teams for review. Yay for properly shared code!

@MathiasVP
Copy link
Contributor Author

I love that this didn't ping all the language teams for review. Yay for properly shared code!

Indeed 😂. Super happy about that as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ DataFlow Library 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.

3 participants