Revert "[clang][dataflow] Transfer more cast expressions." (llvm#157148)

Reverts llvm#153066

copyRecord crashes if copying from the RecordStorageLocation shared by
the base/derived objects after a DerivedToBase cast because the source
type is still `Derived` but the copy destination could be of a sibling
type derived from Base that has children not present in `Derived`.

For example, running the dataflow analysis over the following produces
UB by nullptr deref, or fails asserts if enabled:

```cc
struct Base {};

struct DerivedOne : public Base {
  int DerivedOneField;
};

struct DerivedTwo : public Base {
  int DerivedTwoField;

  DerivedTwo(const DerivedOne& d1)
      : Base(d1), DerivedTwoField(d1.DerivedOneField) {}
};
```

The constructor initializer for `DerivedTwoField` serves the purpose of
forcing `DerivedOneField` to be modeled, which is necessary to trigger
the crash but not the assert failure.

Author: bazuzi

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

@@ -17,7 +17,6 @@
 #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>
 
@@ -153,11 +152,6 @@ 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.
@@ -170,11 +164,6 @@ 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/Transfer.cpp

@@ -20,17 +20,14 @@
 #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>
@@ -290,7 +287,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     }
   }
 
-  void VisitCastExpr(const CastExpr *S) {
+  void VisitImplicitCastExpr(const ImplicitCastExpr *S) {
     const Expr *SubExpr = S->getSubExpr();
     assert(SubExpr != nullptr);
 
@@ -320,70 +317,17 @@ 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
@@ -740,6 +684,15 @@ 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());