-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Python: Shared dataflow, argument passing tests #4488
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
Python: Shared dataflow, argument passing tests #4488
Conversation
RasmusWL
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.
Very good catch 👍
Embarrassingly, I jumped straight into read the first commit before looking at the nice description you made in the PR. However, that illuminated that fact that I wasn't able to understand the problem from the comment for isBarrierIn, so I made a suggestion I think I would have understood if I didn't already understand the problem 😛
I did not look over changes to expected files in cced335
| * This prevents the argument from one call to reach the sink | ||
| * via a different call, by flowing to an argument of that. |
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.
| * This prevents the argument from one call to reach the sink | |
| * via a different call, by flowing to an argument of that. | |
| * We had a problem where the fact that we use `arg` in a sequence of calls such as `func(kw=arg); func(arg)` | |
| * use-use flow would make it seem like we handled both cases, where in fact we only handled the last case. | |
| * Removing flow into source nodes prevents this unwanted use-use flow. |
| * This prevents the argument from one call to reach the sink | ||
| * via a different call, by flowing to an argument of that. |
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.
| * This prevents the argument from one call to reach the sink | |
| * via a different call, by flowing to an argument of that. | |
| * We had a problem where the fact that we use `arg` in a sequence of calls such as `func(kw=arg); func(arg)` | |
| * use-use flow would make it seem like we handled both cases, where in fact we only handled the last case. | |
| * Removing flow into source nodes prevents this unwanted use-use flow. |
| * This prevents the argument from one call to reach the sink | ||
| * via a different call, by flowing to an argument of that. |
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.
| * This prevents the argument from one call to reach the sink | |
| * via a different call, by flowing to an argument of that. | |
| * We had a problem where the fact that we use `arg` in a sequence of calls such as `func(kw=arg); func(arg)` | |
| * use-use flow would make it seem like we handled both cases, where in fact we only handled the last case. | |
| * Removing flow into source nodes prevents this unwanted use-use flow. |
| * This prevents the argument from one call to reach the sink | ||
| * via a different call, by flowing to an argument of that. |
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.
| * This prevents the argument from one call to reach the sink | |
| * via a different call, by flowing to an argument of that. | |
| * We had a problem where the fact that we use `arg` in a sequence of calls such as `func(kw=arg); func(arg)` | |
| * use-use flow would make it seem like we handled both cases, where in fact we only handled the last case. | |
| * Removing flow into source nodes prevents this unwanted use-use flow. |
| * This prevents the argument from one call to reach the sink | ||
| * via a different call, by flowing to an argument of that. |
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.
| * This prevents the argument from one call to reach the sink | |
| * via a different call, by flowing to an argument of that. | |
| * We had a problem where the fact that we use `arg` in a sequence of calls such as `func(kw=arg); func(arg)` | |
| * use-use flow would make it seem like we handled both cases, where in fact we only handled the last case. | |
| * Removing flow into source nodes prevents this unwanted use-use flow. |
| * This prevents the argument from one call to reach the sink | ||
| * via a different call, by flowing to an argument of that. |
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.
| * This prevents the argument from one call to reach the sink | |
| * via a different call, by flowing to an argument of that. | |
| * We had a problem where the fact that we use `arg` in a sequence of calls such as `func(kw=arg); func(arg)` | |
| * use-use flow would make it seem like we handled both cases, where in fact we only handled the last case. | |
| * Removing flow into source nodes prevents this unwanted use-use flow. |
| * This prevents the argument from one call to reach the sink | ||
| * via a different call, by flowing to an argument of that. |
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.
| * This prevents the argument from one call to reach the sink | |
| * via a different call, by flowing to an argument of that. | |
| * We had a problem where the fact that we use `arg` in a sequence of calls such as `func(kw=arg); func(arg)` | |
| * use-use flow would make it seem like we handled both cases, where in fact we only handled the last case. | |
| * Removing flow into source nodes prevents this unwanted use-use flow. |
RasmusWL
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.
👍
The main purpose of this PR is to make the argument passing tests more honest by adding a barrier for inflow at sources.
The way we wrote tests, for instance
meant that
arg2in the second and third calls would flow toarg2in the fourth call and from there to the sink.This made it look like the second and third call handled
arg2correctly just because the fourth call does.The first commit adds the
BarrierInto all sources, removing the spurious flows from the test output.This problem was discovered by looking at paths, so the second commit changes the tests to be path queries. I also added the remaining tests (for arguments 5 through 7).
I changed the first reported location to be source rather than sink, so that the current test output can easily be compared with the proposed output after
select.