[clang][dataflow] Eliminate intermediate ReferenceValues from Environment::DeclToLoc.

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

Reviewed By: ymandel, xazax.hun, gribozavr2

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

Author: martinboehme

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

@@ -269,13 +269,24 @@ class Environment {
   ///
   /// 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;
+  /// 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.
   ///
@@ -320,7 +331,11 @@ class Environment {
 
   /// 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.

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

@@ -247,9 +247,9 @@ void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) {
   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 +291,16 @@ Environment::Environment(DataflowAnalysisContext &DACtx,
 
     for (const auto *ParamDecl : FuncDecl->parameters()) {
       assert(ParamDecl != nullptr);
-      auto &ParamLoc = createStorageLocation(*ParamDecl);
+      // References aren't objects, so the reference itself doesn't have a
+      // storage location. Instead, the storage location for a reference refers
+      // directly to an object of the referenced type -- so strip off any
+      // reference from the type.
+      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 +382,19 @@ void Environment::pushCallInternal(const FunctionDecl *FuncDecl,
       continue;
 
     const VarDecl *Param = *ParamIt;
-    auto &Loc = createStorageLocation(*Param);
-    setStorageLocation(*Param, Loc);
 
     QualType ParamType = Param->getType();
     if (ParamType->isReferenceType()) {
-      auto &Val = DACtx->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 +526,10 @@ LatticeJoinEffect Environment::join(const Environment &Other,
   JoinedEnv.ReturnLoc = ReturnLoc;
   JoinedEnv.ThisPointeeLoc = ThisPointeeLoc;
 
+  // FIXME: Once we're able to remove declarations from `DeclToLoc` when their
+  // lifetime ends, add an assertion that there aren't any entries in
+  // `DeclToLoc` and `Other.DeclToLoc` that map the same declaration to
+  // different storage locations.
   JoinedEnv.DeclToLoc = intersectDenseMaps(DeclToLoc, Other.DeclToLoc);
   if (DeclToLoc.size() != JoinedEnv.DeclToLoc.size())
     Effect = LatticeJoinEffect::Changed;
@@ -589,13 +601,23 @@ 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;
 }
 
 StorageLocation *Environment::getStorageLocation(const ValueDecl &D,
                                                  SkipPast SP) const {
+  assert(SP != SkipPast::ReferenceThenPointer);
+
   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(!isa_and_nonnull<ReferenceValue>(getValue(*Loc)));
+
+  return Loc;
 }
 
 void Environment::setStorageLocation(const Expr &E, StorageLocation &Loc) {
@@ -662,6 +684,8 @@ Value *Environment::getValue(const StorageLocation &Loc) const {
 }
 
 Value *Environment::getValue(const ValueDecl &D, SkipPast SP) const {
+  assert(SP != SkipPast::ReferenceThenPointer);
+
   auto *Loc = getStorageLocation(D, SP);
   if (Loc == nullptr)
     return nullptr;

clang/lib/Analysis/FlowSensitive/Transfer.cpp

@@ -223,80 +223,77 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     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) {
     // Group decls are converted into single decls in the CFG so the cast below
     // 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 +319,16 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
           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 +537,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
         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;
       }
     }