[clang][dataflow] Use `Strict` accessors where we weren't using them yet.

[mboehme]

Summary

This eliminates all uses of the deprecated accessors.

[ymandel]

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

542

loc == nullptr

545

Should this line instead be outside of this if? So that the flow is

Loc = get
if (!Loc) Loc = create
set(Loc)

clang/lib/Analysis/FlowSensitive/Transfer.cpp

473–476

What happens otherwise? I gather that S just isn't associated with anything?

[mboehme]

Use Loc == nullptr instead of !Loc.

[mboehme]

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

545

I don't understand -- wouldn't this just be doing unnecessary work?

If

Loc = cast_or_null<AggregateStorageLocation>(
        State.Env.getStorageLocationStrict(*E));

returns a non-null Loc, we don't create a new Loc, hence

State.Env.setStorageLocationStrict(*E, *Loc);

is a no-op, as E is already associated with Loc.

clang/lib/Analysis/FlowSensitive/Transfer.cpp

473–476

Correct. The idea being that if S is elidable, i.e. just a pass-through, we want to pass through any StructValue that we got, and so if we didn't get a StructValue, then we shouldn't pass one through.

I guess we could also just create a StructValue from scratch and associate it with ArgLoc, but I'm not sure that it makes much of a difference in practice.

[ymandel]

SGTM

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

545

Right, I was not paying enough attention. I was thinking specifically about the value. basically, i confused line 545 for line 549.

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

 }
 
 Environment Environment::pushCall(const CallExpr *Call) const {
   Environment Env(*this);
 
   if (const auto *MethodCall = dyn_cast<CXXMemberCallExpr>(Call)) {
     if (const Expr *Arg = MethodCall->getImplicitObjectArgument()) {
       if (!isa<CXXThisExpr>(Arg))
-        Env.ThisPointeeLoc = cast<AggregateStorageLocation>(
-            getStorageLocation(*Arg, SkipPast::Reference));
+          Env.ThisPointeeLoc =
+              cast<AggregateStorageLocation>(getStorageLocationStrict(*Arg));
       // Otherwise (when the argument is `this`), retain the current
       // environment's `ThisPointeeLoc`.
     }
   }
 
   Env.pushCallInternal(Call->getDirectCallee(),
                        llvm::ArrayRef(Call->getArgs(), Call->getNumArgs()));
 

 StorageLocation &Environment::createObjectInternal(const VarDecl *D,
                                                    QualType Ty,
                                                    const Expr *InitExpr) {
   if (Ty->isReferenceType()) {
     // Although variables of reference type always need to be initialized, it
     // can happen that we can't see the initializer, so `InitExpr` may still
     // be null.
     if (InitExpr) {
-      if (auto *InitExprLoc =
-              getStorageLocation(*InitExpr, SkipPast::Reference))
+      if (auto *InitExprLoc = getStorageLocationStrict(*InitExpr))
           return *InitExprLoc;
     }
 
     // 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.
     return createObjectInternal(D, Ty.getNonReferenceType(), nullptr);
   }

 }
 
 AggregateStorageLocation *
 getImplicitObjectLocation(const CXXMemberCallExpr &MCE,
                           const Environment &Env) {
   Expr *ImplicitObject = MCE.getImplicitObjectArgument();
   if (ImplicitObject == nullptr)
     return nullptr;
-  StorageLocation *Loc =
-      Env.getStorageLocation(*ImplicitObject, SkipPast::Reference);
-  if (Loc == nullptr)
-    return nullptr;
   if (ImplicitObject->getType()->isPointerType()) {
-    if (auto *Val = cast_or_null<PointerValue>(Env.getValue(*Loc)))
+    if (auto *Val = cast_or_null<PointerValue>(Env.getValue(*ImplicitObject)))
       return &cast<AggregateStorageLocation>(Val->getPointeeLoc());
     return nullptr;
   }
-  return cast<AggregateStorageLocation>(Loc);
+  return cast_or_null<AggregateStorageLocation>(
+      Env.getStorageLocationStrict(*ImplicitObject));
 }
 
 AggregateStorageLocation *getBaseObjectLocation(const MemberExpr &ME,
                                                 const Environment &Env) {
   Expr *Base = ME.getBase();
   if (Base == nullptr)
     return nullptr;
-  StorageLocation *Loc = Env.getStorageLocation(*Base, SkipPast::Reference);
-  if (Loc == nullptr)
-    return nullptr;
   if (ME.isArrow()) {
-    if (auto *Val = cast_or_null<PointerValue>(Env.getValue(*Loc)))
+    if (auto *Val = cast_or_null<PointerValue>(Env.getValue(*Base)))
       return &cast<AggregateStorageLocation>(Val->getPointeeLoc());
     return nullptr;
   }
-  return cast<AggregateStorageLocation>(Loc);
+  return cast_or_null<AggregateStorageLocation>(
+      Env.getStorageLocationStrict(*Base));
 }
 
 std::vector<FieldDecl *> getFieldsForInitListExpr(const RecordDecl *RD) {
   // Unnamed bitfields are only used for padding and do not appear in
   // `InitListExpr`'s inits. However, those fields do appear in `RecordDecl`'s
   // field list, and we thus need to remove them before mapping inits to
   // fields to avoid mapping inits to the wrongs fields.
   std::vector<FieldDecl *> Fields;

clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp

JOS->attributeObject(elementIterID(Block, Iter, ElementIndex), [&] {
       JOS->attribute("block", blockID(Block));
       JOS->attribute("iter", Iter);
       JOS->attribute("element", ElementIndex);
 
       // If this state immediately follows an Expr, show its built-in model.
       if (ElementIndex > 0) {
         auto S =
             Iters.back().first->Elements[ElementIndex - 1].getAs<CFGStmt>();
-        if (const Expr *E = S ? llvm::dyn_cast<Expr>(S->getStmt()) : nullptr)
-          if (auto *Loc = State.Env.getStorageLocation(*E, SkipPast::None))
+        if (const Expr *E = S ? llvm::dyn_cast<Expr>(S->getStmt()) : nullptr) {
+          if (E->isPRValue()) {
+            if (auto *V = State.Env.getValue(*E))
+              JOS->attributeObject(
+                  "value", [&] { ModelDumper(*JOS, State.Env).dump(*V); });
+          } else {
+            if (auto *Loc = State.Env.getStorageLocationStrict(*E))
               JOS->attributeObject(
                   "value", [&] { ModelDumper(*JOS, State.Env).dump(*Loc); });
           }
+        }
+      }
       if (!ContextLogs.empty()) {
         JOS->attribute("logs", ContextLogs);
         ContextLogs.clear();
       }
       {
         std::string BuiltinLattice;
         llvm::raw_string_ostream BuiltinLatticeS(BuiltinLattice);
         State.Env.dump(BuiltinLatticeS);

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

 }
 
 /// Ensures that `Expr` is mapped to a `BoolValue` and returns its formula.
 const Formula &forceBoolValue(Environment &Env, const Expr &Expr) {
   auto *Value = cast_or_null<BoolValue>(Env.getValue(Expr));
   if (Value != nullptr)
     return Value->formula();
 
-  auto &Loc = Env.createStorageLocation(Expr);
   Value = &Env.makeAtomicBoolValue();
-  Env.setValue(Loc, *Value);
-  Env.setStorageLocation(Expr, Loc);
+  Env.setValueStrict(Expr, *Value);
   return Value->formula();
 }
 
 /// Sets `HasValueVal` as the symbolic value that represents the "has_value"
 /// property of the optional value `OptionalVal`.
 void setHasValue(Value &OptionalVal, BoolValue &HasValueVal) {
   OptionalVal.setProperty("has_value", HasValueVal);
 }
if (auto *PointerVal = dyn_cast_or_null<PointerValue>(Val))
     return Env.getValue(PointerVal->getPointeeLoc());
   return Val;
 }
 
 void transferUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
                         LatticeTransferState &State) {
   if (auto *OptionalVal =
           getValueBehindPossiblePointer(*ObjectExpr, State.Env)) {
-    if (State.Env.getStorageLocation(*UnwrapExpr, SkipPast::None) == nullptr)
+    if (State.Env.getStorageLocationStrict(*UnwrapExpr) == nullptr)
       if (auto *Loc = maybeInitializeOptionalValueMember(
               UnwrapExpr->getType(), *OptionalVal, State.Env))
-        State.Env.setStorageLocation(*UnwrapExpr, *Loc);
+        State.Env.setStorageLocationStrict(*UnwrapExpr, *Loc);
   }
 }
 
 void transferArrowOpCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
                          LatticeTransferState &State) {
   if (auto *OptionalVal =
           getValueBehindPossiblePointer(*ObjectExpr, State.Env)) {
     if (auto *Loc = maybeInitializeOptionalValueMember(

 }
 
 void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr,
                                   const MatchFinder::MatchResult &,
                                   LatticeTransferState &State) {
   if (auto *HasValueVal = getHasValue(
           State.Env, getValueBehindPossiblePointer(
                          *CallExpr->getImplicitObjectArgument(), State.Env))) {
-    auto &CallExprLoc = State.Env.createStorageLocation(*CallExpr);
-    State.Env.setValue(CallExprLoc, *HasValueVal);
-    State.Env.setStorageLocation(*CallExpr, CallExprLoc);
+    State.Env.setValueStrict(*CallExpr, *HasValueVal);
   }
 }
 
 /// `ModelPred` builds a logical formula relating the predicate in
 /// `ValueOrPredExpr` to the optional's `has_value` property.
 void transferValueOrImpl(
     const clang::Expr *ValueOrPredExpr, const MatchFinder::MatchResult &Result,
     LatticeTransferState &State,
transferValueOrImpl(ComparisonExpr, Result, State,
                         // details about the contents of `opt`.
                         return A.makeImplies(ExprVal, HasValueVal);
                       });
 }
 
 void transferCallReturningOptional(const CallExpr *E,
                                    const MatchFinder::MatchResult &Result,
                                    LatticeTransferState &State) {
-  if (State.Env.getStorageLocation(*E, SkipPast::None) != nullptr)
+  if (State.Env.getValue(*E) != nullptr)
     return;
 
   AggregateStorageLocation *Loc = nullptr;
   if (E->isPRValue()) {
     Loc = &State.Env.getResultObjectLocation(*E);
   } else {
-    Loc = &cast<AggregateStorageLocation>(State.Env.createStorageLocation(*E));
+    Loc = cast_or_null<AggregateStorageLocation>(
+        State.Env.getStorageLocationStrict(*E));
+    if (Loc == nullptr) {
+      Loc =
+          &cast<AggregateStorageLocation>(State.Env.createStorageLocation(*E));
       State.Env.setStorageLocationStrict(*E, *Loc);
     }
+  }
 
   createOptionalValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
 }
 
 void constructOptionalValue(const Expr &E, Environment &Env,
                             BoolValue &HasValueVal) {
   AggregateStorageLocation &Loc = Env.getResultObjectLocation(E);
   Env.setValueStrict(E, createOptionalValue(Loc, HasValueVal, Env));

clang/lib/Analysis/FlowSensitive/Transfer.cpp

case BO_Assign: {
       auto *RHSVal = Env.getValue(*RHS);
       if (RHSVal == nullptr)
         break;
 
       // Assign a value to the storage location of the left-hand side.
       Env.setValue(*LHSLoc, *RHSVal);
 
       // Assign a storage location for the whole expression.
-      Env.setStorageLocation(*S, *LHSLoc);
+      Env.setStorageLocationStrict(*S, *LHSLoc);
       break;
     }
     case BO_LAnd:
     case BO_LOr: {
-      auto &Loc = Env.createStorageLocation(*S);
-      Env.setStorageLocation(*S, Loc);
-
       BoolValue &LHSVal = getLogicOperatorSubExprValue(*LHS);
       BoolValue &RHSVal = getLogicOperatorSubExprValue(*RHS);
 
       if (S->getOpcode() == BO_LAnd)
-        Env.setValue(Loc, Env.makeAnd(LHSVal, RHSVal));
+        Env.setValueStrict(*S, Env.makeAnd(LHSVal, RHSVal));
       else
-        Env.setValue(Loc, Env.makeOr(LHSVal, RHSVal));
+        Env.setValueStrict(*S, Env.makeOr(LHSVal, RHSVal));
       break;
     }
     case BO_NE:
     case BO_EQ: {
       auto &LHSEqRHSValue = evaluateBooleanEquality(*LHS, *RHS, Env);
-      auto &Loc = Env.createStorageLocation(*S);
-      Env.setStorageLocation(*S, Loc);
-      Env.setValue(Loc, S->getOpcode() == BO_EQ ? LHSEqRHSValue
-                                                : Env.makeNot(LHSEqRHSValue));
+      Env.setValueStrict(*S, S->getOpcode() == BO_EQ
+                                 ? LHSEqRHSValue
       break;
     }
     case BO_Comma: {
       propagateValueOrStorageLocation(*RHS, *S, Env);
       break;
     }
     default:
       break;
if (const auto *Decomp = dyn_cast<DecompositionDecl>(&D)) {
           if (DE == nullptr)
             continue;
 
           // ME and its base haven't been visited because they aren't included
           // in the statements of the CFG basic block.
           VisitDeclRefExpr(DE);
           VisitMemberExpr(ME);
 
-          if (auto *Loc = Env.getStorageLocation(*ME, SkipPast::Reference))
+          if (auto *Loc = Env.getStorageLocationStrict(*ME))
             Env.setStorageLocation(*B, *Loc);
         } else if (auto *VD = B->getHoldingVar()) {
           // 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.
void VisitImplicitCastExpr(const ImplicitCastExpr *S) {
     case CK_LValueToRValue: {
       // When an L-value is used as an R-value, it may result in sharing, so we
       // need to unpack any nested `Top`s. We also need to strip off the
       // `ReferenceValue` associated with the lvalue.
       auto *SubExprVal = maybeUnpackLValueExpr(*SubExpr, Env);
       if (SubExprVal == nullptr)
         break;
 
-      auto &ExprLoc = Env.createStorageLocation(*S);
-      Env.setStorageLocation(*S, ExprLoc);
-      Env.setValue(ExprLoc, *SubExprVal);
+      Env.setValueStrict(*S, *SubExprVal);
       break;
     }
 
     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_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
       // of assigning them storage locations.
       propagateValueOrStorageLocation(*SubExpr, *S, Env);
       break;
     }
     case CK_NullToPointer: {
-      auto &Loc = Env.createStorageLocation(S->getType());
-      Env.setStorageLocation(*S, Loc);
-
       auto &NullPointerVal =
           Env.getOrCreateNullPointerValue(S->getType()->getPointeeType());
-      Env.setValue(Loc, NullPointerVal);
+      Env.setValueStrict(*S, NullPointerVal);
       break;
     }
     case CK_NullToMemberPointer:
       // FIXME: Implement pointers to members. For now, don't associate a value
       // with this expression.
       break;
     case CK_FunctionToPointerDecay: {
-      StorageLocation *PointeeLoc =
-          Env.getStorageLocation(*SubExpr, SkipPast::Reference);
+      StorageLocation *PointeeLoc = Env.getStorageLocationStrict(*SubExpr);
       if (PointeeLoc == nullptr)
         break;
 
-      auto &PointerLoc = Env.createStorageLocation(*S);
-      auto &PointerVal = Env.create<PointerValue>(*PointeeLoc);
-      Env.setStorageLocation(*S, PointerLoc);
-      Env.setValue(PointerLoc, PointerVal);
+      Env.setValueStrict(*S, Env.create<PointerValue>(*PointeeLoc));
       break;
     }
     case CK_BuiltinFnToFnPtr:
       // Despite its name, the result type of `BuiltinFnToFnPtr` is a function,
       // not a function pointer. In addition, builtin functions can only be
       // called directly; it is not legal to take their address. We therefore
       // don't need to create a value or storage location for them.
       break;
case UO_AddrOf: {
         Env.setValueStrict(*S, Env.create<PointerValue>(*PointeeLoc));
       break;
     }
     case UO_LNot: {
       auto *SubExprVal = dyn_cast_or_null<BoolValue>(Env.getValue(*SubExpr));
       if (SubExprVal == nullptr)
         break;
 
-      auto &ExprLoc = Env.createStorageLocation(*S);
-      Env.setStorageLocation(*S, ExprLoc);
-      Env.setValue(ExprLoc, Env.makeNot(*SubExprVal));
+      Env.setValueStrict(*S, Env.makeNot(*SubExprVal));
       break;
     }
     default:
       break;
     }
   }
 
   void VisitCXXThisExpr(const CXXThisExpr *S) {
     auto *ThisPointeeLoc = Env.getThisPointeeStorageLocation();
     if (ThisPointeeLoc == nullptr)
       // Unions are not supported yet, and will not have a location for the
       // `this` expression's pointee.
       return;
 
-    auto &Loc = Env.createStorageLocation(*S);
-    Env.setStorageLocation(*S, Loc);
-    Env.setValue(Loc, Env.create<PointerValue>(*ThisPointeeLoc));
+    Env.setValueStrict(*S, Env.create<PointerValue>(*ThisPointeeLoc));
   }
 
   void VisitCXXNewExpr(const CXXNewExpr *S) {
-    auto &Loc = Env.createStorageLocation(*S);
-    Env.setStorageLocation(*S, Loc);
     if (Value *Val = Env.createValue(S->getType()))
-      Env.setValue(Loc, *Val);
+      Env.setValueStrict(*S, *Val);
   }
 
   void VisitCXXDeleteExpr(const CXXDeleteExpr *S) {
     // Empty method.
     // We consciously don't do anything on deletes.  Diagnosing double deletes
     // (for example) should be done by a specific analysis, not by the
     // framework.
   }
if (isa<EnumConstantDecl>(Member))
       return;
 
     if (auto *D = dyn_cast<VarDecl>(Member)) {
       if (D->hasGlobalStorage()) {
         auto *VarDeclLoc = Env.getStorageLocation(*D);
         if (VarDeclLoc == nullptr)
           return;
 
-        Env.setStorageLocation(*S, *VarDeclLoc);
+        Env.setStorageLocationStrict(*S, *VarDeclLoc);
         return;
       }
     }
 
     AggregateStorageLocation *BaseLoc = getBaseObjectLocation(*S, Env);
     if (BaseLoc == nullptr)
       return;
 
if (ConstructorDecl->isCopyOrMoveConstructor()) {
       // It is permissible for a copy/move constructor to have additional
       // parameters as long as they have default arguments defined for them.
       assert(S->getNumArgs() != 0);
 
       const Expr *Arg = S->getArg(0);
       assert(Arg != nullptr);
 
       auto *ArgLoc = cast_or_null<AggregateStorageLocation>(
-          Env.getStorageLocation(*Arg, SkipPast::Reference));
+          Env.getStorageLocationStrict(*Arg));
       if (ArgLoc == nullptr)
         return;
 
       if (S->isElidable()) {
-        Env.setStorageLocation(*S, *ArgLoc);
-      } else if (auto *ArgVal = cast_or_null<StructValue>(Env.getValue(*Arg))) {
+        if (Value *Val = Env.getValue(*ArgLoc))
+          Env.setValueStrict(*S, *Val);
+      } else {
         auto &Val = *cast<StructValue>(Env.createValue(S->getType()));
         Env.setValueStrict(*S, Val);
-        copyRecord(ArgVal->getAggregateLoc(), Val.getAggregateLoc(), Env);
+        copyRecord(*ArgLoc, Val.getAggregateLoc(), Env);
       }
       return;
     }
 
     // `CXXConstructExpr` can have array type if default-initializing an array
     // of records, and we currently can't create values for arrays. So check if
     // we've got a record type.
     if (S->getType()->isRecordType()) {
void VisitCallExpr(const CallExpr *S) {
     // others (like trap, debugtrap, and unreachable) are handled by CFG
     // construction.
     if (S->isCallToStdMove()) {
       assert(S->getNumArgs() == 1);
 
       const Expr *Arg = S->getArg(0);
       assert(Arg != nullptr);
 
-      auto *ArgLoc = Env.getStorageLocation(*Arg, SkipPast::None);
+      auto *ArgLoc = Env.getStorageLocationStrict(*Arg);
       if (ArgLoc == nullptr)
         return;
 
-      Env.setStorageLocation(*S, *ArgLoc);
+      Env.setStorageLocationStrict(*S, *ArgLoc);
     } else if (S->getDirectCallee() != nullptr &&
                S->getDirectCallee()->getBuiltinID() ==
                    Builtin::BI__builtin_expect) {
       assert(S->getNumArgs() > 0);
       assert(S->getArg(0) != nullptr);
-      // `__builtin_expect` returns by-value, so strip away any potential
-      // references in the argument.
-      auto *ArgLoc = Env.getStorageLocation(*S->getArg(0), SkipPast::Reference);
-      if (ArgLoc == nullptr)
+      auto *ArgVal = Env.getValue(*S->getArg(0));
+      if (ArgVal == nullptr)
         return;
-      Env.setStorageLocation(*S, *ArgLoc);
+      Env.setValueStrict(*S, *ArgVal);
     } else if (const FunctionDecl *F = S->getDirectCallee()) {
       transferInlineCall(S, F);
     }
   }
 
   void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *S) {
     const Expr *SubExpr = S->getSubExpr();
     assert(SubExpr != nullptr);
 
     Value *SubExprVal = Env.getValue(*SubExpr);
     if (SubExprVal == nullptr)
       return;
 
     if (StructValue *StructVal = dyn_cast<StructValue>(SubExprVal)) {
-      Env.setStorageLocation(*S, StructVal->getAggregateLoc());
+      Env.setStorageLocationStrict(*S, StructVal->getAggregateLoc());
       return;
     }
 
     StorageLocation &Loc = Env.createStorageLocation(*S);
     Env.setValue(Loc, *SubExprVal);
-    Env.setStorageLocation(*S, Loc);
+    Env.setStorageLocationStrict(*S, Loc);
   }
 
   void VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *S) {
     const Expr *SubExpr = S->getSubExpr();
     assert(SubExpr != nullptr);
 
     propagateValue(*SubExpr, *S, Env);
   }