-
Notifications
You must be signed in to change notification settings - Fork 1.7k
C++: Update static call target resolution semantics in dataflow #19500
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++: Update static call target resolution semantics in dataflow #19500
Conversation
… when there are model-generated summaries AND source definitions.
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.
Pull Request Overview
This PR updates the static call-target resolution in C/C++ dataflow so that manual summaries take priority, then source definitions, then generated summaries. It adds new tests and model entries to validate this order and revises the core DataFlow logic accordingly.
- Add new test functions and calls in
test.cpp
to cover manual vs. generated summaries with and without bodies - Extend
steps.ext.yml
andflow.ext.yml
with manual and generated model entries - Update expected output files to reflect the new dispatch order
- Change
getStaticCallTarget()
inDataFlowPrivate.qll
to implement the prioritized selection
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
cpp/ql/test/library-tests/dataflow/external-models/test.cpp | Add declarations and test calls for manual & generated summaries |
cpp/ql/test/library-tests/dataflow/external-models/steps.ext.yml | Register new manual and generated summaries for test functions |
cpp/ql/test/library-tests/dataflow/external-models/flow.ext.yml | Register new manual and generated flow models |
cpp/ql/test/library-tests/dataflow/external-models/*.expected | Update expected sources, sinks, and flows for new resolution order |
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll | Implement prioritized dispatch in getStaticCallTarget() |
Comments suppressed due to low confidence (1)
cpp/ql/test/library-tests/dataflow/external-models/test.cpp:17
- [nitpick] Variable names like
y
,z
,y2
,y3
can be ambiguous; consider using descriptive names such asmanualResult
orgeneratedResult
for clarity.
int y = ymlStepManual(x);
* - If there is a manual summary then we always use the manual summary. | ||
* - If there is a source callable and we only have generated summaries | ||
* we use the source callable. | ||
* - If there is no source callable then we use the summary regardless of | ||
* whether is it manual or generated. |
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.
[nitpick] Consider rephrasing for clarity, e.g. 'If there is a source callable and no manual summary exists, use the source callable.'
* - If there is a manual summary then we always use the manual summary. | |
* - If there is a source callable and we only have generated summaries | |
* we use the source callable. | |
* - If there is no source callable then we use the summary regardless of | |
* whether is it manual or generated. | |
* - If there is a manual summary, then we always use the manual summary. | |
* - If there is a source callable and no manual summary exists, use the source callable. | |
* - If there is no source callable, then we use the summary regardless of | |
* whether it is manual or generated. |
Copilot uses AI. Check for mistakes.
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll
Outdated
Show resolved
Hide resolved
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.
LGTM
Up until now C/C++ has only had the following two kinds of functions:
And with only those two cases it was easy to choose which function to dispatch to in dataflow: If there was a summary always use that one (since it was always manually created and thus it probably captured all the desired behavior).
However, now that we're adding lots of generated models we need to rethink this since generated models are often less precise. So now we have:
This PR changes the semantics to the following:
This PR is likely to be a no-op right now since we don't actually have any generated summaries. However, I've added a testcase to check that we capture the right behavior.
Commit-by-commit review recommended