-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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++: Clean up SSA and stop relying on memory edges for iterator flow #16345
Merged
MathiasVP
merged 14 commits into
github:main
from
MathiasVP:cleanup-ssa-and-iterator-flow
May 2, 2024
Merged
C++: Clean up SSA and stop relying on memory edges for iterator flow #16345
MathiasVP
merged 14 commits into
github:main
from
MathiasVP:cleanup-ssa-and-iterator-flow
May 2, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll
Dismissed
Show dismissed
Hide dismissed
MathiasVP
force-pushed
the
cleanup-ssa-and-iterator-flow
branch
from
April 28, 2024 22:42
2e7c1a1
to
48f7520
Compare
MathiasVP
force-pushed
the
cleanup-ssa-and-iterator-flow
branch
2 times, most recently
from
April 28, 2024 22:56
df6c268
to
a7fab9a
Compare
MathiasVP
force-pushed
the
cleanup-ssa-and-iterator-flow
branch
from
April 28, 2024 23:02
a7fab9a
to
9de1f49
Compare
MathiasVP
force-pushed
the
cleanup-ssa-and-iterator-flow
branch
from
April 29, 2024 08:38
9de1f49
to
401717d
Compare
jketema
reviewed
May 1, 2024
jketema
reviewed
May 1, 2024
jketema
reviewed
May 2, 2024
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll
Outdated
Show resolved
Hide resolved
jketema
reviewed
May 2, 2024
jketema
reviewed
May 2, 2024
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll
Outdated
Show resolved
Hide resolved
jketema
reviewed
May 2, 2024
jketema
approved these changes
May 2, 2024
MathiasVP
added a commit
to MathiasVP/ql
that referenced
this pull request
May 22, 2024
MathiasVP
added a commit
to MathiasVP/ql
that referenced
this pull request
Jun 3, 2024
MathiasVP
added a commit
to MathiasVP/ql
that referenced
this pull request
Jun 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR ended up being quite a bit larger than expected. I'm sorry!
This PR removes the last dependency on the IR-produced memory edges for dataflow. Some background information:
On
main
we implement dataflow through iterators by defining SSA reads from (and writes to) iterators by traversing back through the SSA graph to find the preceding call tocontainer.begin()
, and we then specify that a write to an iterator is a write to that container (and similar for reads). That is, in order to have flow in an example such as:we have an SSA write to
v
at*it = source()
.However, in order to conclude that
it
was obtained fromv
we need some kind of SSA to find the preceding call tocontainer.begin()
. Onmain
we use the SSA information from the IR. However, as we've learned many times, the sound SSA information produced by the IR isn't well-suited for dataflow. In particular:begin
call on most real-world code since everything escapes (and thus can't be soundly tracked) according to the current IR alias analysis.This PR fixes this by instead computing iterator flow after dataflow SSA has finished. Thus, we have the full power of the dataflow SSA to find the call to
begin
. This is then much easier to plug into dataflow, and gives better results overall.The most terrifying commit in this PR is 50775d0 which is a preparation to ea1b8a3 that implements
getAnUltimateDefinition()
. That's the actual predicate we use to go from*it
toit = v.begin()
, but in order to implement it properly I had to restructure quite a lot of the SSA predicates.As a small bonus of 50775d0 it fixes a small amount of spurious flow and some inconsistency errors. The effect of this can be seen in 5f0efc1.
Finally, all of this leads up to 683fe26 which instantiates the shared SSA library to reason about iterators.
Commit-by-commit heavily recommended. I've split it up in (hopefully) meaningful chunks.