Reland [clang][dataflow] Transfer more cast expressions. (#157535)

Reverts #157148

Adds fixes to `TransferVisitor::VisitCXXConstructExpr` and `copyRecord`
to avoid crashing on base class initialization from sibling-derived
class instances. I believe this is the only use of copyRecord where we
need this special handling for a shared base class.

Author: bazuzi

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

@@ -13,6 +13,7 @@
 #ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_RECORDOPS_H
 #define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_RECORDOPS_H
 
+#include "clang/AST/Type.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/StorageLocation.h"
 
@@ -36,8 +37,11 @@ namespace dataflow {
 ///    - 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).
+///    - The types of `Dest` and `Src` are both derived from a non-null
+///      `TypeToCopy` (in this case, only fields present in `TypeToCopy` are
+///      overwritten).
 void copyRecord(RecordStorageLocation &Src, RecordStorageLocation &Dst,
-                Environment &Env);
+                Environment &Env, QualType TypeToCopy = QualType());
 
 /// Returns whether the records `Loc1` and `Loc2` are equal.
 ///

clang/include/clang/Analysis/FlowSensitive/StorageLocation.h

@@ -17,6 +17,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/Type.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Debug.h"
 #include <cassert>
 
@@ -152,6 +153,11 @@ class RecordStorageLocation final : public StorageLocation {
     return {SyntheticFields.begin(), SyntheticFields.end()};
   }
 
+  /// Add a synthetic field, if none by that name is already present.
+  void addSyntheticField(llvm::StringRef Name, StorageLocation &Loc) {
+    SyntheticFields.insert({Name, &Loc});
+  }
+
   /// Changes the child storage location for a field `D` of reference type.
   /// All other fields cannot change their storage location and always retain
   /// the storage location passed to the `RecordStorageLocation` constructor.
@@ -164,6 +170,11 @@ class RecordStorageLocation final : public StorageLocation {
     Children[&D] = Loc;
   }
 
+  /// Add a child storage location for a field `D`, if not already present.
+  void addChild(const ValueDecl &D, StorageLocation *Loc) {
+    Children.insert({&D, Loc});
+  }
+
   llvm::iterator_range<FieldToLoc::const_iterator> children() const {
     return {Children.begin(), Children.end()};
   }

clang/lib/Analysis/FlowSensitive/RecordOps.cpp

@@ -11,6 +11,9 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/Analysis/FlowSensitive/RecordOps.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/Type.h"
 
 #define DEBUG_TYPE "dataflow"
 
@@ -49,25 +52,30 @@ static void copySyntheticField(QualType FieldType, StorageLocation &SrcFieldLoc,
 }
 
 void copyRecord(RecordStorageLocation &Src, RecordStorageLocation &Dst,
-                Environment &Env) {
+                Environment &Env, const QualType TypeToCopy) {
   auto SrcType = Src.getType().getCanonicalType().getUnqualifiedType();
   auto DstType = Dst.getType().getCanonicalType().getUnqualifiedType();
 
   auto SrcDecl = SrcType->getAsCXXRecordDecl();
   auto DstDecl = DstType->getAsCXXRecordDecl();
 
-  [[maybe_unused]] bool compatibleTypes =
+  const CXXRecordDecl *DeclToCopy =
+      TypeToCopy.isNull() ? nullptr : TypeToCopy->getAsCXXRecordDecl();
+
+  [[maybe_unused]] bool CompatibleTypes =
       SrcType == DstType ||
       (SrcDecl != nullptr && DstDecl != nullptr &&
-       (SrcDecl->isDerivedFrom(DstDecl) || DstDecl->isDerivedFrom(SrcDecl)));
+       (SrcDecl->isDerivedFrom(DstDecl) || DstDecl->isDerivedFrom(SrcDecl) ||
+        (DeclToCopy != nullptr && SrcDecl->isDerivedFrom(DeclToCopy) &&
+         DstDecl->isDerivedFrom(DeclToCopy))));
 
   LLVM_DEBUG({
-    if (!compatibleTypes) {
+    if (!CompatibleTypes) {
       llvm::dbgs() << "Source type " << Src.getType() << "\n";
       llvm::dbgs() << "Destination type " << Dst.getType() << "\n";
     }
   });
-  assert(compatibleTypes);
+  assert(CompatibleTypes);
 
   if (SrcType == DstType || (SrcDecl != nullptr && DstDecl != nullptr &&
                              SrcDecl->isDerivedFrom(DstDecl))) {
@@ -76,12 +84,24 @@ void copyRecord(RecordStorageLocation &Src, RecordStorageLocation &Dst,
     for (const auto &[Name, DstFieldLoc] : Dst.synthetic_fields())
       copySyntheticField(DstFieldLoc->getType(), Src.getSyntheticField(Name),
                          *DstFieldLoc, Env);
-  } else {
+  } else if (SrcDecl != nullptr && DstDecl != nullptr &&
+             DstDecl->isDerivedFrom(SrcDecl)) {
     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);
+  } else {
+    for (const FieldDecl *Field :
+         Env.getDataflowAnalysisContext().getModeledFields(TypeToCopy)) {
+      copyField(*Field, Src.getChild(*Field), Dst.getChild(*Field), Dst, Env);
+    }
+    for (const auto &[SyntheticFieldName, SyntheticFieldType] :
+         Env.getDataflowAnalysisContext().getSyntheticFields(TypeToCopy)) {
+      copySyntheticField(SyntheticFieldType,
+                         Src.getSyntheticField(SyntheticFieldName),
+                         Dst.getSyntheticField(SyntheticFieldName), Env);
+    }
   }
 }
 

clang/lib/Analysis/FlowSensitive/Transfer.cpp

@@ -20,14 +20,17 @@
 #include "clang/AST/OperationKinds.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/StmtVisitor.h"
+#include "clang/AST/Type.h"
 #include "clang/Analysis/FlowSensitive/ASTOps.h"
 #include "clang/Analysis/FlowSensitive/AdornedCFG.h"
 #include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/NoopAnalysis.h"
 #include "clang/Analysis/FlowSensitive/RecordOps.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "clang/Basic/Builtins.h"
+#include "clang/Basic/LLVM.h"
 #include "clang/Basic/OperatorKinds.h"
 #include "llvm/Support/Casting.h"
 #include <assert.h>
@@ -287,7 +290,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     }
   }
 
-  void VisitImplicitCastExpr(const ImplicitCastExpr *S) {
+  void VisitCastExpr(const CastExpr *S) {
     const Expr *SubExpr = S->getSubExpr();
     assert(SubExpr != nullptr);
 
@@ -317,17 +320,70 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
       break;
     }
 
+    case CK_BaseToDerived: {
+      // This is a cast of (single-layer) pointer or reference to a record type.
+      // We should now model the fields for the derived type.
+
+      // Get the RecordStorageLocation for the record object underneath.
+      RecordStorageLocation *Loc = nullptr;
+      if (S->getType()->isPointerType()) {
+        auto *PV = Env.get<PointerValue>(*SubExpr);
+        assert(PV != nullptr);
+        if (PV == nullptr)
+          break;
+        Loc = cast<RecordStorageLocation>(&PV->getPointeeLoc());
+      } else {
+        assert(S->getType()->isRecordType());
+        if (SubExpr->isGLValue()) {
+          Loc = Env.get<RecordStorageLocation>(*SubExpr);
+        } else {
+          Loc = &Env.getResultObjectLocation(*SubExpr);
+        }
+      }
+      if (!Loc) {
+        // Nowhere to add children or propagate from, so we're done.
+        break;
+      }
+
+      // Get the derived record type underneath the reference or pointer.
+      QualType Derived = S->getType().getNonReferenceType();
+      if (Derived->isPointerType()) {
+        Derived = Derived->getPointeeType();
+      }
+
+      // Add children to the storage location for fields (including synthetic
+      // fields) of the derived type and initialize their values.
+      for (const FieldDecl *Field :
+           Env.getDataflowAnalysisContext().getModeledFields(Derived)) {
+        assert(Field != nullptr);
+        QualType FieldType = Field->getType();
+        if (FieldType->isReferenceType()) {
+          Loc->addChild(*Field, nullptr);
+        } else {
+          Loc->addChild(*Field, &Env.createStorageLocation(FieldType));
+        }
+
+        for (const auto &Entry :
+             Env.getDataflowAnalysisContext().getSyntheticFields(Derived)) {
+          Loc->addSyntheticField(Entry.getKey(),
+                                 Env.createStorageLocation(Entry.getValue()));
+        }
+      }
+      Env.initializeFieldsWithValues(*Loc, Derived);
+
+      // Fall through to propagate SubExpr's StorageLocation to the CastExpr.
+      [[fallthrough]];
+    }
     case CK_IntegralCast:
       // FIXME: This cast creates a new integral value from the
       // subexpression. But, because we don't model integers, we don't
       // distinguish between this new value and the underlying one. If integer
       // modeling is added, then update this code to create a fresh location and
       // value.
     case CK_UncheckedDerivedToBase:
+    case CK_DerivedToBase:
     case CK_ConstructorConversion:
     case CK_UserDefinedConversion:
-      // FIXME: Add tests that excercise CK_UncheckedDerivedToBase,
-      // CK_ConstructorConversion, and CK_UserDefinedConversion.
     case CK_NoOp: {
       // FIXME: Consider making `Environment::getStorageLocation` skip noop
       // expressions (this and other similar expressions in the file) instead
@@ -554,7 +610,15 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
       // Even if the copy/move constructor call is elidable, we choose to copy
       // the record in all cases (which isn't wrong, just potentially not
       // optimal).
-      copyRecord(*ArgLoc, Loc, Env);
+      //
+      // To handle cases of base class initializers in constructors, where a
+      // sibling derived class can be used to initialize a shared-base-class
+      // subobject through a DerivedToBase cast, intentionally copy only the
+      // parts of `ArgLoc` that are part of the base class being initialized.
+      // This is necessary because the type of `Loc` in these cases is the
+      // derived type ultimately being constructed, not the type of the base
+      // class subobject.
+      copyRecord(*ArgLoc, Loc, Env, S->getType());
       return;
     }
 
@@ -684,15 +748,6 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     propagateValue(*SubExpr, *S, Env);
   }
 
-  void VisitCXXStaticCastExpr(const CXXStaticCastExpr *S) {
-    if (S->getCastKind() == CK_NoOp) {
-      const Expr *SubExpr = S->getSubExpr();
-      assert(SubExpr != nullptr);
-
-      propagateValueOrStorageLocation(*SubExpr, *S, Env);
-    }
-  }
-
   void VisitConditionalOperator(const ConditionalOperator *S) {
     const Environment *TrueEnv = StmtToEnv.getEnvironment(*S->getTrueExpr());
     const Environment *FalseEnv = StmtToEnv.getEnvironment(*S->getFalseExpr());

clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp

@@ -8,8 +8,15 @@
 
 #include "clang/Analysis/FlowSensitive/RecordOps.h"
 #include "TestingSupport.h"
+#include "clang/AST/Type.h"
+#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/NoopLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
+#include <string>
 
 namespace clang {
 namespace dataflow {
@@ -190,7 +197,7 @@ TEST(RecordOpsTest, RecordsEqual) {
       });
 }
 
-TEST(TransferTest, CopyRecordBetweenDerivedAndBase) {
+TEST(RecordOpsTest, CopyRecordBetweenDerivedAndBase) {
   std::string Code = R"(
     struct A {
       int i;
@@ -266,6 +273,67 @@ TEST(TransferTest, CopyRecordBetweenDerivedAndBase) {
       });
 }
 
+TEST(RecordOpsTest, CopyRecordWithExplicitSharedBaseTypeToCopy) {
+  std::string Code = R"(
+    struct Base {
+      bool BaseField;
+      char UnmodeledField;
+    };
+
+    struct DerivedOne : public Base {
+      int DerivedOneField;
+    };
+
+    struct DerivedTwo : public Base {
+      int DerivedTwoField;
+    };
+
+    void target(Base B, DerivedOne D1, DerivedTwo D2) {
+      (void) B.BaseField;
+      // [[p]]
+    }
+  )";
+  auto SyntheticFieldCallback = [](QualType Ty) -> llvm::StringMap<QualType> {
+    CXXRecordDecl *BaseDecl = nullptr;
+    std::string TypeAsString = Ty.getAsString();
+    if (TypeAsString == "Base")
+      BaseDecl = Ty->getAsCXXRecordDecl();
+    else if (TypeAsString == "DerivedOne" || TypeAsString == "DerivedTwo")
+      BaseDecl = Ty->getAsCXXRecordDecl()
+                     ->bases_begin()
+                     ->getType()
+                     ->getAsCXXRecordDecl();
+    else
+      return {};
+    QualType FieldType = getFieldNamed(BaseDecl, "BaseField")->getType();
+    return {{"synth_field", FieldType}};
+  };
+  // Test copying derived to base class.
+  runDataflow(
+      Code, SyntheticFieldCallback,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        Environment Env = getEnvironmentAtAnnotation(Results, "p").fork();
+
+        const ValueDecl *BaseFieldDecl = findValueDecl(ASTCtx, "BaseField");
+        auto &B = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "B");
+        auto &D1 = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "D1");
+        auto &D2 = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "D2");
+
+        EXPECT_NE(Env.getValue(*D1.getChild(*BaseFieldDecl)),
+                  Env.getValue(*D2.getChild(*BaseFieldDecl)));
+        EXPECT_NE(Env.getValue(D1.getSyntheticField("synth_field")),
+                  Env.getValue(D2.getSyntheticField("synth_field")));
+
+        copyRecord(D1, D2, Env, B.getType());
+
+        EXPECT_EQ(Env.getValue(*D1.getChild(*BaseFieldDecl)),
+                  Env.getValue(*D2.getChild(*BaseFieldDecl)));
+        EXPECT_EQ(Env.getValue(D1.getSyntheticField("synth_field")),
+                  Env.getValue(D2.getSyntheticField("synth_field")));
+      });
+}
+
 } // namespace
 } // namespace test
 } // namespace dataflow