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

[sammccall]

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

5433

assert that the result is non-null?

[mboehme]

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

5433

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

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

5427

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

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

5427

Good point, done.

[mboehme]

Pre-merge failure looks unrelated

clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp

 // field decl will be modeled for all instances of the inherited field.
 static void
 getFieldsFromClassHierarchy(QualType Type,
                             llvm::DenseSet<const FieldDecl *> &Fields) {
   if (Type->isIncompleteType() || Type->isDependentType() ||
       !Type->isRecordType())
     return;
 
-  for (const FieldDecl *Field : Type->getAsRecordDecl()->fields())
+  for (const FieldDecl *Field : Type->getAsRecordDecl()->fields()) {
+    if (Field->isAnonymousStructOrUnion())
+      getFieldsFromClassHierarchy(Field->getType(), Fields);
+    else
       Fields.insert(Field);
+  }
   if (auto *CXXRecord = Type->getAsCXXRecordDecl())
     for (const CXXBaseSpecifier &Base : CXXRecord->bases())
       getFieldsFromClassHierarchy(Base.getType(), Fields);
 }
 
 /// Gets the set of all fields in the type.
 llvm::DenseSet<const FieldDecl *>
 clang::dataflow::getObjectFields(QualType Type) {
   llvm::DenseSet<const FieldDecl *> Fields;
   getFieldsFromClassHierarchy(Type, Fields);
   return Fields;
 }

clang/lib/Analysis/FlowSensitive/Transfer.cpp

if (auto *D = dyn_cast<VarDecl>(Member)) {
         return;
       }
     }
 
     AggregateStorageLocation *BaseLoc = getBaseObjectLocation(*S, Env);
     if (BaseLoc == nullptr)
       return;
 
+    // Fields on anonymous records are treated as being part of the enclosing
+    // records. So if we see a member access for an anonymous record, just pass
+    // through the storage location of the base object.
+    if (auto *Field = dyn_cast<FieldDecl>(Member)) {
+      if (Field->isAnonymousStructOrUnion()) {
+        Env.setStorageLocation(*S, *BaseLoc);
+        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).

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;
 }

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 and treated as children
+// of the enclosing record.
+TEST(TransferTest, AnonymousStruct) {
+  std::string Code = R"(
+    struct S {
+      struct {
+        int i;
+      };
+    };
+    void target() {
+      S s;
+      (void)s.i;
+      // [[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 *IDecl = findValueDecl(ASTCtx, "i");
+
+        auto *S =
+            cast<AggregateStorageLocation>(Env.getStorageLocation(*SDecl));
+        // If we can call getChild() without an assertion failure, it means
+        // that `i` is modeled.
+        S->getChild(*IDecl);
+      });
+}
+
 } // namespace