[clang][dataflow] Add a test that we can access fields of anonymous records.

[sammccall]

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

5464

assert that the result is non-null?

[mboehme]

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

5464

getChild() returns a reference, so the result can't be null. It asserts internally that the child exists, so if the child doesn't exist, we'll get an assertion failure.

[gribozavr2]

clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp

305 ↗

(On Diff #533212)

Could we somehow take advantage of the IndirectFieldDecl instead of recursing ourselves? Seems like that way we would be delegating to Clang the questions of the name injection/lookup.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

5458

Could we make some stronger assertion to prove that the transfer function works? It seems to me that getChild() by itself does not prove that.

For example, store and load the value and assert that it is the same.

[mboehme]

Undo all of the non-test changes. This patch now only introduces a test that
we can access fields of anonymous structs.

I realized based on gribozavr2's comment that we shouldn't be lumping all fields
of anonymous records into the parent record, as this goes against the grain of
the Clang AST and, for example, requires special-case code in MemberExpr.

Instead, I realized that everything essentially already works -- anonymous
records were already being correctly added as fields of their parent record.
The only thing that needs to be fixed is handling IndirectFieldDecl correctly
when transferring CXXCtorInitializer. This will come in a followup patch.

[mboehme]

PTAL

clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp

305 ↗

(On Diff #533212)

Thanks for pointing this out.

I realized I was really working against the grain of the Clang AST. I've changed this patch so that it merely adds a new test (to demonstrate that we are already representing fields of anonymous records correctly).

The thing that actually needs to be fixed is the handling of CXXCtorInitializer; I'll do this in a followup patch.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

5458

Good point, done.

[mboehme]

Pre-merge failure looks unrelated

clang/unittests/Analysis/FlowSensitive/TestingSupport.h

 
 /// Returns the `ValueDecl` for the given identifier.
 ///
 /// Requirements:
 ///
 ///   `Name` must be unique in `ASTCtx`.
 const ValueDecl *findValueDecl(ASTContext &ASTCtx, llvm::StringRef Name);
 
+/// Returns the `IndirectFieldDecl` for the given identifier.
+///
+/// Requirements:
+///
+///   `Name` must be unique in `ASTCtx`.
+const IndirectFieldDecl *findIndirectFieldDecl(ASTContext &ASTCtx,
+                                               llvm::StringRef Name);
+
 /// Returns the storage location (of type `LocT`) for the given identifier.
 /// `LocT` must be a subclass of `StorageLocation` and must be of the
 /// appropriate type.
 ///
 /// Requirements:
 ///
 ///   `Name` must be unique in `ASTCtx`.
 template <class LocT>

clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp

return llvm::createStringError(
             .printToString(SourceManager)
             .data());
   }
 
   return Result;
 }
 
 const ValueDecl *test::findValueDecl(ASTContext &ASTCtx, llvm::StringRef Name) {
-  auto TargetNodes = match(valueDecl(hasName(Name)).bind("v"), ASTCtx);
+  auto TargetNodes = match(
+      valueDecl(unless(indirectFieldDecl()), hasName(Name)).bind("v"), ASTCtx);
   assert(TargetNodes.size() == 1 && "Name must be unique");
   auto *const Result = selectFirst<ValueDecl>("v", TargetNodes);
   assert(Result != nullptr);
   return Result;
 }
+
+const IndirectFieldDecl *test::findIndirectFieldDecl(ASTContext &ASTCtx,
+                                                     llvm::StringRef Name) {
+  auto TargetNodes = match(indirectFieldDecl(hasName(Name)).bind("i"), ASTCtx);
+  assert(TargetNodes.size() == 1 && "Name must be unique");
+  const auto *Result = selectFirst<IndirectFieldDecl>("i", TargetNodes);
+  assert(Result != nullptr);
+  return Result;
+}

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

runDataflow(
                                .bind("implicit_cast")),
                   ASTCtx));
 
         ASSERT_THAT(ImplicitCast, NotNull());
         EXPECT_THAT(Env.getValueStrict(*ImplicitCast), NotNull());
       });
 }
 
+// Check that fields of anonymous records are modeled.
+TEST(TransferTest, AnonymousStruct) {
+  std::string Code = R"(
+    struct S {
+      struct {
+        bool b;
+      };
+    };
+    void target() {
+      S s;
+      s.b = true;
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+        const ValueDecl *SDecl = findValueDecl(ASTCtx, "s");
+        const ValueDecl *BDecl = findValueDecl(ASTCtx, "b");
+        const IndirectFieldDecl *IndirectField =
+            findIndirectFieldDecl(ASTCtx, "b");
+
+        auto *S =
+            cast<AggregateStorageLocation>(Env.getStorageLocation(*SDecl));
+        auto &AnonStruct = cast<AggregateStorageLocation>(
+            S->getChild(*cast<ValueDecl>(IndirectField->chain().front())));
+
+        auto *B = cast<BoolValue>(Env.getValue(AnonStruct.getChild(*BDecl)));
+        ASSERT_TRUE(Env.flowConditionImplies(*B));
+      });
+}
+
 } // namespace