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

[ymandel]

Summary

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.

[xazax.hun]

Does this mean you want to use StructValues for non-struct types to have access to properties? I wonder if it would be a cleaner approach to be able to assign properties to any type of values instead of weakening the type system. E.g., properties could be a table on the side, a mapping from any kind of values to a set of properties.

[ymandel]

In D126973#3556312, @xazax.hun wrote:

Does this mean you want to use StructValues for non-struct types to have access to properties? I wonder if it would be a cleaner approach to be able to assign properties to any type of values instead of weakening the type system. E.g., properties could be a table on the side, a mapping from any kind of values to a set of properties.

Indeed! That would definitely be an improvement. This was motivated by actual crashes in the existing code which relied on this feature. It's fair to say that the assertion was trying to enforce reasonable behavior (just not *required*). We definitely we need a more general properties mechanism. I could put in a FIXME to put back the assert pending that change, but I'm generally hesitant about assertions that don't enforce necessary properties (only "nice"). Ideally, we'd have a way to log places (say, debug only) where we hit a surprising but not illegal condition.

[xazax.hun]

In D126973#3556383, @ymandel wrote:

I'm generally hesitant about assertions that don't enforce necessary properties (only "nice").

I think not enforcing this in the current model is OK. I am more concerned about the future if we plan to run multiple checks/modeling in the same fixed point iteration. One check might make the assumption that the types are matching up while the other can end up producing values where this is not the case.

[gribozavr2]

using the StructValue for its properties, while modeling a non-struct type.

Could you explain the use case in more details? Maybe a better change is to move synthetic properties to the base class Value?

[ymandel]

In D126973#3558304, @gribozavr2 wrote:

using the StructValue for its properties, while modeling a non-struct type.

Could you explain the use case in more details?

A StructValue is used to model a complex underlying type for access to its properties, but the corresponding AggregateStorageLocation is not initialized with a record type -- specifically, the real and complex type -- because that type is irrelevant to the model.

Maybe a better change is to move synthetic properties to the base class Value?

In part, indeed that's what Gabor suggested above. Please see my response earlier and let me know what you think.

In D126973#3556466, @xazax.hun wrote:

In D126973#3556383, @ymandel wrote:

I'm generally hesitant about assertions that don't enforce necessary properties (only "nice").

I think not enforcing this in the current model is OK. I am more concerned about the future if we plan to run multiple checks/modeling in the same fixed point iteration. One check might make the assumption that the types are matching up while the other can end up producing values where this is not the case.

Good point -- but I think this gets at a larger issue of how to handle multiple models touching the same type. Even if the types align, for example, the properties might not. So, we need an overall story.

[xazax.hun]

With properties moved up in the hierarchy this could be abandoned now, right?

[ymandel]

In D126973#3592537, @xazax.hun wrote:

With properties moved up in the hierarchy this could be abandoned now, right?

Yes, I think that would be best. Thanks for the reminder!

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

StorageLocation *Environment::getThisPointeeStorageLocation() const {
   return DACtx->getThisPointeeStorageLocation();
 }
 
 void Environment::setValue(const StorageLocation &Loc, Value &Val) {
   LocToVal[&Loc] = &Val;
 
   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);
       if (auto *FieldVal = StructVal->getChild(*Field))
         setValue(FieldLoc, *FieldVal);
     }
   }
 

clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp

 //===- unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp -------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #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"
 #include "gtest/gtest.h"
 #include <memory>
 
 namespace {
 
 using namespace clang;
 using namespace dataflow;
-using ::testing::ElementsAre;
-using ::testing::Pair;
+using ::testing::NotNull;
 
 class EnvironmentTest : public ::testing::Test {
   DataflowAnalysisContext Context;
 
 protected:
   EnvironmentTest()
       : Context(std::make_unique<WatchedLiteralsSolver>()), Env(Context) {}
 
TEST_F(EnvironmentTest, CreateValueRecursiveType) {
   StructValue *SVal = clang::dyn_cast<StructValue>(Val);
   ASSERT_NE(SVal, nullptr);
   Val = SVal->getChild(*R);
   ASSERT_NE(Val, nullptr);
   PointerValue *PV = clang::dyn_cast<PointerValue>(Val);
   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