[clang][dataflow] Eliminate ReferenceValue.

There are no remaining uses of this class in the framework.

This patch is part of the ongoing migration to strict handling of value categories (see https://discourse.llvm.org/t/70086 for details).

Reviewed By: ymandel, xazax.hun, gribozavr2

Differential Revision: https://reviews.llvm.org/D155922

Author: martinboehme

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

@@ -47,6 +47,7 @@ enum class SkipPast {
   /// No indirections should be skipped past.
   None,
   /// An optional reference should be skipped past.
+  /// This is deprecated; it is equivalent to `None` and will be removed.
   Reference,
 };
 
@@ -304,9 +305,10 @@ class Environment {
   ///  `E` must be a glvalue or a `BuiltinType::BuiltinFn`
   void setStorageLocationStrict(const Expr &E, StorageLocation &Loc);
 
-  /// Returns the storage location assigned to `E` in the environment, applying
-  /// the `SP` policy for skipping past indirections, or null if `E` isn't
-  /// assigned a storage location in the environment.
+  /// Returns the storage location assigned to `E` in the environment, or null
+  /// if `E` isn't assigned a storage location in the environment.
+  ///
+  /// The `SP` parameter has no effect.
   ///
   /// This function is deprecated; prefer `getStorageLocationStrict()`.
   /// For details, see https://discourse.llvm.org/t/70086.
@@ -490,12 +492,14 @@ class Environment {
   /// isn't assigned a value in the environment.
   Value *getValue(const StorageLocation &Loc) const;
 
-  /// Equivalent to `getValue(getStorageLocation(D, SP), SkipPast::None)` if `D`
-  /// is assigned a storage location in the environment, otherwise returns null.
+  /// Equivalent to `getValue(getStorageLocation(D))` if `D` is assigned a
+  /// storage location in the environment, otherwise returns null.
   Value *getValue(const ValueDecl &D) const;
 
-  /// Equivalent to `getValue(getStorageLocation(E, SP), SkipPast::None)` if `E`
-  /// is assigned a storage location in the environment, otherwise returns null.
+  /// Equivalent to `getValue(getStorageLocation(E, SP))` if `E` is assigned a
+  /// storage location in the environment, otherwise returns null.
+  ///
+  /// The `SP` parameter has no effect.
   ///
   /// This function is deprecated; prefer `getValueStrict()`. For details, see
   /// https://discourse.llvm.org/t/70086.
@@ -672,9 +676,6 @@ class Environment {
   StorageLocation &createObjectInternal(const VarDecl *D, QualType Ty,
                                         const Expr *InitExpr);
 
-  StorageLocation &skip(StorageLocation &Loc, SkipPast SP) const;
-  const StorageLocation &skip(const StorageLocation &Loc, SkipPast SP) const;
-
   /// Shared implementation of `pushCall` overloads. Note that unlike
   /// `pushCall`, this member is invoked on the environment of the callee, not
   /// of the caller.

clang/include/clang/Analysis/FlowSensitive/Value.h

@@ -34,7 +34,6 @@ class Value {
 public:
   enum class Kind {
     Integer,
-    Reference,
     Pointer,
     Struct,
 
@@ -165,23 +164,6 @@ class IntegerValue : public Value {
   }
 };
 
-/// Models a dereferenced pointer. For example, a reference in C++ or an lvalue
-/// in C.
-class ReferenceValue final : public Value {
-public:
-  explicit ReferenceValue(StorageLocation &ReferentLoc)
-      : Value(Kind::Reference), ReferentLoc(ReferentLoc) {}
-
-  static bool classof(const Value *Val) {
-    return Val->getKind() == Kind::Reference;
-  }
-
-  StorageLocation &getReferentLoc() const { return ReferentLoc; }
-
-private:
-  StorageLocation &ReferentLoc;
-};
-
 /// Models a symbolic pointer. Specifically, any value of type `T*`.
 class PointerValue final : public Value {
 public:

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

@@ -68,7 +68,6 @@ static bool compareDistinctValues(QualType Type, Value &Val1,
   case ComparisonResult::Unknown:
     switch (Val1.getKind()) {
     case Value::Kind::Integer:
-    case Value::Kind::Reference:
     case Value::Kind::Pointer:
     case Value::Kind::Struct:
       // FIXME: this choice intentionally introduces unsoundness to allow
@@ -140,11 +139,6 @@ static Value *mergeDistinctValues(QualType Type, Value &Val1,
 
   // FIXME: Consider destroying `MergedValue` immediately if `ValueModel::merge`
   // returns false to avoid storing unneeded values in `DACtx`.
-  // FIXME: Creating the value based on the type alone creates misshapen values
-  // for lvalues, since the type does not reflect the need for `ReferenceValue`.
-  // This issue will be resolved when `ReferenceValue` is eliminated as part
-  // of the ongoing migration to strict handling of value categories (see
-  // https://discourse.llvm.org/t/70086 for details).
   if (MergedVal)
     if (Model.merge(Type, Val1, Env1, Val2, Env2, *MergedVal, MergedEnv))
       return MergedVal;
@@ -611,7 +605,6 @@ StorageLocation &Environment::createStorageLocation(const Expr &E) {
 
 void Environment::setStorageLocation(const ValueDecl &D, StorageLocation &Loc) {
   assert(!DeclToLoc.contains(&D));
-  assert(!isa_and_nonnull<ReferenceValue>(getValue(Loc)));
   DeclToLoc[&D] = &Loc;
 }
 
@@ -622,8 +615,6 @@ StorageLocation *Environment::getStorageLocation(const ValueDecl &D) const {
 
   StorageLocation *Loc = It->second;
 
-  assert(!isa_and_nonnull<ReferenceValue>(getValue(*Loc)));
-
   return Loc;
 }
 
@@ -647,7 +638,7 @@ StorageLocation *Environment::getStorageLocation(const Expr &E,
                                                  SkipPast SP) const {
   // FIXME: Add a test with parens.
   auto It = ExprToLoc.find(&ignoreCFGOmittedNodes(E));
-  return It == ExprToLoc.end() ? nullptr : &skip(*It->second, SP);
+  return It == ExprToLoc.end() ? nullptr : &*It->second;
 }
 
 StorageLocation *Environment::getStorageLocationStrict(const Expr &E) const {
@@ -659,9 +650,6 @@ StorageLocation *Environment::getStorageLocationStrict(const Expr &E) const {
   if (Loc == nullptr)
     return nullptr;
 
-  if (auto *RefVal = dyn_cast_or_null<ReferenceValue>(getValue(*Loc)))
-    return &RefVal->getReferentLoc();
-
   return Loc;
 }
 
@@ -696,7 +684,6 @@ void Environment::setValue(const StorageLocation &Loc, Value &Val) {
 
 void Environment::setValueStrict(const Expr &E, Value &Val) {
   assert(E.isPRValue());
-  assert(!isa<ReferenceValue>(Val));
 
   if (auto *StructVal = dyn_cast<StructValue>(&Val)) {
     if (auto *ExistingVal = cast_or_null<StructValue>(getValueStrict(E)))
@@ -739,8 +726,6 @@ Value *Environment::getValueStrict(const Expr &E) const {
   assert(E.isPRValue());
   Value *Val = getValue(E, SkipPast::None);
 
-  assert(Val == nullptr || !isa<ReferenceValue>(Val));
-
   return Val;
 }
 
@@ -760,6 +745,7 @@ Value *Environment::createValueUnlessSelfReferential(
     QualType Type, llvm::DenseSet<QualType> &Visited, int Depth,
     int &CreatedValuesCount) {
   assert(!Type.isNull());
+  assert(!Type->isReferenceType());
 
   // Allow unlimited fields at depth 1; only cap at deeper nesting levels.
   if ((Depth > 1 && CreatedValuesCount > MaxCompositeValueSize) ||
@@ -779,16 +765,13 @@ Value *Environment::createValueUnlessSelfReferential(
     return &arena().create<IntegerValue>();
   }
 
-  if (Type->isReferenceType() || Type->isPointerType()) {
+  if (Type->isPointerType()) {
     CreatedValuesCount++;
     QualType PointeeType = Type->getPointeeType();
     StorageLocation &PointeeLoc =
         createLocAndMaybeValue(PointeeType, Visited, Depth, CreatedValuesCount);
 
-    if (Type->isReferenceType())
-      return &arena().create<ReferenceValue>(PointeeLoc);
-    else
-      return &arena().create<PointerValue>(PointeeLoc);
+    return &arena().create<PointerValue>(PointeeLoc);
   }
 
   if (Type->isRecordType()) {
@@ -892,25 +875,6 @@ StorageLocation &Environment::createObjectInternal(const VarDecl *D,
   return Loc;
 }
 
-StorageLocation &Environment::skip(StorageLocation &Loc, SkipPast SP) const {
-  switch (SP) {
-  case SkipPast::None:
-    return Loc;
-  case SkipPast::Reference:
-    // References cannot be chained so we only need to skip past one level of
-    // indirection.
-    if (auto *Val = dyn_cast_or_null<ReferenceValue>(getValue(Loc)))
-      return Val->getReferentLoc();
-    return Loc;
-  }
-  llvm_unreachable("bad SkipPast kind");
-}
-
-const StorageLocation &Environment::skip(const StorageLocation &Loc,
-                                         SkipPast SP) const {
-  return skip(*const_cast<StorageLocation *>(&Loc), SP);
-}
-
 void Environment::addToFlowCondition(const Formula &Val) {
   DACtx->addFlowConditionConstraint(FlowConditionToken, Val);
 }

clang/lib/Analysis/FlowSensitive/DebugSupport.cpp

@@ -26,8 +26,6 @@ llvm::StringRef debugString(Value::Kind Kind) {
   switch (Kind) {
   case Value::Kind::Integer:
     return "Integer";
-  case Value::Kind::Reference:
-    return "Reference";
   case Value::Kind::Pointer:
     return "Pointer";
   case Value::Kind::Struct:

clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp

@@ -99,10 +99,6 @@ class ModelDumper {
     case Value::Kind::AtomicBool:
     case Value::Kind::FormulaBool:
       break;
-    case Value::Kind::Reference:
-      JOS.attributeObject(
-          "referent", [&] { dump(cast<ReferenceValue>(V).getReferentLoc()); });
-      break;
     case Value::Kind::Pointer:
       JOS.attributeObject(
           "pointee", [&] { dump(cast<PointerValue>(V).getPointeeLoc()); });

clang/lib/Analysis/FlowSensitive/Value.cpp

@@ -19,10 +19,6 @@ namespace dataflow {
 
 static bool areEquivalentIndirectionValues(const Value &Val1,
                                            const Value &Val2) {
-  if (auto *IndVal1 = dyn_cast<ReferenceValue>(&Val1)) {
-    auto *IndVal2 = cast<ReferenceValue>(&Val2);
-    return &IndVal1->getReferentLoc() == &IndVal2->getReferentLoc();
-  }
   if (auto *IndVal1 = dyn_cast<PointerValue>(&Val1)) {
     auto *IndVal2 = cast<PointerValue>(&Val2);
     return &IndVal1->getPointeeLoc() == &IndVal2->getPointeeLoc();
@@ -38,10 +34,6 @@ bool areEquivalentValues(const Value &Val1, const Value &Val2) {
 
 raw_ostream &operator<<(raw_ostream &OS, const Value &Val) {
   switch (Val.getKind()) {
-  case Value::Kind::Reference: {
-    const auto *RV = cast<ReferenceValue>(&Val);
-    return OS << "Reference(" << &RV->getReferentLoc() << ")";
-  }
   case Value::Kind::Pointer: {
     const auto *PV = dyn_cast<PointerValue>(&Val);
     return OS << "Pointer(" << &PV->getPointeeLoc() << ")";

clang/unittests/Analysis/FlowSensitive/ValueTest.cpp

@@ -29,14 +29,6 @@ TEST(ValueTest, DifferentIntegerValuesNotEquivalent) {
   EXPECT_FALSE(areEquivalentValues(V1, V2));
 }
 
-TEST(ValueTest, AliasedReferencesEquivalent) {
-  auto L = ScalarStorageLocation(QualType());
-  ReferenceValue V1(L);
-  ReferenceValue V2(L);
-  EXPECT_TRUE(areEquivalentValues(V1, V2));
-  EXPECT_TRUE(areEquivalentValues(V2, V1));
-}
-
 TEST(ValueTest, AliasedPointersEquivalent) {
   auto L = ScalarStorageLocation(QualType());
   PointerValue V1(L);
@@ -68,21 +60,12 @@ TEST(ValueTest, EquivalentValuesWithDifferentPropsEquivalent) {
 TEST(ValueTest, DifferentKindsNotEquivalent) {
   Arena A;
   auto L = ScalarStorageLocation(QualType());
-  ReferenceValue V1(L);
+  PointerValue V1(L);
   TopBoolValue V2(A.makeAtomRef(Atom(0)));
   EXPECT_FALSE(areEquivalentValues(V1, V2));
   EXPECT_FALSE(areEquivalentValues(V2, V1));
 }
 
-TEST(ValueTest, NotAliasedReferencesNotEquivalent) {
-  auto L1 = ScalarStorageLocation(QualType());
-  ReferenceValue V1(L1);
-  auto L2 = ScalarStorageLocation(QualType());
-  ReferenceValue V2(L2);
-  EXPECT_FALSE(areEquivalentValues(V1, V2));
-  EXPECT_FALSE(areEquivalentValues(V2, V1));
-}
-
 TEST(ValueTest, NotAliasedPointersNotEquivalent) {
   auto L1 = ScalarStorageLocation(QualType());
   PointerValue V1(L1);