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

[Yitzhak Mandelbaum via cfe-commits]

Author: Yitzhak Mandelbaum
Date: 2023-01-24T16:10:50Z
New Revision: b84ac96a35c72420b45db6385f83a5e0c516349f

URL: https://github.com/llvm/llvm-project/commit/b84ac96a35c72420b45db6385f83a5e0c516349f
DIFF: https://github.com/llvm/llvm-project/commit/b84ac96a35c72420b45db6385f83a5e0c516349f.diff

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

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.

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

Added: 
    

Modified: 
    clang/lib/Analysis/FlowSensitive/Transfer.cpp
    clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 9fa17c86d2d9d..0e6c484b67e7d 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -197,6 +197,8 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
       return;
 
     if (VD->getType()->isReferenceType()) {
+      assert(isa_and_nonnull<ReferenceValue>(Env.getValue((*DeclLoc))) &&
+             "reference-typed declarations map to `ReferenceValue`s");
       Env.setStorageLocation(*S, *DeclLoc);
     } else {
       auto &Loc = Env.createStorageLocation(*S);
@@ -474,6 +476,8 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
           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);
@@ -494,7 +498,22 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
 
     auto &MemberLoc = BaseLoc->getChild(*Member);
     if (MemberLoc.getType()->isReferenceType()) {
-      Env.setStorageLocation(*S, MemberLoc);
+      // 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);

diff  --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 1c06755e5aa3c..5d56d98235f15 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -156,6 +156,89 @@ TEST(TransferTest, IntVarDecl) {
       });
 }
 
+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 {