[PATCH] D153409: [clang][dataflow] Treat fields of anonymous records as being part of their parent.

2023-06-27 Thread Martin Böhme via Phabricator via cfe-commits
mboehme marked 2 inline comments as done.
mboehme added a comment.

PTAL




Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:305
+if (Field->isAnonymousStructOrUnion())
+  getFieldsFromClassHierarchy(Field->getType(), Fields);
+else

gribozavr2 wrote:
> 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.
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.



Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:5427
+const ValueDecl *SDecl = findValueDecl(ASTCtx, "s");
+const ValueDecl *IDecl = findValueDecl(ASTCtx, "i");
+

gribozavr2 wrote:
> 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.
Good point, done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153409/new/

https://reviews.llvm.org/D153409

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D153409: [clang][dataflow] Treat fields of anonymous records as being part of their parent.

2023-06-27 Thread Martin Böhme via Phabricator via cfe-commits
mboehme updated this revision to Diff 534891.
mboehme added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153409/new/

https://reviews.llvm.org/D153409

Files:
  clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.h
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -5434,4 +5434,38 @@
   });
 }
 
+// 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> ,
+ ASTContext ) {
+const Environment  = getEnvironmentAtAnnotation(Results, "p");
+const ValueDecl *SDecl = findValueDecl(ASTCtx, "s");
+const ValueDecl *BDecl = findValueDecl(ASTCtx, "b");
+const IndirectFieldDecl *IndirectField =
+findIndirectFieldDecl(ASTCtx, "b");
+
+auto *S =
+cast(Env.getStorageLocation(*SDecl));
+auto  = cast(
+S->getChild(*cast(IndirectField->chain().front(;
+
+auto *B = cast(Env.getValue(AnonStruct.getChild(*BDecl)));
+ASSERT_TRUE(Env.flowConditionImplies(*B));
+  });
+}
+
 } // namespace
Index: clang/unittests/Analysis/FlowSensitive/TestingSupport.h
===
--- clang/unittests/Analysis/FlowSensitive/TestingSupport.h
+++ clang/unittests/Analysis/FlowSensitive/TestingSupport.h
@@ -428,6 +428,14 @@
 ///   `Name` must be unique in `ASTCtx`.
 const ValueDecl *findValueDecl(ASTContext , llvm::StringRef Name);
 
+/// Returns the `IndirectFieldDecl` for the given identifier.
+///
+/// Requirements:
+///
+///   `Name` must be unique in `ASTCtx`.
+const IndirectFieldDecl *findIndirectFieldDecl(ASTContext ,
+   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.
Index: clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
+++ clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
@@ -154,9 +154,19 @@
 }
 
 const ValueDecl *test::findValueDecl(ASTContext , 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("v", TargetNodes);
   assert(Result != nullptr);
   return Result;
 }
+
+const IndirectFieldDecl *test::findIndirectFieldDecl(ASTContext ,
+ llvm::StringRef Name) {
+  auto TargetNodes = match(indirectFieldDecl(hasName(Name)).bind("i"), ASTCtx);
+  assert(TargetNodes.size() == 1 && "Name must be unique");
+  const auto *Result = selectFirst("i", TargetNodes);
+  assert(Result != nullptr);
+  return Result;
+}


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -5434,4 +5434,38 @@
   });
 }
 
+// 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> ,
+ ASTContext ) {
+const Environment  = getEnvironmentAtAnnotation(Results, "p");
+const ValueDecl 

[PATCH] D153409: [clang][dataflow] Treat fields of anonymous records as being part of their parent.

2023-06-21 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision.
gribozavr2 added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:305
+if (Field->isAnonymousStructOrUnion())
+  getFieldsFromClassHierarchy(Field->getType(), Fields);
+else

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.



Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:5427
+const ValueDecl *SDecl = findValueDecl(ASTCtx, "s");
+const ValueDecl *IDecl = findValueDecl(ASTCtx, "i");
+

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153409/new/

https://reviews.llvm.org/D153409

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits