[PATCH] D126973: [clang][dataflow] Relax assumption that `AggregateStorageLocations` correspond to struct type.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added reviewers: xazax.hun, sgatev.
Herald added subscribers: tschuett, steakhal, rnkovacs.
Herald added a project: All.
ymandel requested review of this revision.
Herald added a project: clang.

Currently, when an `AggregateStorageLocation` is mapped to a `StructValue`, we
enforce that it's type is a struct or class. However, we don't need this to hold
and there are reasonable violations of this assumption -- for example, using the
`StructValue` for its properties, while modeling a non-struct type.

This patch removes the assertion.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126973

Files:
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp


Index: clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
@@ -10,6 +10,7 @@
 #include "NoopAnalysis.h"
 #include "TestingSupport.h"
 #include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "clang/Analysis/FlowSensitive/WatchedLiteralsSolver.h"
 #include "gmock/gmock.h"
@@ -20,8 +21,7 @@
 
 using namespace clang;
 using namespace dataflow;
-using ::testing::ElementsAre;
-using ::testing::Pair;
+using ::testing::NotNull;
 
 class EnvironmentTest : public ::testing::Test {
   DataflowAnalysisContext Context;
@@ -98,4 +98,25 @@
   EXPECT_NE(PV, nullptr);
 }
 
+TEST_F(EnvironmentTest, SetValueAdmitsNonStructAggregateStorageLocations) {
+  using namespace ast_matchers;
+
+  std::string Code = "bool X;";
+  auto Unit =
+      tooling::buildASTFromCodeWithArgs(Code, {"-fsyntax-only", "-std=c++11"});
+  auto &Context = Unit->getASTContext();
+
+  ASSERT_EQ(Context.getDiagnostics().getClient()->getNumErrors(), 0U);
+
+  const auto *Ty = selectFirst<QualType>(
+      "target", match(qualType(builtinType()).bind("target"), Context));
+  ASSERT_TRUE(Ty != nullptr && !Ty->isNull());
+
+  // Create an `AggregateStorageLocation` that is *not* backed by a struct.
+  auto &Loc =
+      Env.takeOwnership(std::make_unique<AggregateStorageLocation>(*Ty));
+  auto &V = Env.takeOwnership(std::make_unique<StructValue>());
+  Env.setValue(Loc, V);
+  EXPECT_EQ(Env.getValue(Loc), &V);
+}
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -374,11 +374,7 @@
 
   if (auto *StructVal = dyn_cast<StructValue>(&Val)) {
     auto &AggregateLoc = *cast<AggregateStorageLocation>(&Loc);
-
-    const QualType Type = AggregateLoc.getType();
-    assert(Type->isStructureOrClassType());
-
-    for (const FieldDecl *Field : getObjectFields(Type)) {
+    for (const FieldDecl *Field : getObjectFields(AggregateLoc.getType())) {
       assert(Field != nullptr);
       StorageLocation &FieldLoc = AggregateLoc.getChild(*Field);
       MemberLocToStruct[&FieldLoc] = std::make_pair(StructVal, Field);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D126973.434054.patch
Type: text/x-patch
Size: 2562 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220603/61cbe74b/attachment-0001.bin>