Skip to content

Commit b5b8165

Browse files
MathiasVPMathiasVP
authored
Merge pull request #22654 from MathiasVP/fix-forward-interpretation
C++: Improve logic for perfect forwarding
2 parents ed5f381 + 3113489 commit b5b8165

9 files changed

Lines changed: 1008 additions & 63 deletions

File tree

‎cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll‎

Lines changed: 398 additions & 13 deletions
Large diffs are not rendered by default.

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowNodes.qll‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ private import semmle.code.cpp.ir.ValueNumbering
33
private import semmle.code.cpp.ir.IR
44
private import semmle.code.cpp.models.interfaces.DataFlow
55
private import semmle.code.cpp.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
6+
private import semmle.code.cpp.dataflow.ExternalFlow as External
67
private import DataFlowPrivate
78
private import DataFlowUtil
89
private import ModelUtil
@@ -192,7 +193,7 @@ private module Cached {
192193
TSsaSynthNode(SsaImpl::SynthNode n) or
193194
TSsaIteratorNode(IteratorFlow::IteratorFlowNode n) or
194195
TForwarderConstructorArgumentNode(CallInstruction call) {
195-
isForwarderConstructorArgumentNodeImpl(call)
196+
External::ConstructorForwarding::isForwarderConstructorArgumentNodeImpl(call)
196197
} or
197198
TRawIndirectOperand0(Node0Impl node, int indirectionIndex) {
198199
SsaImpl::hasRawIndirectOperand(node.asOperand(), indirectionIndex)

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll‎

Lines changed: 4 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -593,52 +593,6 @@ private class SideEffectArgumentNode extends ArgumentNode, SideEffectOperandNode
593593
}
594594
}
595595

596-
/**
597-
* Gets `unspecifiedType`, but with the outermost `ReferenceType` removed, if any.
598-
*/
599-
private Type stripReferences(Type unspecifiedType) {
600-
result = unspecifiedType.(Cpp::ReferenceType).getBaseType().getUnspecifiedType()
601-
or
602-
not unspecifiedType instanceof Cpp::ReferenceType and
603-
result = unspecifiedType
604-
}
605-
606-
predicate forwardingCallTargetsConstructor(
607-
CallInstruction call, Cpp::Constructor constructor, int start
608-
) {
609-
exists(int numberOfForwardedArguments |
610-
numberOfForwardedArguments <= constructor.getNumberOfParameters()
611-
or
612-
constructor.isVarargs()
613-
|
614-
External::forwards(call.getStaticCallTarget(), constructor, start) and
615-
call.getNumberOfPositionalArguments() = start + numberOfForwardedArguments and
616-
forall(int i | i = [0 .. constructor.getNumberOfParameters() - 1] |
617-
// If we are still processing the forwarded arguments then we need to
618-
// check that the argument types match the parameter types.
619-
// Functions that perform perfect forwarding are always written as:
620-
// ```
621-
// template<typename... Args> void emplace(Args&&... args) { ... }
622-
// ```
623-
// and so all the arguments will be reference typed (lvalue or rvalued).
624-
// However, the constructor may not specify all the arguments by
625-
// reference.
626-
i < numberOfForwardedArguments and
627-
stripReferences(call.getPositionalArgument(start + i).getResultType()) =
628-
stripReferences(constructor.getParameter(i).getUnspecifiedType())
629-
or
630-
// If the constructor has a default argument and we have processed all
631-
// the forwarded arguments then we don't need to check the types.
632-
i >= numberOfForwardedArguments and constructor.getParameter(i).hasInitializer()
633-
)
634-
)
635-
}
636-
637-
/** Holds if `call` is a call that forwards arguments to a constructor call. */
638-
predicate isForwarderConstructorArgumentNodeImpl(CallInstruction call) {
639-
forwardingCallTargetsConstructor(call, _, _)
640-
}
641-
642596
/**
643597
* In order to implement a MaD summary for a flow such as:
644598
* ```
@@ -679,7 +633,10 @@ private class ForwarderConstructorArgumentNode extends ArgumentNode,
679633
/**
680634
* Gets a constructor which may be targeted by this forwarding call.
681635
*/
682-
Cpp::Constructor getAConstructor() { forwardingCallTargetsConstructor(call, result, _) }
636+
Cpp::Constructor getAConstructor() {
637+
result =
638+
External::ConstructorForwarding::getForwardingConstructor(call.getStaticCallTarget(), _)
639+
}
683640

684641
override DataFlowCallable getEnclosingCallable() {
685642
result.asSourceCallable() = this.getFunction()

‎cpp/ql/test/library-tests/dataflow/external-models/flow.expected‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1100,22 +1100,40 @@ edges
11001100
| test.cpp:362:15:362:23 | call to ymlSource | test.cpp:362:15:362:25 | call to ymlSource | provenance | Src:MaD:48 |
11011101
| test.cpp:362:15:362:25 | call to ymlSource | test.cpp:363:15:363:15 | *x | provenance | |
11021102
| test.cpp:363:5:363:5 | forward output argument [s] | test.cpp:365:30:365:30 | *f [s] | provenance | |
1103+
| test.cpp:363:5:363:5 | forward output argument [ul] | test.cpp:365:30:365:30 | *f [ul] | provenance | |
11031104
| test.cpp:363:15:363:15 | *x | test.cpp:341:30:341:32 | arg | provenance | |
1105+
| test.cpp:363:15:363:15 | *x | test.cpp:345:38:345:40 | arg | provenance | |
11041106
| test.cpp:363:15:363:15 | *x | test.cpp:363:5:363:5 | forward output argument [s] | provenance | |
1107+
| test.cpp:363:15:363:15 | *x | test.cpp:363:5:363:5 | forward output argument [ul] | provenance | |
11051108
| test.cpp:365:30:365:30 | *f [s] | test.cpp:365:32:365:34 | call to get [s] | provenance | MaD:88 |
1109+
| test.cpp:365:30:365:30 | *f [ul] | test.cpp:365:32:365:34 | call to get [ul] | provenance | MaD:88 |
11061110
| test.cpp:365:32:365:34 | call to get [s] | test.cpp:365:32:365:34 | call to get [s] | provenance | |
11071111
| test.cpp:365:32:365:34 | call to get [s] | test.cpp:366:13:366:13 | *c [s] | provenance | |
1112+
| test.cpp:365:32:365:34 | call to get [ul] | test.cpp:365:32:365:34 | call to get [ul] | provenance | |
1113+
| test.cpp:365:32:365:34 | call to get [ul] | test.cpp:367:13:367:13 | *c [ul] | provenance | |
11081114
| test.cpp:366:13:366:13 | *c [s] | test.cpp:366:13:366:15 | s | provenance | |
11091115
| test.cpp:366:13:366:13 | *c [s] | test.cpp:366:15:366:15 | s | provenance | Sink:MaD:3 |
11101116
| test.cpp:366:13:366:15 | s | test.cpp:366:15:366:15 | s | provenance | Sink:MaD:3 |
1117+
| test.cpp:367:13:367:13 | *c [ul] | test.cpp:367:13:367:16 | ul | provenance | |
1118+
| test.cpp:367:13:367:13 | *c [ul] | test.cpp:367:15:367:16 | ul | provenance | Sink:MaD:3 |
1119+
| test.cpp:367:13:367:16 | ul | test.cpp:367:15:367:16 | ul | provenance | Sink:MaD:3 |
11111120
| test.cpp:371:24:371:32 | call to ymlSource | test.cpp:371:24:371:34 | call to ymlSource | provenance | Src:MaD:48 |
11121121
| test.cpp:371:24:371:34 | call to ymlSource | test.cpp:372:15:372:16 | *ul | provenance | |
1122+
| test.cpp:372:5:372:5 | forward output argument [s] | test.cpp:374:30:374:30 | *f [s] | provenance | |
11131123
| test.cpp:372:5:372:5 | forward output argument [ul] | test.cpp:374:30:374:30 | *f [ul] | provenance | |
1124+
| test.cpp:372:15:372:16 | *ul | test.cpp:341:30:341:32 | arg | provenance | |
11141125
| test.cpp:372:15:372:16 | *ul | test.cpp:345:38:345:40 | arg | provenance | |
1126+
| test.cpp:372:15:372:16 | *ul | test.cpp:372:5:372:5 | forward output argument [s] | provenance | |
11151127
| test.cpp:372:15:372:16 | *ul | test.cpp:372:5:372:5 | forward output argument [ul] | provenance | |
1128+
| test.cpp:374:30:374:30 | *f [s] | test.cpp:374:32:374:34 | call to get [s] | provenance | MaD:88 |
11161129
| test.cpp:374:30:374:30 | *f [ul] | test.cpp:374:32:374:34 | call to get [ul] | provenance | MaD:88 |
1130+
| test.cpp:374:32:374:34 | call to get [s] | test.cpp:374:32:374:34 | call to get [s] | provenance | |
1131+
| test.cpp:374:32:374:34 | call to get [s] | test.cpp:375:13:375:13 | *c [s] | provenance | |
11171132
| test.cpp:374:32:374:34 | call to get [ul] | test.cpp:374:32:374:34 | call to get [ul] | provenance | |
11181133
| test.cpp:374:32:374:34 | call to get [ul] | test.cpp:376:13:376:13 | *c [ul] | provenance | |
1134+
| test.cpp:375:13:375:13 | *c [s] | test.cpp:375:13:375:15 | s | provenance | |
1135+
| test.cpp:375:13:375:13 | *c [s] | test.cpp:375:15:375:15 | s | provenance | Sink:MaD:3 |
1136+
| test.cpp:375:13:375:15 | s | test.cpp:375:15:375:15 | s | provenance | Sink:MaD:3 |
11191137
| test.cpp:376:13:376:13 | *c [ul] | test.cpp:376:13:376:16 | ul | provenance | |
11201138
| test.cpp:376:13:376:13 | *c [ul] | test.cpp:376:15:376:16 | ul | provenance | Sink:MaD:3 |
11211139
| test.cpp:376:13:376:16 | ul | test.cpp:376:15:376:16 | ul | provenance | Sink:MaD:3 |
@@ -2314,20 +2332,34 @@ nodes
23142332
| test.cpp:362:15:362:23 | call to ymlSource | semmle.label | call to ymlSource |
23152333
| test.cpp:362:15:362:25 | call to ymlSource | semmle.label | call to ymlSource |
23162334
| test.cpp:363:5:363:5 | forward output argument [s] | semmle.label | forward output argument [s] |
2335+
| test.cpp:363:5:363:5 | forward output argument [ul] | semmle.label | forward output argument [ul] |
23172336
| test.cpp:363:15:363:15 | *x | semmle.label | *x |
23182337
| test.cpp:365:30:365:30 | *f [s] | semmle.label | *f [s] |
2338+
| test.cpp:365:30:365:30 | *f [ul] | semmle.label | *f [ul] |
23192339
| test.cpp:365:32:365:34 | call to get [s] | semmle.label | call to get [s] |
23202340
| test.cpp:365:32:365:34 | call to get [s] | semmle.label | call to get [s] |
2341+
| test.cpp:365:32:365:34 | call to get [ul] | semmle.label | call to get [ul] |
2342+
| test.cpp:365:32:365:34 | call to get [ul] | semmle.label | call to get [ul] |
23212343
| test.cpp:366:13:366:13 | *c [s] | semmle.label | *c [s] |
23222344
| test.cpp:366:13:366:15 | s | semmle.label | s |
23232345
| test.cpp:366:15:366:15 | s | semmle.label | s |
2346+
| test.cpp:367:13:367:13 | *c [ul] | semmle.label | *c [ul] |
2347+
| test.cpp:367:13:367:16 | ul | semmle.label | ul |
2348+
| test.cpp:367:15:367:16 | ul | semmle.label | ul |
23242349
| test.cpp:371:24:371:32 | call to ymlSource | semmle.label | call to ymlSource |
23252350
| test.cpp:371:24:371:34 | call to ymlSource | semmle.label | call to ymlSource |
2351+
| test.cpp:372:5:372:5 | forward output argument [s] | semmle.label | forward output argument [s] |
23262352
| test.cpp:372:5:372:5 | forward output argument [ul] | semmle.label | forward output argument [ul] |
23272353
| test.cpp:372:15:372:16 | *ul | semmle.label | *ul |
2354+
| test.cpp:374:30:374:30 | *f [s] | semmle.label | *f [s] |
23282355
| test.cpp:374:30:374:30 | *f [ul] | semmle.label | *f [ul] |
2356+
| test.cpp:374:32:374:34 | call to get [s] | semmle.label | call to get [s] |
2357+
| test.cpp:374:32:374:34 | call to get [s] | semmle.label | call to get [s] |
23292358
| test.cpp:374:32:374:34 | call to get [ul] | semmle.label | call to get [ul] |
23302359
| test.cpp:374:32:374:34 | call to get [ul] | semmle.label | call to get [ul] |
2360+
| test.cpp:375:13:375:13 | *c [s] | semmle.label | *c [s] |
2361+
| test.cpp:375:13:375:15 | s | semmle.label | s |
2362+
| test.cpp:375:15:375:15 | s | semmle.label | s |
23312363
| test.cpp:376:13:376:13 | *c [ul] | semmle.label | *c [ul] |
23322364
| test.cpp:376:13:376:16 | ul | semmle.label | ul |
23332365
| test.cpp:376:15:376:16 | ul | semmle.label | ul |
@@ -2626,6 +2658,8 @@ subpaths
26262658
| test.cpp:32:41:32:41 | x | test.cpp:7:47:7:52 | value2 | test.cpp:7:5:7:30 | *ymlStepGenerated_with_body | test.cpp:32:11:32:36 | call to ymlStepGenerated_with_body |
26272659
| test.cpp:172:51:172:51 | x | test.cpp:164:34:164:34 | x | test.cpp:164:7:164:7 | *templateFunction3 | test.cpp:172:13:172:44 | call to templateFunction3 |
26282660
| test.cpp:363:15:363:15 | *x | test.cpp:341:30:341:32 | arg | test.cpp:341:3:341:22 | *this [Return] [s] | test.cpp:363:5:363:5 | forward output argument [s] |
2661+
| test.cpp:363:15:363:15 | *x | test.cpp:345:38:345:40 | arg | test.cpp:345:3:345:22 | *this [Return] [ul] | test.cpp:363:5:363:5 | forward output argument [ul] |
2662+
| test.cpp:372:15:372:16 | *ul | test.cpp:341:30:341:32 | arg | test.cpp:341:3:341:22 | *this [Return] [s] | test.cpp:372:5:372:5 | forward output argument [s] |
26292663
| test.cpp:372:15:372:16 | *ul | test.cpp:345:38:345:40 | arg | test.cpp:345:3:345:22 | *this [Return] [ul] | test.cpp:372:5:372:5 | forward output argument [ul] |
26302664
| test.cpp:443:18:443:18 | *x | test.cpp:435:34:435:38 | first | test.cpp:435:3:435:28 | *this [Return] [x] | test.cpp:443:5:443:5 | emplace output argument [element, x] |
26312665
| test.cpp:453:21:453:21 | *x | test.cpp:436:39:436:44 | second | test.cpp:436:3:436:28 | *this [Return] [x] | test.cpp:453:5:453:5 | emplace output argument [element, x] |

‎cpp/ql/test/library-tests/dataflow/external-models/test.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -364,15 +364,15 @@ void forward_test() {
364364

365365
ConstructableFromInt c = f.get();
366366
ymlSink(c.s); // $ ir
367-
ymlSink(c.ul); // clean
367+
ymlSink(c.ul); // $ SPURIOUS: ir
368368
}
369369
{
370370
Forwarder<ConstructableFromInt> f;
371371
unsigned long ul = ymlSource();
372372
f.forward(ul);
373373

374374
ConstructableFromInt c = f.get();
375-
ymlSink(c.s); // clean
375+
ymlSink(c.s); // $ SPURIOUS: ir
376376
ymlSink(c.ul); // $ ir
377377
}
378378
}

0 commit comments

Comments
 (0)