Skip to content

C++: Use the shared type-tracking library for virtual dispatch resolution #20249

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Aug 21, 2025
Merged

C++: Use the shared type-tracking library for virtual dispatch resolution #20249

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c1b91db
C++: Add more virtual dispatch tests.
MathiasVP Aug 19, 2025
42fcfca
C++: Remove the old virtual dispatch case from 'defaultViableCallable…
MathiasVP Aug 19, 2025
fdb9f7b
C++: Move these predicates to make the diff smaller.
MathiasVP Aug 19, 2025
caf7464
C++: Prefix with 'DataflowPrivate'.
MathiasVP Aug 19, 2025
d4188d5
C++: Instantiate the type tracking module inside a reusable module li…
MathiasVP Aug 19, 2025
383799c
C++: Perform 6 rounds of virtual dispatch resolution like Java.
MathiasVP Aug 19, 2025
cca5bd9
C++: Update 'mayBenefitFromCallContext' to not use the old virtual di…
MathiasVP Aug 19, 2025
302d35b
C++: Accept test changes.
MathiasVP Aug 19, 2025
16508b1
C++: Fix off-by-one error in getType on 'FinalGlobalValue' nodes and …
MathiasVP Aug 19, 2025
0631bd7
C++: Add object/flow conflation for unions when resolving function po…
MathiasVP Aug 19, 2025
02bf923
C++: Add change note.
MathiasVP Aug 19, 2025
70d3e69
C++: Rename 'lambda' to 'virtual'.
MathiasVP Aug 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,15 @@ private import DataFlowImplCommon as DataFlowImplCommon
* from `AdditionalCallTarget` into account.
*/
cached
DataFlowCallable defaultViableCallable(DataFlowCall call) {
DataFlowPrivate::DataFlowCallable defaultViableCallable(DataFlowPrivate::DataFlowCall call) {
result = defaultViableCallableWithoutLambda(call)
or
result = DataFlowImplCommon::viableCallableLambda(call, _)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this extra case needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is coming from 42fcfca

Copy link
Contributor Author

Choose a reason for hiding this comment

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

defaultViableCallable should also resolve function pointers.

On main we essentially have one way of dealing with both function pointer resolution and virtual dispatch resolution. That's the line marked as // Virtual dispatch here (which uses the old library that we're deleting in this PR). Then in #17788 we added another way of handling function pointer resolution.

So on main we currently have one way of dealing with virtual functions, and two ways of dealing with function pointers (i.e., the // Virtual dispatch way and the lambda flow one I added in #17788). Sadly, only one of those function pointer resolutions were exposed in defaultViableCallable (the one marked as // Virtual dispatch).

This PR totally removes the old library that did both function pointer resolution and virtual dispatch, and instead we now only have one library for virtual dispatch, and one library for function pointers. So in order to continue to resolve function pointers in defaultViableCallable we now need to expose the lambda flow resolution since that's the canonical way we now resolve function pointer calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not helping me, as it basically reiterates what is written in the PR description. Might be most effective to have a brief call about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally fair. Happy to have a chat about it tomorrow (or whenever it fits you)

}

private DataFlowPrivate::DataFlowCallable defaultViableCallableWithoutLambda(
DataFlowPrivate::DataFlowCall call
) {
DataFlowImplCommon::forceCachingInSameStage() and
result = call.getStaticCallTarget()
or
Expand All @@ -26,17 +34,13 @@ DataFlowCallable defaultViableCallable(DataFlowCall call) {
functionSignatureWithBody(qualifiedName, nparams, result.getUnderlyingCallable()) and
strictcount(Function other | functionSignatureWithBody(qualifiedName, nparams, other)) = 1
)
or
// Virtual dispatch
result.asSourceCallable() = call.(VirtualDispatch::DataSensitiveCall).resolve()
}

/**
* Gets a function that might be called by `call`.
*/
cached
DataFlowCallable viableCallable(DataFlowCall call) {
result = defaultViableCallable(call)
private DataFlowPrivate::DataFlowCallable nonVirtualDispatch(DataFlowPrivate::DataFlowCall call) {
result = defaultViableCallableWithoutLambda(call)
or
// Additional call targets
result.getUnderlyingCallable() =
Expand Down