[clang][dataflow] Use `Strict` accessors in more places in Transfer.cpp.

[mboehme]

Summary

This patch handles the straightforward cases. Upcoming separate patches will handle the cases that are more subtle.

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

Depends On D150653

[sammccall]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

138

the name "forward" isn't clear to me - if anything suggesting std::forward, but not assignment.

Maybe assignValue or copyValue, and aliasLocation for the storageloc variant?

That leaves "forwardValueOrStorageLocation", but that only has one callsite and part of the point of this refactoring is that we're best thinking about these cases explicitly, right? So could just inline the if.

[mboehme]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

138

(Haven't made any changes yet because I'd like to discuss first.)

That leaves "forwardValueOrStorageLocation", but that only has one callsite and part of the point of this refactoring is that we're best thinking about these cases explicitly, right? So could just inline the if.

This function will end up being used in more places though. I have some draft patches that use it (for the comma operator, for example) but these aren't ready for review yet. But as I know the operation will be used elsewhere, it made sense to create an abstraction now (and we'll definitely need a name for it at some point).

I think the most generic of the names you suggest is copy, and I think it works reasonably well: copyValue(), copyStorageLocation(), copyValueOrStorageLocation(). WDYT?

[ymandel]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

138

I see Martin's point in not wanting to use copy/assign, since the same value will be shared between the locations. but, I see why "forward" has multiple meanings. Maybe "share" or "propogate"?

751–752

just else if?

[mboehme]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

138

I like "propagate", since it conveys the same meaning that I was going for with "forward", but without the allusion to std::forward. I like it better than "copy" or "assign" because those also have specific meanings in C++, and "propagate" doesn't.

@sammccall WDYT?

[sammccall]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

138

Propagate is fine, personally I like "share" better though because "sharing a location" is direct and "propagating a location" is abstract.

I don't think propagateValueOrStorageLocation() should exist though. I can deal with the new model where lvalues and rvalues are totally different things, or the old model where we pretend references are bizarro-pointers and lvalues and rvalues are basically the same. I don't think saving a few if statements is a good reason to mix the two. In the new model the function does two ~unrelated things.

[mboehme]

Simplified else { if {} } to else if {}.

[mboehme]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

751–752

Done.

[mboehme]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

138

Propagate is fine, personally I like "share" better though because "sharing a location" is direct and "propagating a location" is abstract.

"Share" doesn't imply a direction ("from ... to ...") in the way that "propagate" does -- so if it's OK with you, I'd like to use "propagate".

I don't think propagateValueOrStorageLocation() should exist though. I can deal with the new model where lvalues and rvalues are totally different things, or the old model where we pretend references are bizarro-pointers and lvalues and rvalues are basically the same. I don't think saving a few if statements is a good reason to mix the two. In the new model the function does two ~unrelated things.

While most AST nodes operate either on prvalues or glvalues, there are some that can propagate either a prvalue or a glvalue. For example:

• The conditional operator
• The comma operator (for the RHS)
• No-op casts

The operation I'm trying to express is "propagate a value of any category (but don't change its category)". I think this is a useful abstraction that corresponds to a concept in the language, but I'm happy to inline the code if you feel strongly about this.

[xazax.hun]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

717–724

This operation is basically something like an RValue to LValue cast. I am not sure how many of these will we have, but it might be a good idea to look out if we want to create something like propagateAsLValue similar to propagateValue.

[sammccall]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

138

Yeah, propagate is fine, and go ahead with the function, I don't think I feel strongly about it in the end.

propagate a value of any category

(For the record, I think "value of any category" is pretty hard to keep hold of now: it has no type (Value and StorageLocation are distinct) and no name (we're using "Value" to mean ~rvalue), which is why I find it hard to think about this operation)

[mboehme]

Renamed forward... functions to propagate....

[mboehme]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

717–724

I'll keep an eye out for this, but it's plausible to me that "materialize temporary" is the only operation of this kind. ("Materialize temporary" essentially means "rvalue to lvalue", if you think about it.)

clang/lib/Analysis/FlowSensitive/Transfer.cpp

if (!CFCtx.isBlockReachable(*BlockIt->getSecond()))
     return nullptr;
   const auto &State = BlockToState[BlockIt->getSecond()->getBlockID()];
   assert(State);
   return &State->Env;
 }
 
 static BoolValue &evaluateBooleanEquality(const Expr &LHS, const Expr &RHS,
                                           Environment &Env) {
-  if (auto *LHSValue =
-          dyn_cast_or_null<BoolValue>(Env.getValue(LHS, SkipPast::Reference)))
-    if (auto *RHSValue =
-            dyn_cast_or_null<BoolValue>(Env.getValue(RHS, SkipPast::Reference)))
+  if (auto *LHSValue = dyn_cast_or_null<BoolValue>(Env.getValueStrict(LHS)))
+    if (auto *RHSValue = dyn_cast_or_null<BoolValue>(Env.getValueStrict(RHS)))
       return Env.makeIff(*LHSValue, *RHSValue);
 
   return Env.makeAtomicBoolValue();
 }
 
 // Functionally updates `V` such that any instances of `TopBool` are replaced
 // with fresh atomic bools. Note: This implementation assumes that `B` is a
 // tree; if `B` is a DAG, it will lose any sharing between subvalues that was
static BoolValue &unpackValue(BoolValue &V, Environment &Env) {
   }
   llvm_unreachable("All reachable cases in switch return");
 }
 
 // Unpacks the value (if any) associated with `E` and updates `E` to the new
 // value, if any unpacking occured. Also, does the lvalue-to-rvalue conversion,
 // by skipping past the reference.
 static Value *maybeUnpackLValueExpr(const Expr &E, Environment &Env) {
-  // FIXME: this is too flexible: it _allows_ a reference, while it should
-  // _require_ one, since lvalues should always be wrapped in `ReferenceValue`.
-  auto *Loc = Env.getStorageLocation(E, SkipPast::Reference);
+  auto *Loc = Env.getStorageLocationStrict(E);
   if (Loc == nullptr)
     return nullptr;
   auto *Val = Env.getValue(*Loc);
 
   auto *B = dyn_cast_or_null<BoolValue>(Val);
   if (B == nullptr)
     return Val;
 
   auto &UnpackedVal = unpackValue(*B, Env);
   if (&UnpackedVal == Val)
     return Val;
   Env.setValue(*Loc, UnpackedVal);
   return &UnpackedVal;
 }
 
+static void propagateValue(const Expr &From, const Expr &To, Environment &Env) {
+  if (auto *Val = Env.getValueStrict(From))
+    Env.setValueStrict(To, *Val);
+}
+
+static void propagateStorageLocation(const Expr &From, const Expr &To,
+                                     Environment &Env) {
+  if (auto *Loc = Env.getStorageLocationStrict(From))
+    Env.setStorageLocationStrict(To, *Loc);
+}
+
+// Forwards the value or storage location of `From` to `To` in cases where
+// `From` may be either a glvalue or a prvalue. `To` must be a glvalue iff
+// `From` is a glvalue.
+static void propagateValueOrStorageLocation(const Expr &From, const Expr &To,
+                                            Environment &Env) {
+  assert(From.isGLValue() == To.isGLValue());
+  if (From.isGLValue())
+    propagateStorageLocation(From, To, Env);
+  else
+    propagateValue(From, To, Env);
+}
+
 namespace {
 
 class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
 public:
   TransferVisitor(const StmtToEnvMap &StmtToEnv, Environment &Env)
       : StmtToEnv(StmtToEnv), Env(Env) {}
 
   void VisitBinaryOperator(const BinaryOperator *S) {
     const Expr *LHS = S->getLHS();
     assert(LHS != nullptr);
 
     const Expr *RHS = S->getRHS();
     assert(RHS != nullptr);
 
     switch (S->getOpcode()) {
     case BO_Assign: {
-      auto *LHSLoc = Env.getStorageLocation(*LHS, SkipPast::Reference);
+      auto *LHSLoc = Env.getStorageLocationStrict(*LHS);
       if (LHSLoc == nullptr)
         break;
 
-      // No skipping should be necessary, because any lvalues should have
-      // already been stripped off in evaluating the LValueToRValue cast.
-      auto *RHSVal = Env.getValue(*RHS, SkipPast::None);
+      auto *RHSVal = Env.getValueStrict(*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);
if (D.getType()->isReferenceType()) {
       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;
       }
 
-      if (auto *InitExprVal = Env.getValue(*InitExpr, SkipPast::None))
+      if (auto *InitExprVal = Env.getValueStrict(*InitExpr))
         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
case UO_AddrOf: {
       if (PointeeLoc == nullptr)
         break;
 
       Env.setValueStrict(*S, Env.create<PointerValue>(*PointeeLoc));
       break;
     }
     case UO_LNot: {
       auto *SubExprVal =
-          dyn_cast_or_null<BoolValue>(Env.getValue(*SubExpr, SkipPast::None));
+          dyn_cast_or_null<BoolValue>(Env.getValueStrict(*SubExpr));
       if (SubExprVal == nullptr)
         break;
 
       auto &ExprLoc = Env.createStorageLocation(*S);
       Env.setStorageLocation(*S, ExprLoc);
       Env.setValue(ExprLoc, Env.makeNot(*SubExprVal));
       break;
     }
void VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *S) {
     }
   }
 
   void VisitCXXFunctionalCastExpr(const CXXFunctionalCastExpr *S) {
     if (S->getCastKind() == CK_ConstructorConversion) {
       const Expr *SubExpr = S->getSubExpr();
       assert(SubExpr != nullptr);
 
-      auto *SubExprLoc = Env.getStorageLocation(*SubExpr, SkipPast::None);
-      if (SubExprLoc == nullptr)
-        return;
-
-      Env.setStorageLocation(*S, *SubExprLoc);
+      propagateValue(*SubExpr, *S, Env);
     }
   }
 
   void VisitCXXTemporaryObjectExpr(const CXXTemporaryObjectExpr *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 VisitCallExpr(const CallExpr *S) {
     // Of clang's builtins, only `__builtin_expect` is handled explicitly, since
     // others (like trap, debugtrap, and unreachable) are handled by CFG
     // construction.
     if (S->isCallToStdMove()) {
       assert(S->getNumArgs() == 1);
if (S->isCallToStdMove()) {
       transferInlineCall(S, F);
     }
   }
 
   void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *S) {
     const Expr *SubExpr = S->getSubExpr();
     assert(SubExpr != nullptr);
 
-    auto *SubExprLoc = Env.getStorageLocation(*SubExpr, SkipPast::None);
-    if (SubExprLoc == nullptr)
+    Value *SubExprVal = Env.getValueStrict(*SubExpr);
+    if (SubExprVal == nullptr)
       return;
 
-    Env.setStorageLocation(*S, *SubExprLoc);
+    auto &Loc = Env.createStorageLocation(*S);
+    Env.setStorageLocationStrict(*S, Loc);
+    Env.setValue(Loc, *SubExprVal);
   }
 
   void VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *S) {
     const Expr *SubExpr = S->getSubExpr();
     assert(SubExpr != nullptr);
 
-    auto *SubExprLoc = Env.getStorageLocation(*SubExpr, SkipPast::None);
-    if (SubExprLoc == nullptr)
-      return;
-
-    Env.setStorageLocation(*S, *SubExprLoc);
+    propagateValue(*SubExpr, *S, Env);
   }
 
   void VisitCXXStaticCastExpr(const CXXStaticCastExpr *S) {
     if (S->getCastKind() == CK_NoOp) {
       const Expr *SubExpr = S->getSubExpr();
       assert(SubExpr != nullptr);
 
-      auto *SubExprLoc = Env.getStorageLocation(*SubExpr, SkipPast::None);
-      if (SubExprLoc == nullptr)
-        return;
-
-      Env.setStorageLocation(*S, *SubExprLoc);
+      propagateValueOrStorageLocation(*SubExpr, *S, Env);
     }
   }
 
   void VisitConditionalOperator(const ConditionalOperator *S) {
     // FIXME: Revisit this once flow conditions are added to the framework. For
     // `a = b ? c : d` we can add `b => a == c && !b => a == d` to the flow
     // condition.
+    if (S->isGLValue()) {
       auto &Loc = Env.createStorageLocation(*S);
-    Env.setStorageLocation(*S, Loc);
+      Env.setStorageLocationStrict(*S, Loc);
       if (Value *Val = Env.createValue(S->getType()))
         Env.setValue(Loc, *Val);
+    } else if (Value *Val = Env.createValue(S->getType())) {
+      Env.setValueStrict(*S, *Val);
+    }
   }
 
   void VisitInitListExpr(const InitListExpr *S) {
     QualType Type = S->getType();
 
     auto &Loc = Env.createStorageLocation(*S);
     Env.setStorageLocation(*S, Loc);
 
if (Type->isStructureOrClassType()) {
         if (Value *InitVal = Env.getValue(*Init, SkipPast::None))
           cast<StructValue>(Val)->setChild(*Field, *InitVal);
       }
     }
     // FIXME: Implement array initialization.
   }
 
   void VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *S) {
-    auto &Loc = Env.createStorageLocation(*S);
-    Env.setStorageLocation(*S, Loc);
-    Env.setValue(Loc, Env.getBoolLiteralValue(S->getValue()));
+    Env.setValueStrict(*S, Env.getBoolLiteralValue(S->getValue()));
   }
 
   void VisitParenExpr(const ParenExpr *S) {
     // The CFG does not contain `ParenExpr` as top-level statements in basic
     // blocks, however manual traversal to sub-expressions may encounter them.
     // Redirect to the sub-expression.
     auto *SubExpr = S->getSubExpr();
     assert(SubExpr != nullptr);
private:
   BoolValue *getLogicOperatorSubExprValue(const Expr &SubExpr) {
     // `SubExpr` and its parent logic operator might be part of different basic
     // blocks. We try to access the value that is assigned to `SubExpr` in the
     // corresponding environment.
     const Environment *SubExprEnv = StmtToEnv.getEnvironment(SubExpr);
     if (!SubExprEnv)
       return nullptr;
 
-    if (auto *Val = dyn_cast_or_null<BoolValue>(
-            SubExprEnv->getValue(SubExpr, SkipPast::Reference)))
+    if (auto *Val =
+            dyn_cast_or_null<BoolValue>(SubExprEnv->getValueStrict(SubExpr)))
       return Val;
 
-    if (Env.getStorageLocation(SubExpr, SkipPast::None) == nullptr) {
+    if (Env.getValueStrict(SubExpr) == nullptr) {
       // Sub-expressions that are logic operators are not added in basic blocks
       // (e.g. see CFG for `bool d = a && (b || c);`). If `SubExpr` is a logic
       // operator, it may not have been evaluated and assigned a value yet. In
       // that case, we need to first visit `SubExpr` and then try to get the
       // value that gets assigned to it.
       Visit(&SubExpr);
     }
 
-    if (auto *Val = dyn_cast_or_null<BoolValue>(
-            Env.getValue(SubExpr, SkipPast::Reference)))
+    if (auto *Val = dyn_cast_or_null<BoolValue>(Env.getValueStrict(SubExpr)))
       return Val;
 
     // If the value of `SubExpr` is still unknown, we create a fresh symbolic
     // boolean value for it.
     return &Env.makeAtomicBoolValue();
   }
 
   // If context sensitivity is enabled, try to analyze the body of the callee