[clang] [clang][dataflow] Model assignment to derived class from base. (PR #85064)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 13 05:08:03 PDT 2024
https://github.com/martinboehme created https://github.com/llvm/llvm-project/pull/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.
>From 9362adfb0d1a61cb56b897f31fd9f2ead1605383 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Wed, 13 Mar 2024 12:07:24 +0000
Subject: [PATCH] [clang][dataflow] Model assignment to derived class from
base.
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.
---
.../clang/Analysis/FlowSensitive/RecordOps.h | 6 +-
.../lib/Analysis/FlowSensitive/RecordOps.cpp | 94 +++++++++++--------
clang/lib/Analysis/FlowSensitive/Transfer.cpp | 9 --
.../Analysis/FlowSensitive/RecordOpsTest.cpp | 46 ++++++++-
.../Analysis/FlowSensitive/TransferTest.cpp | 25 ++++-
5 files changed, 126 insertions(+), 54 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/RecordOps.h b/clang/include/clang/Analysis/FlowSensitive/RecordOps.h
index 783e53e980aa2c..8fad45fc11d81e 100644
--- a/clang/include/clang/Analysis/FlowSensitive/RecordOps.h
+++ b/clang/include/clang/Analysis/FlowSensitive/RecordOps.h
@@ -31,7 +31,11 @@ namespace dataflow {
///
/// Requirements:
///
-/// `Src` and `Dst` must have the same canonical unqualified type.
+/// Either:
+/// - `Src` and `Dest` must have the same canonical unqualified type, or
+/// - The type of `Src` must be derived from `Dest`, or
+/// - The type of `Dest` must be derived from `Src` (in this case, any fields
+/// that are only present in `Dest` are not overwritten).
void copyRecord(RecordStorageLocation &Src, RecordStorageLocation &Dst,
Environment &Env);
diff --git a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp
index da4dd6dc078515..4fc4c15a07a1ce 100644
--- a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp
+++ b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp
@@ -14,18 +14,52 @@
#define DEBUG_TYPE "dataflow"
-void clang::dataflow::copyRecord(RecordStorageLocation &Src,
- RecordStorageLocation &Dst, Environment &Env) {
+namespace clang::dataflow {
+
+static void copyField(const ValueDecl *Field, StorageLocation *SrcFieldLoc,
+ StorageLocation *DstFieldLoc, RecordStorageLocation &Dst,
+ Environment &Env) {
+ assert(Field->getType()->isReferenceType() ||
+ (SrcFieldLoc != nullptr && DstFieldLoc != nullptr));
+
+ if (Field->getType()->isRecordType()) {
+ copyRecord(cast<RecordStorageLocation>(*SrcFieldLoc),
+ cast<RecordStorageLocation>(*DstFieldLoc), Env);
+ } else if (Field->getType()->isReferenceType()) {
+ Dst.setChild(*Field, SrcFieldLoc);
+ } else {
+ if (Value *Val = Env.getValue(*SrcFieldLoc))
+ Env.setValue(*DstFieldLoc, *Val);
+ else
+ Env.clearValue(*DstFieldLoc);
+ }
+}
+
+static void copySyntheticField(QualType FieldType, StorageLocation &SrcFieldLoc,
+ StorageLocation &DstFieldLoc, Environment &Env) {
+ if (FieldType->isRecordType()) {
+ copyRecord(cast<RecordStorageLocation>(SrcFieldLoc),
+ cast<RecordStorageLocation>(DstFieldLoc), Env);
+ } else {
+ if (Value *Val = Env.getValue(SrcFieldLoc))
+ Env.setValue(DstFieldLoc, *Val);
+ else
+ Env.clearValue(DstFieldLoc);
+ }
+}
+
+void copyRecord(RecordStorageLocation &Src, RecordStorageLocation &Dst,
+ Environment &Env) {
auto SrcType = Src.getType().getCanonicalType().getUnqualifiedType();
auto DstType = Dst.getType().getCanonicalType().getUnqualifiedType();
auto SrcDecl = SrcType->getAsCXXRecordDecl();
auto DstDecl = DstType->getAsCXXRecordDecl();
- bool compatibleTypes =
+ [[maybe_unused]] bool compatibleTypes =
SrcType == DstType ||
- (SrcDecl && DstDecl && SrcDecl->isDerivedFrom(DstDecl));
- (void)compatibleTypes;
+ (SrcDecl != nullptr && DstDecl != nullptr &&
+ (SrcDecl->isDerivedFrom(DstDecl) || DstDecl->isDerivedFrom(SrcDecl)));
LLVM_DEBUG({
if (!compatibleTypes) {
@@ -35,45 +69,27 @@ void clang::dataflow::copyRecord(RecordStorageLocation &Src,
});
assert(compatibleTypes);
- for (auto [Field, DstFieldLoc] : Dst.children()) {
- StorageLocation *SrcFieldLoc = Src.getChild(*Field);
-
- assert(Field->getType()->isReferenceType() ||
- (SrcFieldLoc != nullptr && DstFieldLoc != nullptr));
-
- if (Field->getType()->isRecordType()) {
- copyRecord(cast<RecordStorageLocation>(*SrcFieldLoc),
- cast<RecordStorageLocation>(*DstFieldLoc), Env);
- } else if (Field->getType()->isReferenceType()) {
- Dst.setChild(*Field, SrcFieldLoc);
- } else {
- if (Value *Val = Env.getValue(*SrcFieldLoc))
- Env.setValue(*DstFieldLoc, *Val);
- else
- Env.clearValue(*DstFieldLoc);
- }
- }
-
- for (const auto &[Name, SynthFieldLoc] : Src.synthetic_fields()) {
- if (SynthFieldLoc->getType()->isRecordType()) {
- copyRecord(*cast<RecordStorageLocation>(SynthFieldLoc),
- cast<RecordStorageLocation>(Dst.getSyntheticField(Name)), Env);
- } else {
- if (Value *Val = Env.getValue(*SynthFieldLoc))
- Env.setValue(Dst.getSyntheticField(Name), *Val);
- else
- Env.clearValue(Dst.getSyntheticField(Name));
- }
+ if (SrcType == DstType || (SrcDecl != nullptr && DstDecl != nullptr &&
+ SrcDecl->isDerivedFrom(DstDecl))) {
+ for (auto [Field, DstFieldLoc] : Dst.children())
+ copyField(Field, Src.getChild(*Field), DstFieldLoc, Dst, Env);
+ for (const auto &[Name, DstFieldLoc] : Dst.synthetic_fields())
+ copySyntheticField(DstFieldLoc->getType(), Src.getSyntheticField(Name),
+ *DstFieldLoc, Env);
+ } else {
+ for (auto [Field, SrcFieldLoc] : Src.children())
+ copyField(Field, SrcFieldLoc, Dst.getChild(*Field), Dst, Env);
+ for (const auto &[Name, SrcFieldLoc] : Src.synthetic_fields())
+ copySyntheticField(SrcFieldLoc->getType(), *SrcFieldLoc,
+ Dst.getSyntheticField(Name), Env);
}
RecordValue *DstVal = &Env.create<RecordValue>(Dst);
Env.setValue(Dst, *DstVal);
}
-bool clang::dataflow::recordsEqual(const RecordStorageLocation &Loc1,
- const Environment &Env1,
- const RecordStorageLocation &Loc2,
- const Environment &Env2) {
+bool recordsEqual(const RecordStorageLocation &Loc1, const Environment &Env1,
+ const RecordStorageLocation &Loc2, const Environment &Env2) {
LLVM_DEBUG({
if (Loc2.getType().getCanonicalType().getUnqualifiedType() !=
Loc1.getType().getCanonicalType().getUnqualifiedType()) {
@@ -116,3 +132,5 @@ bool clang::dataflow::recordsEqual(const RecordStorageLocation &Loc1,
return true;
}
+
+} // namespace clang::dataflow
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 04aa2831df0558..14060cbc5f3358 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -525,15 +525,6 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
if (LocSrc == nullptr || LocDst == nullptr)
return;
- // The assignment operators are different from the type of the destination
- // in this model (i.e. in one of their base classes). This must be very
- // rare and we just bail.
- if (Method->getFunctionObjectParameterType()
- .getCanonicalType()
- .getUnqualifiedType() !=
- LocDst->getType().getCanonicalType().getUnqualifiedType())
- return;
-
copyRecord(*LocSrc, *LocDst, Env);
// If the expr is a glvalue, we can reasonably assume the operator is
diff --git a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp
index cd6a37d370e854..dd109eb90984eb 100644
--- a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp
@@ -198,7 +198,7 @@ TEST(RecordOpsTest, RecordsEqual) {
});
}
-TEST(TransferTest, CopyRecordFromDerivedToBase) {
+TEST(TransferTest, CopyRecordBetweenDerivedAndBase) {
std::string Code = R"(
struct A {
int i;
@@ -212,8 +212,23 @@ TEST(TransferTest, CopyRecordFromDerivedToBase) {
// [[p]]
}
)";
+ auto SyntheticFieldCallback = [](QualType Ty) -> llvm::StringMap<QualType> {
+ CXXRecordDecl *ADecl = nullptr;
+ if (Ty.getAsString() == "A")
+ ADecl = Ty->getAsCXXRecordDecl();
+ else if (Ty.getAsString() == "B")
+ ADecl = Ty->getAsCXXRecordDecl()
+ ->bases_begin()
+ ->getType()
+ ->getAsCXXRecordDecl();
+ else
+ return {};
+ QualType IntTy = getFieldNamed(ADecl, "i")->getType();
+ return {{"synth_int", IntTy}};
+ };
+ // Copy derived to base class.
runDataflow(
- Code, /*SyntheticFieldCallback=*/{},
+ Code, SyntheticFieldCallback,
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
ASTContext &ASTCtx) {
Environment Env = getEnvironmentAtAnnotation(Results, "p").fork();
@@ -224,11 +239,38 @@ TEST(TransferTest, CopyRecordFromDerivedToBase) {
EXPECT_NE(Env.getValue(*A.getChild(*IDecl)),
Env.getValue(*B.getChild(*IDecl)));
+ EXPECT_NE(Env.getValue(A.getSyntheticField("synth_int")),
+ Env.getValue(B.getSyntheticField("synth_int")));
copyRecord(B, A, Env);
EXPECT_EQ(Env.getValue(*A.getChild(*IDecl)),
Env.getValue(*B.getChild(*IDecl)));
+ EXPECT_EQ(Env.getValue(A.getSyntheticField("synth_int")),
+ Env.getValue(B.getSyntheticField("synth_int")));
+ });
+ // Copy base to derived class.
+ runDataflow(
+ Code, SyntheticFieldCallback,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ Environment Env = getEnvironmentAtAnnotation(Results, "p").fork();
+
+ const ValueDecl *IDecl = findValueDecl(ASTCtx, "i");
+ auto &A = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "a");
+ auto &B = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "b");
+
+ EXPECT_NE(Env.getValue(*A.getChild(*IDecl)),
+ Env.getValue(*B.getChild(*IDecl)));
+ EXPECT_NE(Env.getValue(A.getSyntheticField("synth_int")),
+ Env.getValue(B.getSyntheticField("synth_int")));
+
+ copyRecord(A, B, Env);
+
+ EXPECT_EQ(Env.getValue(*A.getChild(*IDecl)),
+ Env.getValue(*B.getChild(*IDecl)));
+ EXPECT_EQ(Env.getValue(A.getSyntheticField("synth_int")),
+ Env.getValue(B.getSyntheticField("synth_int")));
});
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index a8c282f140b4cd..0e9255737f6d64 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2313,8 +2313,6 @@ TEST(TransferTest, AssignmentOperator_ArgByValue) {
}
TEST(TransferTest, AssignmentOperatorFromBase) {
- // This is a crash repro. We don't model the copy this case, so no
- // expectations on the copied field of the base class are checked.
std::string Code = R"(
struct Base {
int base;
@@ -2326,14 +2324,33 @@ TEST(TransferTest, AssignmentOperatorFromBase) {
void target(Base B, Derived D) {
D.base = 1;
D.derived = 1;
+ // [[before]]
D = B;
- // [[p]]
+ // [[after]]
}
)";
runDataflow(
Code,
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
- ASTContext &ASTCtx) {});
+ ASTContext &ASTCtx) {
+ const Environment &EnvBefore =
+ getEnvironmentAtAnnotation(Results, "before");
+ const Environment &EnvAfter =
+ getEnvironmentAtAnnotation(Results, "after");
+
+ auto &BLoc =
+ getLocForDecl<RecordStorageLocation>(ASTCtx, EnvBefore, "B");
+ auto &DLoc =
+ getLocForDecl<RecordStorageLocation>(ASTCtx, EnvBefore, "D");
+
+ EXPECT_NE(getFieldValue(&BLoc, "base", ASTCtx, EnvBefore),
+ getFieldValue(&DLoc, "base", ASTCtx, EnvBefore));
+ EXPECT_EQ(getFieldValue(&BLoc, "base", ASTCtx, EnvAfter),
+ getFieldValue(&DLoc, "base", ASTCtx, EnvAfter));
+
+ EXPECT_EQ(getFieldValue(&DLoc, "derived", ASTCtx, EnvBefore),
+ getFieldValue(&DLoc, "derived", ASTCtx, EnvAfter));
+ });
}
TEST(TransferTest, AssignmentOperatorFromCallResult) {
More information about the cfe-commits
mailing list