[clang][dataflow] Model assignment to derived class from base.

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);
 

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

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

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}};
+  };
+  // Test copying 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")));
+      });
+  // Test copying 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")));
       });
 }
 

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) {