Skip to content

Commit b788e46

Browse files
martinboehmemartinboehme
authored
[clang][dataflow] Model assignment to derived class from base. (#85064)
This is a relatively rare case, but - It's still nice to get this right, - We can remove the special case for this in `VisitCXXOperatorCallExpr()` (that simply bails out), and - With this in place, I can avoid having to add a similar special case in an upcoming patch.
1 parent eac6844 commit b788e46

File tree

5 files changed

+126
-54
lines changed

5 files changed

+126
-54
lines changed

‎clang/include/clang/Analysis/FlowSensitive/RecordOps.h

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,11 @@ namespace dataflow {
3131
///
3232
/// Requirements:
3333
///
34-
/// `Src` and `Dst` must have the same canonical unqualified type.
34+
/// Either:
35+
/// - `Src` and `Dest` must have the same canonical unqualified type, or
36+
/// - The type of `Src` must be derived from `Dest`, or
37+
/// - The type of `Dest` must be derived from `Src` (in this case, any fields
38+
/// that are only present in `Dest` are not overwritten).
3539
void copyRecord(RecordStorageLocation &Src, RecordStorageLocation &Dst,
3640
Environment &Env);
3741

‎clang/lib/Analysis/FlowSensitive/RecordOps.cpp

Lines changed: 56 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -14,18 +14,52 @@
1414

1515
#define DEBUG_TYPE "dataflow"
1616

17-
void clang::dataflow::copyRecord(RecordStorageLocation &Src,
18-
RecordStorageLocation &Dst, Environment &Env) {
17+
namespace clang::dataflow {
18+
19+
static void copyField(const ValueDecl &Field, StorageLocation *SrcFieldLoc,
20+
StorageLocation *DstFieldLoc, RecordStorageLocation &Dst,
21+
Environment &Env) {
22+
assert(Field.getType()->isReferenceType() ||
23+
(SrcFieldLoc != nullptr && DstFieldLoc != nullptr));
24+
25+
if (Field.getType()->isRecordType()) {
26+
copyRecord(cast<RecordStorageLocation>(*SrcFieldLoc),
27+
cast<RecordStorageLocation>(*DstFieldLoc), Env);
28+
} else if (Field.getType()->isReferenceType()) {
29+
Dst.setChild(Field, SrcFieldLoc);
30+
} else {
31+
if (Value *Val = Env.getValue(*SrcFieldLoc))
32+
Env.setValue(*DstFieldLoc, *Val);
33+
else
34+
Env.clearValue(*DstFieldLoc);
35+
}
36+
}
37+
38+
static void copySyntheticField(QualType FieldType, StorageLocation &SrcFieldLoc,
39+
StorageLocation &DstFieldLoc, Environment &Env) {
40+
if (FieldType->isRecordType()) {
41+
copyRecord(cast<RecordStorageLocation>(SrcFieldLoc),
42+
cast<RecordStorageLocation>(DstFieldLoc), Env);
43+
} else {
44+
if (Value *Val = Env.getValue(SrcFieldLoc))
45+
Env.setValue(DstFieldLoc, *Val);
46+
else
47+
Env.clearValue(DstFieldLoc);
48+
}
49+
}
50+
51+
void copyRecord(RecordStorageLocation &Src, RecordStorageLocation &Dst,
52+
Environment &Env) {
1953
auto SrcType = Src.getType().getCanonicalType().getUnqualifiedType();
2054
auto DstType = Dst.getType().getCanonicalType().getUnqualifiedType();
2155

2256
auto SrcDecl = SrcType->getAsCXXRecordDecl();
2357
auto DstDecl = DstType->getAsCXXRecordDecl();
2458

25-
bool compatibleTypes =
59+
[[maybe_unused]] bool compatibleTypes =
2660
SrcType == DstType ||
27-
(SrcDecl && DstDecl && SrcDecl->isDerivedFrom(DstDecl));
28-
(void)compatibleTypes;
61+
(SrcDecl != nullptr && DstDecl != nullptr &&
62+
(SrcDecl->isDerivedFrom(DstDecl) || DstDecl->isDerivedFrom(SrcDecl)));
2963

3064
LLVM_DEBUG({
3165
if (!compatibleTypes) {
@@ -35,45 +69,27 @@ void clang::dataflow::copyRecord(RecordStorageLocation &Src,
3569
});
3670
assert(compatibleTypes);
3771

38-
for (auto [Field, DstFieldLoc] : Dst.children()) {
39-
StorageLocation *SrcFieldLoc = Src.getChild(*Field);
40-
41-
assert(Field->getType()->isReferenceType() ||
42-
(SrcFieldLoc != nullptr && DstFieldLoc != nullptr));
43-
44-
if (Field->getType()->isRecordType()) {
45-
copyRecord(cast<RecordStorageLocation>(*SrcFieldLoc),
46-
cast<RecordStorageLocation>(*DstFieldLoc), Env);
47-
} else if (Field->getType()->isReferenceType()) {
48-
Dst.setChild(*Field, SrcFieldLoc);
49-
} else {
50-
if (Value *Val = Env.getValue(*SrcFieldLoc))
51-
Env.setValue(*DstFieldLoc, *Val);
52-
else
53-
Env.clearValue(*DstFieldLoc);
54-
}
55-
}
56-
57-
for (const auto &[Name, SynthFieldLoc] : Src.synthetic_fields()) {
58-
if (SynthFieldLoc->getType()->isRecordType()) {
59-
copyRecord(*cast<RecordStorageLocation>(SynthFieldLoc),
60-
cast<RecordStorageLocation>(Dst.getSyntheticField(Name)), Env);
61-
} else {
62-
if (Value *Val = Env.getValue(*SynthFieldLoc))
63-
Env.setValue(Dst.getSyntheticField(Name), *Val);
64-
else
65-
Env.clearValue(Dst.getSyntheticField(Name));
66-
}
72+
if (SrcType == DstType || (SrcDecl != nullptr && DstDecl != nullptr &&
73+
SrcDecl->isDerivedFrom(DstDecl))) {
74+
for (auto [Field, DstFieldLoc] : Dst.children())
75+
copyField(*Field, Src.getChild(*Field), DstFieldLoc, Dst, Env);
76+
for (const auto &[Name, DstFieldLoc] : Dst.synthetic_fields())
77+
copySyntheticField(DstFieldLoc->getType(), Src.getSyntheticField(Name),
78+
*DstFieldLoc, Env);
79+
} else {
80+
for (auto [Field, SrcFieldLoc] : Src.children())
81+
copyField(*Field, SrcFieldLoc, Dst.getChild(*Field), Dst, Env);
82+
for (const auto &[Name, SrcFieldLoc] : Src.synthetic_fields())
83+
copySyntheticField(SrcFieldLoc->getType(), *SrcFieldLoc,
84+
Dst.getSyntheticField(Name), Env);
6785
}
6886

6987
RecordValue *DstVal = &Env.create<RecordValue>(Dst);
7088
Env.setValue(Dst, *DstVal);
7189
}
7290

73-
bool clang::dataflow::recordsEqual(const RecordStorageLocation &Loc1,
74-
const Environment &Env1,
75-
const RecordStorageLocation &Loc2,
76-
const Environment &Env2) {
91+
bool recordsEqual(const RecordStorageLocation &Loc1, const Environment &Env1,
92+
const RecordStorageLocation &Loc2, const Environment &Env2) {
7793
LLVM_DEBUG({
7894
if (Loc2.getType().getCanonicalType().getUnqualifiedType() !=
7995
Loc1.getType().getCanonicalType().getUnqualifiedType()) {
@@ -116,3 +132,5 @@ bool clang::dataflow::recordsEqual(const RecordStorageLocation &Loc1,
116132

117133
return true;
118134
}
135+
136+
} // namespace clang::dataflow

‎clang/lib/Analysis/FlowSensitive/Transfer.cpp

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -544,15 +544,6 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
544544
if (LocSrc == nullptr || LocDst == nullptr)
545545
return;
546546

547-
// The assignment operators are different from the type of the destination
548-
// in this model (i.e. in one of their base classes). This must be very
549-
// rare and we just bail.
550-
if (Method->getFunctionObjectParameterType()
551-
.getCanonicalType()
552-
.getUnqualifiedType() !=
553-
LocDst->getType().getCanonicalType().getUnqualifiedType())
554-
return;
555-
556547
copyRecord(*LocSrc, *LocDst, Env);
557548

558549
// If the expr is a glvalue, we can reasonably assume the operator is

‎clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ TEST(RecordOpsTest, RecordsEqual) {
198198
});
199199
}
200200

201-
TEST(TransferTest, CopyRecordFromDerivedToBase) {
201+
TEST(TransferTest, CopyRecordBetweenDerivedAndBase) {
202202
std::string Code = R"(
203203
struct A {
204204
int i;
@@ -212,8 +212,23 @@ TEST(TransferTest, CopyRecordFromDerivedToBase) {
212212
// [[p]]
213213
}
214214
)";
215+
auto SyntheticFieldCallback = [](QualType Ty) -> llvm::StringMap<QualType> {
216+
CXXRecordDecl *ADecl = nullptr;
217+
if (Ty.getAsString() == "A")
218+
ADecl = Ty->getAsCXXRecordDecl();
219+
else if (Ty.getAsString() == "B")
220+
ADecl = Ty->getAsCXXRecordDecl()
221+
->bases_begin()
222+
->getType()
223+
->getAsCXXRecordDecl();
224+
else
225+
return {};
226+
QualType IntTy = getFieldNamed(ADecl, "i")->getType();
227+
return {{"synth_int", IntTy}};
228+
};
229+
// Test copying derived to base class.
215230
runDataflow(
216-
Code, /*SyntheticFieldCallback=*/{},
231+
Code, SyntheticFieldCallback,
217232
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
218233
ASTContext &ASTCtx) {
219234
Environment Env = getEnvironmentAtAnnotation(Results, "p").fork();
@@ -224,11 +239,38 @@ TEST(TransferTest, CopyRecordFromDerivedToBase) {
224239

225240
EXPECT_NE(Env.getValue(*A.getChild(*IDecl)),
226241
Env.getValue(*B.getChild(*IDecl)));
242+
EXPECT_NE(Env.getValue(A.getSyntheticField("synth_int")),
243+
Env.getValue(B.getSyntheticField("synth_int")));
227244

228245
copyRecord(B, A, Env);
229246

230247
EXPECT_EQ(Env.getValue(*A.getChild(*IDecl)),
231248
Env.getValue(*B.getChild(*IDecl)));
249+
EXPECT_EQ(Env.getValue(A.getSyntheticField("synth_int")),
250+
Env.getValue(B.getSyntheticField("synth_int")));
251+
});
252+
// Test copying base to derived class.
253+
runDataflow(
254+
Code, SyntheticFieldCallback,
255+
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
256+
ASTContext &ASTCtx) {
257+
Environment Env = getEnvironmentAtAnnotation(Results, "p").fork();
258+
259+
const ValueDecl *IDecl = findValueDecl(ASTCtx, "i");
260+
auto &A = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "a");
261+
auto &B = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "b");
262+
263+
EXPECT_NE(Env.getValue(*A.getChild(*IDecl)),
264+
Env.getValue(*B.getChild(*IDecl)));
265+
EXPECT_NE(Env.getValue(A.getSyntheticField("synth_int")),
266+
Env.getValue(B.getSyntheticField("synth_int")));
267+
268+
copyRecord(A, B, Env);
269+
270+
EXPECT_EQ(Env.getValue(*A.getChild(*IDecl)),
271+
Env.getValue(*B.getChild(*IDecl)));
272+
EXPECT_EQ(Env.getValue(A.getSyntheticField("synth_int")),
273+
Env.getValue(B.getSyntheticField("synth_int")));
232274
});
233275
}
234276

‎clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2313,8 +2313,6 @@ TEST(TransferTest, AssignmentOperator_ArgByValue) {
23132313
}
23142314

23152315
TEST(TransferTest, AssignmentOperatorFromBase) {
2316-
// This is a crash repro. We don't model the copy this case, so no
2317-
// expectations on the copied field of the base class are checked.
23182316
std::string Code = R"(
23192317
struct Base {
23202318
int base;
@@ -2326,14 +2324,33 @@ TEST(TransferTest, AssignmentOperatorFromBase) {
23262324
void target(Base B, Derived D) {
23272325
D.base = 1;
23282326
D.derived = 1;
2327+
// [[before]]
23292328
D = B;
2330-
// [[p]]
2329+
// [[after]]
23312330
}
23322331
)";
23332332
runDataflow(
23342333
Code,
23352334
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
2336-
ASTContext &ASTCtx) {});
2335+
ASTContext &ASTCtx) {
2336+
const Environment &EnvBefore =
2337+
getEnvironmentAtAnnotation(Results, "before");
2338+
const Environment &EnvAfter =
2339+
getEnvironmentAtAnnotation(Results, "after");
2340+
2341+
auto &BLoc =
2342+
getLocForDecl<RecordStorageLocation>(ASTCtx, EnvBefore, "B");
2343+
auto &DLoc =
2344+
getLocForDecl<RecordStorageLocation>(ASTCtx, EnvBefore, "D");
2345+
2346+
EXPECT_NE(getFieldValue(&BLoc, "base", ASTCtx, EnvBefore),
2347+
getFieldValue(&DLoc, "base", ASTCtx, EnvBefore));
2348+
EXPECT_EQ(getFieldValue(&BLoc, "base", ASTCtx, EnvAfter),
2349+
getFieldValue(&DLoc, "base", ASTCtx, EnvAfter));
2350+
2351+
EXPECT_EQ(getFieldValue(&DLoc, "derived", ASTCtx, EnvBefore),
2352+
getFieldValue(&DLoc, "derived", ASTCtx, EnvAfter));
2353+
});
23372354
}
23382355

23392356
TEST(TransferTest, AssignmentOperatorFromCallResult) {

0 commit comments

Comments
 (0)