mboehme created this revision.
Herald added subscribers: martong, xazax.hun, inglorion.
Herald added a reviewer: NoQ.
Herald added a project: All.
mboehme requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

For the wider context of this change, see the RFC at
https://discourse.llvm.org/t/70086.

After this change, global and local variables of reference type are associated
directly with the `StorageLocation` of the referenced object instead of the
`StorageLocation` of a `ReferenceValue`.

Some tests that explicitly check for an existence of `ReferenceValue` for a
variable of reference type have been modified accordingly.

As discussed in the RFC, I have added an assertion to `Environment::join()` to
check that if both environments contain an entry for the same declaration in
`DeclToLoc`, they both map to the same `StorageLocation`. As discussed in
https://discourse.llvm.org/t/70086/5, this also necessitates removing
declarations from `DeclToLoc` when they go out of scope.

In the RFC, I proposed a gradual migration for this change, but it appears
that all of the callers of `Environment::setStorageLocation(const ValueDecl &,
SkipPast` are in the dataflow framework itself, and that there are only a few of
them.

As this is the function whose semantics are changing in a way that callers
potentially need to adapt to, I've decided to change the semantics of the
function directly.

The semantics of `getStorageLocation(const ValueDecl &, SkipPast SP` now no
longer depend on the behavior of the `SP` parameter. (There don't appear to be
any callers that use `SkipPast::ReferenceThenPointer`, so I've added an
assertion that forbids this usage.)

This patch adds a default argument for the `SP` parameter and removes the
explicit `SP` argument at the callsites that are touched by this change. A
followup patch will remove the argument from the remaining callsites,
allowing the `SkipPast` parameter to be removed entirely. (I don't want to do
that in this patch so that semantics-changing changes can be reviewed separately
from semantics-neutral changes.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149144

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
  clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  clang/lib/Analysis/FlowSensitive/Transfer.cpp
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -403,14 +403,9 @@
 
         const StorageLocation *FooLoc =
             Env.getStorageLocation(*FooDecl, SkipPast::None);
-        ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(FooLoc));
-
-        const ReferenceValue *FooVal =
-            cast<ReferenceValue>(Env.getValue(*FooLoc));
-        const StorageLocation &FooReferentLoc = FooVal->getReferentLoc();
-        EXPECT_TRUE(isa<AggregateStorageLocation>(&FooReferentLoc));
+        ASSERT_TRUE(isa_and_nonnull<AggregateStorageLocation>(FooLoc));
 
-        const Value *FooReferentVal = Env.getValue(FooReferentLoc);
+        const Value *FooReferentVal = Env.getValue(*FooLoc);
         EXPECT_TRUE(isa_and_nonnull<StructValue>(FooReferentVal));
       });
 }
@@ -494,11 +489,9 @@
     ASSERT_THAT(BazRefDecl, NotNull());
     ASSERT_THAT(BazPtrDecl, NotNull());
 
-    const auto *FooLoc = cast<ScalarStorageLocation>(
+    const auto *FooLoc = cast<AggregateStorageLocation>(
         Env.getStorageLocation(*FooDecl, SkipPast::None));
-    const auto *FooVal = cast<ReferenceValue>(Env.getValue(*FooLoc));
-    const auto *FooReferentVal =
-        cast<StructValue>(Env.getValue(FooVal->getReferentLoc()));
+    const auto *FooReferentVal = cast<StructValue>(Env.getValue(*FooLoc));
 
     const auto *BarVal =
         cast<ReferenceValue>(FooReferentVal->getChild(*BarDecl));
@@ -507,13 +500,17 @@
 
     const auto *FooRefVal =
         cast<ReferenceValue>(BarReferentVal->getChild(*FooRefDecl));
-    const StorageLocation &FooReferentLoc = FooRefVal->getReferentLoc();
-    EXPECT_THAT(Env.getValue(FooReferentLoc), IsNull());
+    const auto &FooReferentLoc =
+        cast<AggregateStorageLocation>(FooRefVal->getReferentLoc());
+    EXPECT_THAT(Env.getValue(FooReferentLoc), NotNull());
+    EXPECT_THAT(Env.getValue(FooReferentLoc.getChild(*BarDecl)), IsNull());
 
     const auto *FooPtrVal =
         cast<PointerValue>(BarReferentVal->getChild(*FooPtrDecl));
-    const StorageLocation &FooPtrPointeeLoc = FooPtrVal->getPointeeLoc();
-    EXPECT_THAT(Env.getValue(FooPtrPointeeLoc), IsNull());
+    const auto &FooPtrPointeeLoc =
+        cast<AggregateStorageLocation>(FooPtrVal->getPointeeLoc());
+    EXPECT_THAT(Env.getValue(FooPtrPointeeLoc), NotNull());
+    EXPECT_THAT(Env.getValue(FooPtrPointeeLoc.getChild(*BarDecl)), IsNull());
 
     const auto *BazRefVal =
         cast<ReferenceValue>(BarReferentVal->getChild(*BazRefDecl));
@@ -1058,16 +1055,9 @@
 
         const StorageLocation *FooLoc =
             Env.getStorageLocation(*FooDecl, SkipPast::None);
-        ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(FooLoc));
-
-        const ReferenceValue *FooVal =
-            dyn_cast<ReferenceValue>(Env.getValue(*FooLoc));
-        ASSERT_THAT(FooVal, NotNull());
+        ASSERT_TRUE(isa_and_nonnull<AggregateStorageLocation>(FooLoc));
 
-        const StorageLocation &FooReferentLoc = FooVal->getReferentLoc();
-        EXPECT_TRUE(isa<AggregateStorageLocation>(&FooReferentLoc));
-
-        const Value *FooReferentVal = Env.getValue(FooReferentLoc);
+        const Value *FooReferentVal = Env.getValue(*FooLoc);
         EXPECT_TRUE(isa_and_nonnull<StructValue>(FooReferentVal));
       });
 }
@@ -1905,15 +1895,13 @@
         const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
         ASSERT_THAT(FooDecl, NotNull());
 
-        const auto *FooVal =
-            cast<ReferenceValue>(Env.getValue(*FooDecl, SkipPast::None));
+        const auto *FooLoc = Env.getStorageLocation(*FooDecl);
 
         const ValueDecl *QuxDecl = findValueDecl(ASTCtx, "Qux");
         ASSERT_THAT(QuxDecl, NotNull());
 
-        const auto *QuxVal =
-            cast<ReferenceValue>(Env.getValue(*QuxDecl, SkipPast::None));
-        EXPECT_EQ(&QuxVal->getReferentLoc(), &FooVal->getReferentLoc());
+        const auto *QuxLoc = Env.getStorageLocation(*QuxDecl);
+        EXPECT_EQ(QuxLoc, FooLoc);
       });
 }
 
@@ -2593,9 +2581,8 @@
 
         const auto *FooVal =
             cast<PointerValue>(Env.getValue(*FooDecl, SkipPast::None));
-        const auto *BarVal =
-            cast<ReferenceValue>(Env.getValue(*BarDecl, SkipPast::None));
-        EXPECT_EQ(&BarVal->getReferentLoc(), &FooVal->getPointeeLoc());
+        const auto *BarLoc = Env.getStorageLocation(*BarDecl);
+        EXPECT_EQ(BarLoc, &FooVal->getPointeeLoc());
       });
 }
 
@@ -2643,17 +2630,20 @@
     void target(int *Foo) {
       do {
         int Bar = *Foo;
+        // [[in_loop]]
       } while (true);
       (void)0;
-      /*[[p]]*/
+      // [[after_loop]]
     }
   )";
   runDataflow(
       Code,
       [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
          ASTContext &ASTCtx) {
-        ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
-        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+        const Environment &EnvInLoop =
+            getEnvironmentAtAnnotation(Results, "in_loop");
+        const Environment &EnvAfterLoop =
+            getEnvironmentAtAnnotation(Results, "after_loop");
 
         const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
         ASSERT_THAT(FooDecl, NotNull());
@@ -2662,15 +2652,14 @@
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *FooVal =
-            cast<PointerValue>(Env.getValue(*FooDecl, SkipPast::None));
+            cast<PointerValue>(EnvAfterLoop.getValue(*FooDecl));
         const auto *FooPointeeVal =
-            cast<IntegerValue>(Env.getValue(FooVal->getPointeeLoc()));
-
-        const auto *BarVal = dyn_cast_or_null<IntegerValue>(
-            Env.getValue(*BarDecl, SkipPast::None));
-        ASSERT_THAT(BarVal, NotNull());
+            cast<IntegerValue>(EnvAfterLoop.getValue(FooVal->getPointeeLoc()));
 
+        const auto *BarVal = cast<IntegerValue>(EnvInLoop.getValue(*BarDecl));
         EXPECT_EQ(BarVal, FooPointeeVal);
+
+        ASSERT_THAT(EnvAfterLoop.getValue(*BarDecl), IsNull());
       });
 }
 
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -40,9 +40,10 @@
 namespace clang {
 namespace dataflow {
 
+namespace {
+
 /// Returns the index of `Block` in the successors of `Pred`.
-static int blockIndexInPredecessor(const CFGBlock &Pred,
-                                   const CFGBlock &Block) {
+int blockIndexInPredecessor(const CFGBlock &Pred, const CFGBlock &Block) {
   auto BlockPos = llvm::find_if(
       Pred.succs(), [&Block](const CFGBlock::AdjacentBlock &Succ) {
         return Succ && Succ->getBlockID() == Block.getBlockID();
@@ -50,7 +51,7 @@
   return BlockPos - Pred.succ_begin();
 }
 
-static bool isLoopHead(const CFGBlock &B) {
+bool isLoopHead(const CFGBlock &B) {
   if (const auto *T = B.getTerminatorStmt())
     switch (T->getStmtClass()) {
       case Stmt::WhileStmtClass:
@@ -193,8 +194,8 @@
 ///   All predecessors of `Block` except those with loop back edges must have
 ///   already been transferred. States in `AC.BlockStates` that are set to
 ///   `std::nullopt` represent basic blocks that are not evaluated yet.
-static TypeErasedDataflowAnalysisState
-computeBlockInputState(const CFGBlock &Block, AnalysisContext &AC) {
+TypeErasedDataflowAnalysisState computeBlockInputState(const CFGBlock &Block,
+                                                       AnalysisContext &AC) {
   llvm::DenseSet<const CFGBlock *> Preds;
   Preds.insert(Block.pred_begin(), Block.pred_end());
   if (Block.getTerminator().isTemporaryDtorsBranch()) {
@@ -319,6 +320,13 @@
   }
 }
 
+void builtinTransferScopeEnd(const CFGScopeEnd &Elt,
+                             TypeErasedDataflowAnalysisState &InputState) {
+  if (const VarDecl *VD = Elt.getVarDecl()) {
+    InputState.Env.unsetStorageLocation(*VD);
+  }
+}
+
 void builtinTransfer(const CFGElement &Elt,
                      TypeErasedDataflowAnalysisState &State,
                      AnalysisContext &AC) {
@@ -329,6 +337,9 @@
   case CFGElement::Initializer:
     builtinTransferInitializer(Elt.castAs<CFGInitializer>(), State);
     break;
+  case CFGElement::ScopeEnd:
+    builtinTransferScopeEnd(Elt.castAs<CFGScopeEnd>(), State);
+    break;
   default:
     // FIXME: Evaluate other kinds of `CFGElement`.
     break;
@@ -370,6 +381,8 @@
   return State;
 }
 
+} // namespace
+
 TypeErasedDataflowAnalysisState transferBlock(
     const ControlFlowContext &CFCtx,
     llvm::ArrayRef<std::optional<TypeErasedDataflowAnalysisState>> BlockStates,
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -223,24 +223,7 @@
     if (DeclLoc == nullptr)
       return;
 
-    // If the value is already an lvalue, don't double-wrap it.
-    if (isa_and_nonnull<ReferenceValue>(Env.getValue(*DeclLoc))) {
-      // We only expect to encounter a `ReferenceValue` for a reference type
-      // (always) or for `BindingDecl` (sometimes). For the latter, we can't
-      // rely on type, because their type does not indicate whether they are a
-      // reference type. The assert is not strictly necessary, since we don't
-      // depend on its truth to proceed. But, it verifies our assumptions,
-      // which, if violated, probably indicate a problem elsewhere.
-      assert((VD->getType()->isReferenceType() || isa<BindingDecl>(VD)) &&
-             "Only reference-typed declarations or `BindingDecl`s should map "
-             "to `ReferenceValue`s");
-      Env.setStorageLocation(*S, *DeclLoc);
-    } else {
-      auto &Loc = Env.createStorageLocation(*S);
-      auto &Val = Env.create<ReferenceValue>(*DeclLoc);
-      Env.setStorageLocation(*S, Loc);
-      Env.setValue(Loc, Val);
-    }
+    Env.setStorageLocation(*S, *DeclLoc);
   }
 
   void VisitDeclStmt(const DeclStmt *S) {
@@ -248,55 +231,70 @@
     // is safe.
     const auto &D = *cast<VarDecl>(S->getSingleDecl());
 
+    ProcessVarDecl(D);
+  }
+
+  void ProcessVarDecl(const VarDecl &D) {
     // Static local vars are already initialized in `Environment`.
     if (D.hasGlobalStorage())
       return;
 
-    // The storage location for `D` could have been created earlier, before the
-    // variable's declaration statement (for example, in the case of
-    // BindingDecls).
-    auto *MaybeLoc = Env.getStorageLocation(D, SkipPast::None);
-    if (MaybeLoc == nullptr) {
-      MaybeLoc = &Env.createStorageLocation(D);
-      Env.setStorageLocation(D, *MaybeLoc);
-    }
-    auto &Loc = *MaybeLoc;
+    if (D.getType()->isReferenceType()) {
+      // If this is the holding variable for a `BindingDecl`, we may already
+      // have a storage location set up -- so check. (See also explanation below
+      // where we process the `BindingDecl`.)
+      if (Env.getStorageLocation(D) == nullptr) {
+        const Expr *InitExpr = D.getInit();
+        assert(InitExpr != nullptr);
+        if (auto *InitExprLoc =
+                Env.getStorageLocation(*InitExpr, SkipPast::Reference)) {
+          Env.setStorageLocation(D, *InitExprLoc);
+        } else {
+          // Even though we have an initializer, we might not get an
+          // InitExprLoc, for example if the InitExpr is a CallExpr for which we
+          // don't have a function body. In this case, we just invent a storage
+          // location and value -- it's the best we can do.
+          StorageLocation &Loc =
+              Env.createStorageLocation(D.getType().getNonReferenceType());
+          Env.setStorageLocation(D, Loc);
+          if (Value *Val = Env.createValue(D.getType().getNonReferenceType()))
+            Env.setValue(Loc, *Val);
+        }
+      }
+    } else {
+      // Not a reference type.
 
-    const Expr *InitExpr = D.getInit();
-    if (InitExpr == nullptr) {
-      // No initializer expression - associate `Loc` with a new value.
-      if (Value *Val = Env.createValue(D.getType()))
-        Env.setValue(Loc, *Val);
-      return;
-    }
+      assert(Env.getStorageLocation(D) == nullptr);
+      StorageLocation &Loc = Env.createStorageLocation(D);
+      Env.setStorageLocation(D, Loc);
 
-    if (D.getType()->isReferenceType()) {
-      // Initializing a reference variable - do not create a reference to
-      // reference.
-      // FIXME: reuse the ReferenceValue instead of creating a new one.
-      if (auto *InitExprLoc =
-              Env.getStorageLocation(*InitExpr, SkipPast::Reference)) {
-        auto &Val = Env.create<ReferenceValue>(*InitExprLoc);
-        Env.setValue(Loc, Val);
+      const Expr *InitExpr = D.getInit();
+      if (InitExpr == nullptr) {
+        // No initializer expression - associate `Loc` with a new value.
+        if (Value *Val = Env.createValue(D.getType()))
+          Env.setValue(Loc, *Val);
+        return;
       }
-    } else if (auto *InitExprVal = Env.getValue(*InitExpr, SkipPast::None)) {
-      Env.setValue(Loc, *InitExprVal);
-    }
 
-    if (Env.getValue(Loc) == nullptr) {
-      // We arrive here in (the few) cases where an expression is intentionally
-      // "uninterpreted". There are two ways to handle this situation: propagate
-      // the status, so that uninterpreted initializers result in uninterpreted
-      // variables, or provide a default value. We choose the latter so that
-      // later refinements of the variable can be used for reasoning about the
-      // surrounding code.
-      //
-      // FIXME. If and when we interpret all language cases, change this to
-      // assert that `InitExpr` is interpreted, rather than supplying a default
-      // value (assuming we don't update the environment API to return
-      // references).
-      if (Value *Val = Env.createValue(D.getType()))
-        Env.setValue(Loc, *Val);
+      if (auto *InitExprVal = Env.getValue(*InitExpr, SkipPast::None)) {
+        Env.setValue(Loc, *InitExprVal);
+      }
+
+      if (Env.getValue(Loc) == nullptr) {
+        // We arrive here in (the few) cases where an expression is
+        // intentionally "uninterpreted". There are two ways to handle this
+        // situation: propagate the status, so that uninterpreted initializers
+        // result in uninterpreted variables, or provide a default value. We
+        // choose the latter so that later refinements of the variable can be
+        // used for reasoning about the surrounding code.
+        //
+        // FIXME. If and when we interpret all language cases, change this to
+        // assert that `InitExpr` is interpreted, rather than supplying a
+        // default value (assuming we don't update the environment API to return
+        // references).
+        if (Value *Val = Env.createValue(D.getType()))
+          Env.setValue(Loc, *Val);
+      }
     }
 
     // `DecompositionDecl` must be handled after we've interpreted the loc
@@ -322,15 +320,16 @@
           if (auto *Loc = Env.getStorageLocation(*ME, SkipPast::Reference))
             Env.setStorageLocation(*B, *Loc);
         } else if (auto *VD = B->getHoldingVar()) {
-          // Holding vars are used to back the BindingDecls of tuple-like
-          // types. The holding var declarations appear *after* this statement,
-          // so we have to create a location for them here to share with `B`. We
-          // don't visit the binding, because we know it will be a DeclRefExpr
-          // to `VD`. Note that, by construction of the AST, `VD` will always be
-          // a reference -- either lvalue or rvalue.
-          auto &VDLoc = Env.createStorageLocation(*VD);
-          Env.setStorageLocation(*VD, VDLoc);
-          Env.setStorageLocation(*B, VDLoc);
+          // Holding vars are used to back the `BindingDecl`s of tuple-like
+          // types. The holding var declarations appear after the
+          // `DecompositionDecl`, so we have to explicitly process them here
+          // to know their storage location. They will be processed a second
+          // time when we visit their `VarDecl`s, so we have code that protects
+          // against this above.
+          ProcessVarDecl(*VD);
+          auto *VDLoc = Env.getStorageLocation(*VD);
+          assert(VDLoc != nullptr);
+          Env.setStorageLocation(*B, *VDLoc);
         }
       }
     }
@@ -539,15 +538,7 @@
         if (VarDeclLoc == nullptr)
           return;
 
-        if (VarDeclLoc->getType()->isReferenceType()) {
-          assert(isa_and_nonnull<ReferenceValue>(Env.getValue((*VarDeclLoc))) &&
-                 "reference-typed declarations map to `ReferenceValue`s");
-          Env.setStorageLocation(*S, *VarDeclLoc);
-        } else {
-          auto &Loc = Env.createStorageLocation(*S);
-          Env.setStorageLocation(*S, Loc);
-          Env.setValue(Loc, Env.create<ReferenceValue>(*VarDeclLoc));
-        }
+        Env.setStorageLocation(*S, *VarDeclLoc);
         return;
       }
     }
Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -49,6 +49,19 @@
   return Result;
 }
 
+/// Returns whether there is a key that is present in both maps and maps to
+/// different values.
+template <typename K, typename V>
+bool conflictingValues(const llvm::DenseMap<K, V> &Map1,
+                       const llvm::DenseMap<K, V> &Map2) {
+  for (auto &Entry : Map1) {
+    auto It = Map2.find(Entry.first);
+    if (It != Map2.end() && Entry.second != It->second)
+      return true;
+  }
+  return false;
+}
+
 static bool compareDistinctValues(QualType Type, Value &Val1,
                                   const Environment &Env1, Value &Val2,
                                   const Environment &Env2,
@@ -247,9 +260,9 @@
   for (const VarDecl *D : Vars) {
     if (getStorageLocation(*D, SkipPast::None) != nullptr)
       continue;
-    auto &Loc = createStorageLocation(*D);
+    auto &Loc = createStorageLocation(D->getType().getNonReferenceType());
     setStorageLocation(*D, Loc);
-    if (auto *Val = createValue(D->getType()))
+    if (auto *Val = createValue(D->getType().getNonReferenceType()))
       setValue(Loc, *Val);
   }
 
@@ -291,10 +304,12 @@
 
     for (const auto *ParamDecl : FuncDecl->parameters()) {
       assert(ParamDecl != nullptr);
-      auto &ParamLoc = createStorageLocation(*ParamDecl);
+      auto &ParamLoc =
+          createStorageLocation(ParamDecl->getType().getNonReferenceType());
       setStorageLocation(*ParamDecl, ParamLoc);
-      if (Value *ParamVal = createValue(ParamDecl->getType()))
-        setValue(ParamLoc, *ParamVal);
+      if (Value *ParamVal =
+              createValue(ParamDecl->getType().getNonReferenceType()))
+          setValue(ParamLoc, *ParamVal);
     }
 
     QualType ReturnType = FuncDecl->getReturnType();
@@ -376,17 +391,19 @@
       continue;
 
     const VarDecl *Param = *ParamIt;
-    auto &Loc = createStorageLocation(*Param);
-    setStorageLocation(*Param, Loc);
 
     QualType ParamType = Param->getType();
     if (ParamType->isReferenceType()) {
-      auto &Val = arena().create<ReferenceValue>(*ArgLoc);
-      setValue(Loc, Val);
-    } else if (auto *ArgVal = getValue(*ArgLoc)) {
-      setValue(Loc, *ArgVal);
-    } else if (Value *Val = createValue(ParamType)) {
-      setValue(Loc, *Val);
+      setStorageLocation(*Param, *ArgLoc);
+    } else {
+      auto &Loc = createStorageLocation(*Param);
+      setStorageLocation(*Param, Loc);
+
+      if (auto *ArgVal = getValue(*ArgLoc)) {
+        setValue(Loc, *ArgVal);
+      } else if (Value *Val = createValue(ParamType)) {
+        setValue(Loc, *Val);
+      }
     }
   }
 }
@@ -518,6 +535,7 @@
   JoinedEnv.ReturnLoc = ReturnLoc;
   JoinedEnv.ThisPointeeLoc = ThisPointeeLoc;
 
+  assert(!conflictingValues(DeclToLoc, Other.DeclToLoc));
   JoinedEnv.DeclToLoc = intersectDenseMaps(DeclToLoc, Other.DeclToLoc);
   if (DeclToLoc.size() != JoinedEnv.DeclToLoc.size())
     Effect = LatticeJoinEffect::Changed;
@@ -589,13 +607,28 @@
 
 void Environment::setStorageLocation(const ValueDecl &D, StorageLocation &Loc) {
   assert(!DeclToLoc.contains(&D));
+  assert(dyn_cast_or_null<ReferenceValue>(getValue(Loc)) == nullptr);
   DeclToLoc[&D] = &Loc;
 }
 
+void Environment::unsetStorageLocation(const ValueDecl &D) {
+  bool erased = DeclToLoc.erase(&D);
+  if (!erased)
+    D.dump();
+  assert(erased);
+}
+
 StorageLocation *Environment::getStorageLocation(const ValueDecl &D,
                                                  SkipPast SP) const {
   auto It = DeclToLoc.find(&D);
-  return It == DeclToLoc.end() ? nullptr : &skip(*It->second, SP);
+  if (It == DeclToLoc.end())
+    return nullptr;
+
+  StorageLocation *Loc = It->second;
+
+  assert(dyn_cast_or_null<ReferenceValue>(getValue(*Loc)) == nullptr);
+
+  return Loc;
 }
 
 void Environment::setStorageLocation(const Expr &E, StorageLocation &Loc) {
Index: clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
+++ clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
@@ -75,6 +75,7 @@
   Options.AddTemporaryDtors = true;
   Options.AddInitializers = true;
   Options.AddCXXDefaultInitExprInCtors = true;
+  Options.AddScopes = true;
 
   // Ensure that all sub-expressions in basic blocks are evaluated.
   Options.setAllAlwaysAdd();
Index: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
===================================================================
--- clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -264,13 +264,31 @@
   ///
   /// Requirements:
   ///
-  ///  `D` must not be assigned a storage location in the environment.
+  ///  `D` must not already have a storage location in the environment.
+  ///
+  ///  If `D` has reference type, `Loc` must refer directly to the referenced
+  ///  object (if any), not to a `ReferenceValue`, and it is not permitted to
+  ///  later change `Loc` to refer to a `ReferenceValue.`
   void setStorageLocation(const ValueDecl &D, StorageLocation &Loc);
 
-  /// Returns the storage location assigned to `D` in the environment, applying
-  /// the `SP` policy for skipping past indirections, or null if `D` isn't
-  /// assigned a storage location in the environment.
-  StorageLocation *getStorageLocation(const ValueDecl &D, SkipPast SP) const;
+  /// Removes the storage location assigned to `D`.
+  ///
+  /// Requirements:
+  ///
+  ///  `D` must have a storage location in the environment.
+  void unsetStorageLocation(const ValueDecl &D);
+
+  /// Returns the storage location assigned to `D` in the environment, or null
+  /// if `D` isn't assigned a storage location in the environment.
+  ///
+  /// Note that if `D` has reference type, the storage location that is returned
+  /// refers directly to the referenced object, not a `ReferenceValue`.
+  ///
+  /// The `SP` parameter is deprecated and has no effect. In addition, it is
+  /// not permitted to pass `SkipPast::ReferenceThenPointer` for this parameter.
+  /// New uses of this function should use the default argument for `SP`.
+  StorageLocation *getStorageLocation(const ValueDecl &D,
+                                      SkipPast SP = SkipPast::None) const;
 
   /// Assigns `Loc` as the storage location of `E` in the environment.
   ///
@@ -315,7 +333,11 @@
 
   /// Equivalent to `getValue(getStorageLocation(D, SP), SkipPast::None)` if `D`
   /// is assigned a storage location in the environment, otherwise returns null.
-  Value *getValue(const ValueDecl &D, SkipPast SP) const;
+  ///
+  /// The `SP` parameter is deprecated and has no effect. In addition, it is
+  /// not permitted to pass `SkipPast::ReferenceThenPointer` for this parameter.
+  /// New uses of this function should use the default argument for `SP`.
+  Value *getValue(const ValueDecl &D, SkipPast SP = SkipPast::None) const;
 
   /// Equivalent to `getValue(getStorageLocation(E, SP), SkipPast::None)` if `E`
   /// is assigned a storage location in the environment, otherwise returns null.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to