[clang][dataflow] Fix bug in handling of reference-typed fields.

[ymandel]

Summary

This patch fixes a subtle bug in how we create lvalues to reference-typed
fields. In the rare case that the field is umodeled because of the depth limit
on field modeling, the lvalue created can be malformed. This patch prevents that
and adds some related assertions to other code dealing with lvalues for
references.

[sgatev]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

209

Can we use isa_and_nonnull(Env.getValue(Loc)) instead?

[ymandel]

use isa_and_nonnull

[ymandel]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

209

Yes, much better. Thanks!

clang/lib/Analysis/FlowSensitive/Transfer.cpp

static Value *maybeUnpackLValueExpr(const Expr &E, Environment &Env) {
 
   auto &UnpackedVal = unpackValue(*B, Env);
   if (&UnpackedVal == Val)
     return Val;
   Env.setValue(*Loc, UnpackedVal);
   return &UnpackedVal;
 }
 
+// `isValidReferenceLoc` is only called inside `assert`, so we wrap it in
+// #ifndef to prevent warnings in opt builds.
+#ifndef NDEBUG
+static bool isValidReferenceLoc(StorageLocation &Loc, Environment &Env) {
+  auto *V = Env.getValue(Loc);
+  return V != nullptr && isa<ReferenceValue>(V);
+}
+#endif
+
 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);
public:
   void VisitDeclRefExpr(const DeclRefExpr *S) {
     const ValueDecl *VD = S->getDecl();
     assert(VD != nullptr);
     auto *DeclLoc = Env.getStorageLocation(*VD, SkipPast::None);
     if (DeclLoc == nullptr)
       return;
 
     if (VD->getType()->isReferenceType()) {
+      assert(isValidReferenceLoc(*DeclLoc, Env) &&
+             "reference-typed declarations map to `ReferenceValue`s");
       Env.setStorageLocation(*S, *DeclLoc);
     } else {
       auto &Loc = Env.createStorageLocation(*S);
       auto &Val = Env.takeOwnership(std::make_unique<ReferenceValue>(*DeclLoc));
       Env.setStorageLocation(*S, Loc);
       Env.setValue(Loc, Val);
     }
   }
void VisitMemberExpr(const MemberExpr *S) {
 
     if (auto *D = dyn_cast<VarDecl>(Member)) {
       if (D->hasGlobalStorage()) {
         auto *VarDeclLoc = Env.getStorageLocation(*D, SkipPast::None);
         if (VarDeclLoc == nullptr)
           return;
 
         if (VarDeclLoc->getType()->isReferenceType()) {
+          assert(isValidReferenceLoc(*VarDeclLoc, Env) &&
+                 "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.takeOwnership(
                                 std::make_unique<ReferenceValue>(*VarDeclLoc)));
         }
         return;
       }
     }
 
     // The receiver can be either a value or a pointer to a value. Skip past the
     // indirection to handle both cases.
     auto *BaseLoc = cast_or_null<AggregateStorageLocation>(
         Env.getStorageLocation(*S->getBase(), SkipPast::ReferenceThenPointer));
     if (BaseLoc == nullptr)
       return;
 
     auto &MemberLoc = BaseLoc->getChild(*Member);
     if (MemberLoc.getType()->isReferenceType()) {
+      // Based on its type, `MemberLoc` must be mapped either to nothing or to a
+      // `ReferenceValue`. For the former, we won't set a storage location for
+      // this expression, so as to maintain an invariant lvalue expressions;
+      // namely, that their location maps to a `ReferenceValue`.  In this,
+      // lvalues are unlike other expressions, where it is valid for their
+      // location to map to nothing (because they are not modeled).
+      //
+      // Note: we need this invariant for lvalues so that, when accessing a
+      // value, we can distinguish an rvalue from an lvalue. An alternative
+      // design, which takes the expression's value category into account, would
+      // avoid the need for this invariant.
+      if (auto *V = Env.getValue(MemberLoc)) {
+        assert(isa<ReferenceValue>(V) &&
+               "reference-typed declarations map to `ReferenceValue`s");
         Env.setStorageLocation(*S, MemberLoc);
+      }
     } else {
       auto &Loc = Env.createStorageLocation(*S);
       Env.setStorageLocation(*S, Loc);
       Env.setValue(
           Loc, Env.takeOwnership(std::make_unique<ReferenceValue>(MemberLoc)));
     }
   }
 

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

runDataflow(
             Env.getStorageLocation(*FooDecl, SkipPast::None);
         ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(FooLoc));
 
         const Value *FooVal = Env.getValue(*FooLoc);
         EXPECT_TRUE(isa_and_nonnull<IntegerValue>(FooVal));
       });
 }
 
+TEST(TransferTest, StructIncomplete) {
+  std::string Code = R"(
+    struct A;
+
+    void target() {
+      A* Foo;
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+        ASSERT_THAT(FooDecl, NotNull());
+        auto *FooValue = dyn_cast_or_null<PointerValue>(
+            Env.getValue(*FooDecl, SkipPast::None));
+        ASSERT_THAT(FooValue, NotNull());
+
+        EXPECT_TRUE(isa<AggregateStorageLocation>(FooValue->getPointeeLoc()));
+        auto *FooPointeeValue = Env.getValue(FooValue->getPointeeLoc());
+        ASSERT_THAT(FooPointeeValue, NotNull());
+        EXPECT_TRUE(isa<StructValue>(FooPointeeValue));
+      });
+}
+
+// As a memory optimization, we prevent modeling fields nested below a certain
+// level (currently, depth 3). This test verifies this lack of modeling. We also
+// include a regression test for the case that the unmodeled field is a
+// reference to a struct; previously, we crashed when accessing such a field.
+TEST(TransferTest, StructFieldUnmodeled) {
+  std::string Code = R"(
+    struct S { int X; };
+    S GlobalS;
+    struct A { S &Unmodeled = GlobalS; };
+    struct B { A F3; };
+    struct C { B F2; };
+    struct D { C F1; };
+
+    void target() {
+      D Bar;
+      A Foo = Bar.F1.F2.F3;
+      int Zab = Foo.Unmodeled.X;
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+        ASSERT_THAT(FooDecl, NotNull());
+        ASSERT_TRUE(FooDecl->getType()->isStructureType());
+        auto FooFields = FooDecl->getType()->getAsRecordDecl()->fields();
+
+        FieldDecl *UnmodeledDecl = nullptr;
+        for (FieldDecl *Field : FooFields) {
+          if (Field->getNameAsString() == "Unmodeled") {
+            UnmodeledDecl = Field;
+          } else {
+            FAIL() << "Unexpected field: " << Field->getNameAsString();
+          }
+        }
+        ASSERT_THAT(UnmodeledDecl, NotNull());
+
+        const auto *FooLoc = cast<AggregateStorageLocation>(
+            Env.getStorageLocation(*FooDecl, SkipPast::None));
+        const auto *UnmodeledLoc = &FooLoc->getChild(*UnmodeledDecl);
+        ASSERT_TRUE(isa<ScalarStorageLocation>(UnmodeledLoc));
+        ASSERT_THAT(Env.getValue(*UnmodeledLoc), IsNull());
+
+        const ValueDecl *ZabDecl = findValueDecl(ASTCtx, "Zab");
+        ASSERT_THAT(ZabDecl, NotNull());
+        EXPECT_THAT(Env.getValue(*ZabDecl, SkipPast::None), NotNull());
+      });
+}
+
 TEST(TransferTest, StructVarDecl) {
   std::string Code = R"(
     struct A {
       int Bar;
     };
 
     void target() {
       A Foo;