[clang][dataflow] Propagate locations from result objects to initializers. (#87320)

Previously, we were propagating storage locations the other way around,
i.e.
from initializers to result objects, using `RecordValue::getLoc()`. This
gave
the wrong behavior in some cases -- see the newly added or fixed tests
in this
patch.

In addition, this patch now unblocks removing the `RecordValue` class
entirely,
as we no longer need `RecordValue::getLoc()`.

With this patch, the test `TransferTest.DifferentReferenceLocInJoin`
started to
fail because the framework now always uses the same storge location for
a
`MaterializeTemporaryExpr`, meaning that the code under test no longer
set up
the desired state where a variable of reference type is mapped to two
different
storage locations in environments being joined. Rather than trying to
modify
this test to set up the test condition again, I have chosen to replace
the test
with an equivalent test in DataflowEnvironmentTest.cpp that sets up the
test
condition directly; because this test is more direct, it will also be
less
brittle in the face of future changes.

1 files changed, 115 insertions, 57 deletions

diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index ca055a4..00dafb2 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -1582,10 +1582,9 @@ TEST(TransferTest, FieldsDontHaveValuesInConstructorWithBaseClass) {
       [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
          ASTContext &ASTCtx) {
         const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
-        // FIXME: The field of the base class should already have been
-        // initialized with a value by the base constructor. This test documents
-        // the current buggy behavior.
-        EXPECT_EQ(getFieldValue(Env.getThisPointeeStorageLocation(), "BaseVal",
+        // The field of the base class should already have been initialized with
+        // a value by the base constructor.
+        EXPECT_NE(getFieldValue(Env.getThisPointeeStorageLocation(), "BaseVal",
                                 ASTCtx, Env),
                   nullptr);
         EXPECT_EQ(getFieldValue(Env.getThisPointeeStorageLocation(), "Val",
@@ -2998,8 +2997,12 @@ TEST(TransferTest, ResultObjectLocation) {
 
 TEST(TransferTest, ResultObjectLocationForDefaultArgExpr) {
   std::string Code = R"(
-    struct S {};
-    void funcWithDefaultArg(S s = S());
+    struct Inner {};
+    struct Outer {
+        Inner I = {};
+    };
+
+    void funcWithDefaultArg(Outer O = {});
     void target() {
       funcWithDefaultArg();
       // [[p]]
@@ -3058,13 +3061,7 @@ TEST(TransferTest, ResultObjectLocationForDefaultInitExpr) {
 
         RecordStorageLocation &Loc = Env.getResultObjectLocation(*DefaultInit);
 
-        // FIXME: The result object location for the `CXXDefaultInitExpr` should
-        // be the location of the member variable being initialized, but we
-        // don't do this correctly yet; see also comments in
-        // `builtinTransferInitializer()`.
-        // For the time being, we just document the current erroneous behavior
-        // here (this should be `EXPECT_EQ` when the behavior is fixed).
-        EXPECT_NE(&Loc, Env.getThisPointeeStorageLocation()->getChild(*SField));
+        EXPECT_EQ(&Loc, Env.getThisPointeeStorageLocation()->getChild(*SField));
       });
 }
 
@@ -3101,6 +3098,79 @@ TEST(TransferTest, ResultObjectLocationForCXXOperatorCallExpr) {
       });
 }
 
+TEST(TransferTest, ResultObjectLocationForStdInitializerListExpr) {
+  std::string Code = R"(
+    namespace std {
+    template <typename T>
+    struct initializer_list {};
+    } // namespace std
+
+    void target() {
+      std::initializer_list<int> list = {1};
+      // [[p]]
+    }
+  )";
+
+  using ast_matchers::cxxStdInitializerListExpr;
+  using ast_matchers::match;
+  using ast_matchers::selectFirst;
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        auto *StdInitList = selectFirst<CXXStdInitializerListExpr>(
+            "std_init_list",
+            match(cxxStdInitializerListExpr().bind("std_init_list"), ASTCtx));
+        ASSERT_NE(StdInitList, nullptr);
+
+        EXPECT_EQ(&Env.getResultObjectLocation(*StdInitList),
+                  &getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "list"));
+      });
+}
+
+TEST(TransferTest, ResultObjectLocationPropagatesThroughConditionalOperator) {
+  std::string Code = R"(
+    struct A {
+      A(int);
+    };
+
+    void target(bool b) {
+      A a = b ? A(0) : A(1);
+      (void)0; // [[p]]
+    }
+  )";
+  using ast_matchers::cxxConstructExpr;
+  using ast_matchers::equals;
+  using ast_matchers::hasArgument;
+  using ast_matchers::integerLiteral;
+  using ast_matchers::match;
+  using ast_matchers::selectFirst;
+  using ast_matchers::traverse;
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        auto *ConstructExpr0 = selectFirst<CXXConstructExpr>(
+            "construct",
+            match(cxxConstructExpr(hasArgument(0, integerLiteral(equals(0))))
+                      .bind("construct"),
+                  ASTCtx));
+        auto *ConstructExpr1 = selectFirst<CXXConstructExpr>(
+            "construct",
+            match(cxxConstructExpr(hasArgument(0, integerLiteral(equals(1))))
+                      .bind("construct"),
+                  ASTCtx));
+
+        auto &ALoc = getLocForDecl<StorageLocation>(ASTCtx, Env, "a");
+        EXPECT_EQ(&Env.getResultObjectLocation(*ConstructExpr0), &ALoc);
+        EXPECT_EQ(&Env.getResultObjectLocation(*ConstructExpr1), &ALoc);
+      });
+}
+
 TEST(TransferTest, StaticCast) {
   std::string Code = R"(
     void target(int Foo) {
@@ -5886,6 +5956,38 @@ TEST(TransferTest, ContextSensitiveReturnRecord) {
       {BuiltinOptions{ContextSensitiveOptions{}}});
 }
 
+TEST(TransferTest, ContextSensitiveReturnSelfReferentialRecord) {
+  std::string Code = R"(
+    struct S {
+      S() { self = this; }
+      S *self;
+    };
+
+    S makeS() {
+      // RVO guarantees that this will be constructed directly into `MyS`.
+      return S();
+    }
+
+    void target() {
+      S MyS = makeS();
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        auto &MySLoc = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "MyS");
+
+        auto *SelfVal =
+            cast<PointerValue>(getFieldValue(&MySLoc, "self", ASTCtx, Env));
+        EXPECT_EQ(&SelfVal->getPointeeLoc(), &MySLoc);
+      },
+      {BuiltinOptions{ContextSensitiveOptions{}}});
+}
+
 TEST(TransferTest, ContextSensitiveMethodLiteral) {
   std::string Code = R"(
     class MyClass {
@@ -6830,50 +6932,6 @@ TEST(TransferTest, LambdaCaptureThis) {
       });
 }
 
-TEST(TransferTest, DifferentReferenceLocInJoin) {
-  // This test triggers a case where the storage location for a reference-type
-  // variable is different for two states being joined. We used to believe this
-  // could not happen and therefore had an assertion disallowing this; this test
-  // exists to demonstrate that we can handle this condition without a failing
-  // assertion. See also the discussion here:
-  // https://discourse.llvm.org/t/70086/6
-  std::string Code = R"(
-    namespace std {
-      template <class T> struct initializer_list {
-        const T* begin();
-        const T* end();
-      };
-    }
-
-    void target(char* p, char* end) {
-      while (p != end) {
-        if (*p == ' ') {
-          p++;
-          continue;
-        }
-
-        auto && range = {1, 2};
-        for (auto b = range.begin(), e = range.end(); b != e; ++b) {
-        }
-        (void)0;
-        // [[p]]
-      }
-    }
-  )";
-  runDataflow(
-      Code,
-      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
-         ASTContext &ASTCtx) {
-        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
-
-        // Joining environments with different storage locations for the same
-        // declaration results in the declaration being removed from the joined
-        // environment.
-        const ValueDecl *VD = findValueDecl(ASTCtx, "range");
-        ASSERT_EQ(Env.getStorageLocation(*VD), nullptr);
-      });
-}
-
 // This test verifies correct modeling of a relational dependency that goes
 // through unmodeled functions (the simple `cond()` in this case).
 TEST(TransferTest, ConditionalRelation) {