[clang][dataflow] Use `Strict` accessors in SignAnalysisTest.cpp.

[mboehme]

Summary

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 D150656

[sammccall]

clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp

240

this seems to belong on Environment rather than here!

[mboehme]

Added a comment

[mboehme]

clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp

240

I've consciously kept this function local to this file for now because it has relatively complex semantics and I haven't seen any other places where these are required so far. I've added a comment noting that this should be moved if it turns out to be needed elsewhere.

I think that, as much as possible, the methods on Environment should be specific about the value category they operate on, because most of the time, it's known what the value category is, and I'd like to encourage users to use the most specific operation possible (because using more general operations breeds bugs).

[sammccall]

clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp

240

I think that, as much as possible, the methods on Environment should be specific about the value category they operate on

I agree, but don't think that's specific to Environment.
I think my favorite factoring of this would be to inline the if/else and move the branches to methods on environment

Value *Val = E->isGLValue() ? Env.getOrCreateLValue(*E) : Env.getOrCreateValue(*E);

but this is test code, it doesn't matter much, up to you.

[xazax.hun]

clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp

134

Wow, this is so much cleaner than before!

240

So, we could either make an API that forces users to think about value categories explicitly, or we could try to design an API where the users do not need to know about value categories at all. I think it might be worth exploring if the latter is possible without any major caveats, the simpler the APIs are the better. But this is for the future, not for this PR.

[mboehme]

clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp

240

I think that, as much as possible, the methods on Environment should be specific about the value category they operate on

I agree, but don't think that's specific to Environment.
I think my favorite factoring of this would be to inline the if/else and move the branches to methods on environment

Value *Val = E->isGLValue() ? Env.getOrCreateLValue(*E) : Env.getOrCreateValue(*E);

I'm hesitant to do this because even getOrCreateValue() is an operation with relatively "loose" semantics. If anything, my sense is that operations like this should go in TestingSupport.h rather than Environment -- from what I've seen so far, these types of operations tend to occur mostly in tests, while the non-test code usually has more "rigid" semantics.

240

So, we could either make an API that forces users to think about value categories explicitly, or we could try to design an API where the users do not need to know about value categories at all. I think it might be worth exploring if the latter is possible without any major caveats, the simpler the APIs are the better. But this is for the future, not for this PR.

I'll keep this in mind, but I'll admit I have my doubts. The existing API can be seen as doing this (i.e. providing an API that does not require knowing about value categories), and it is actually more complex to use in many cases, and more error-prone. Fundamentally, I think if you're dealing with static analysis on the Clang AST you need to understand value categories to get the code right.

clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp

getValueAndSignProperties(const UnaryOperator *UO,
   // The DeclRefExpr refers to this variable in the operand.
   const auto *OperandVar = M.Nodes.getNodeAs<clang::VarDecl>(kVar);
   assert(OperandVar != nullptr);
   const auto *OperandValue = State.Env.getValue(*OperandVar);
   if (!OperandValue)
     return {nullptr, {}, {}};
 
   // Value of the unary op.
-  auto *UnaryOpValue = State.Env.getValue(*UO, SkipPast::None);
+  auto *UnaryOpValue = State.Env.getValueStrict(*UO);
   if (!UnaryOpValue) {
-    auto &Loc = State.Env.createStorageLocation(*UO);
-    State.Env.setStorageLocation(*UO, Loc);
     UnaryOpValue = &State.Env.makeAtomicBoolValue();
-    State.Env.setValue(Loc, *UnaryOpValue);
+    State.Env.setValueStrict(*UO, *UnaryOpValue);
   }
 
   // Properties for the operand (sub expression).
   SignProperties OperandProps = getSignProperties(*OperandValue, State.Env);
   if (OperandProps.Neg == nullptr)
     return {nullptr, {}, {}};
   // Properties for the operator expr itself.
   SignProperties UnaryOpProps = initUnknown(*UnaryOpValue, State.Env);
   return {UnaryOpValue, UnaryOpProps, OperandProps};
 }
 
 void transferBinary(const BinaryOperator *BO, const MatchFinder::MatchResult &M,
                     LatticeTransferState &State) {
-  StorageLocation *Loc = State.Env.getStorageLocation(*BO, SkipPast::None);
-  if (!Loc) {
-    Loc = &State.Env.createStorageLocation(*BO);
-    State.Env.setStorageLocation(*BO, *Loc);
-  }
-  BoolValue *Comp = cast_or_null<BoolValue>(State.Env.getValue(*Loc));
+  BoolValue *Comp = cast_or_null<BoolValue>(State.Env.getValueStrict(*BO));
   if (!Comp) {
     Comp = &State.Env.makeAtomicBoolValue();
-    State.Env.setValue(*Loc, *Comp);
+    State.Env.setValueStrict(*BO, *Comp);
   }
 
   // FIXME Use this as well:
   // auto *NegatedComp = &State.Env.makeNot(*Comp);
 
-  auto *LHS = State.Env.getValue(*BO->getLHS(), SkipPast::None);
-  auto *RHS = State.Env.getValue(*BO->getRHS(), SkipPast::None);
+  auto *LHS = State.Env.getValueStrict(*BO->getLHS());
+  auto *RHS = State.Env.getValueStrict(*BO->getRHS());
 
   if (!LHS || !RHS)
     return;
 
   SignProperties LHSProps = getSignProperties(*LHS, State.Env);
   SignProperties RHSProps = getSignProperties(*RHS, State.Env);
   if (LHSProps.Neg == nullptr || RHSProps.Neg == nullptr)
     return;
if (auto *UOBoolVal = dyn_cast<BoolValue>(UnaryOpValue)) {
     State.Env.addToFlowCondition(
         State.Env.makeIff(*UOBoolVal, *OperandProps.Zero));
     // !a <==> !a is not zero
     State.Env.addToFlowCondition(
         State.Env.makeIff(*UOBoolVal, State.Env.makeNot(*UnaryOpProps.Zero)));
   }
 }
 
-void transferExpr(const Expr *E, const MatchFinder::MatchResult &M,
-                  LatticeTransferState &State) {
-  const ASTContext &Context = *M.Context;
-  StorageLocation *Loc = State.Env.getStorageLocation(*E, SkipPast::None);
+// Returns the `Value` associated with `E` (which may be either a prvalue or
+// glvalue). Creates a `Value` or `StorageLocation` as needed if `E` does not
+// have either of these associated with it yet.
+//
+// If this functionality turns out to be needed in more cases, this function
+// should be moved to a more central location.
+Value *getOrCreateValue(const Expr *E, Environment &Env) {
+  Value *Val = nullptr;
+  if (E->isGLValue()) {
+    StorageLocation *Loc = Env.getStorageLocationStrict(*E);
     if (!Loc) {
-    Loc = &State.Env.createStorageLocation(*E);
-    State.Env.setStorageLocation(*E, *Loc);
+      Loc = &Env.createStorageLocation(*E);
+      Env.setStorageLocationStrict(*E, *Loc);
     }
-  Value *Val = State.Env.getValue(*Loc);
+    Val = Env.getValue(*Loc);
+    if (!Val) {
+      Val = Env.createValue(E->getType());
+      Env.setValue(*Loc, *Val);
+    }
+  } else {
+    Val = Env.getValueStrict(*E);
     if (!Val) {
-    Val = State.Env.createValue(Context.IntTy);
-    State.Env.setValue(*Loc, *Val);
+      Val = Env.createValue(E->getType());
+      Env.setValueStrict(*E, *Val);
     }
+  }
+  assert(Val != nullptr);
+
+  return Val;
+}
+
+void transferExpr(const Expr *E, const MatchFinder::MatchResult &M,
+                  LatticeTransferState &State) {
+  const ASTContext &Context = *M.Context;
+
+  Value *Val = getOrCreateValue(E, State.Env);
+
   // The sign symbolic values have been initialized already.
   if (Val->getProperty("neg"))
     return;
 
   Expr::EvalResult R;
   // An integer expression which we cannot evaluate.
   if (!(E->EvaluateAsInt(R, Context) && R.Val.isInt())) {
     initUnknown(*Val, State.Env);