[clang][dataflow] Model assignment to derived class from base. #85064
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: None (martinboehme) ChangesThis is a relatively rare case, but
Full diff: https://github.com/llvm/llvm-project/pull/85064.diff 5 Files Affected:
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) {
|
Xazax-hun
reviewed
Mar 13, 2024
Xazax-hun
approved these changes
Mar 18, 2024
Xazax-hun
approved these changes
Mar 18, 2024
chencha3
pushed a commit
to chencha3/llvm-project
that referenced
this pull request
Mar 23, 2024
…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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a relatively rare case, but
VisitCXXOperatorCallExpr()(thatsimply bails out), and
upcoming patch.