[clang][dataflow] Add `refreshStructValue()`.

[mboehme]

Summary

Besides being a useful abstraction, this function will help insulate existing clients of the framework from upcoming changes to the API of StructValue and AggregateStorageLocation.

Depends On D155202

[xazax.hun]

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

704–709

I think this is fine for now, but I wonder if we should come up with an API where errors like this cannot happen. One potential way would be to no longer include these properties in the StructValues, but have a separate mapping StructValue => Properties. So, one can update the mapping in an environment without any unintended consequences in other environments.

[mboehme]

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

704–709

I think this is fine for now, but I wonder if we should come up with an API where errors like this cannot happen.

Thanks for bringing this up.

I agree that this needs more improvement in the future. The underlying issue is that StructValue isn't really a very useful concept.

Value works great for scalar values, where we do actually treat it as the immutable value that it's intended to be. Attaching properties to values makes a lot of sense in this context: If we're modelling a property of an immutable value, then that property is presumably itself immutable.

However, the Value concept has never worked well for structs / records because, when we mutate fields, we don't update the StructValue. We could try to change this, but I don't think this would be worth it because StructValue isn't a useful concept in C++ anyway. As the value categories RFC discusses, prvalues of class type are a very niche concept in C++. The only thing you can do with them is to initialize a result object -- you can't otherwise perform any operations on them (i.e. access member variables or call member functions). This has been part of the motivation for reducing their importance as part of the value categories work.

I think we should continue down that path and, ultimately, maybe eliminate StructValue entirely. So while introducing a mapping StructValue => Properties would be a good option, I think an even better one would be to start attaching properties to AggregateStorageLocations instead of StructValues. Because AggregateStorageLocations are in essence, mutable, the mapping of AggregateStorageLocations to property values would need to be reflected in the Environment in some form. I think a natural way to do this would be to model properties in a similar way to fields: An AggregateStorageLocation would have a mapping PropertyName => StorageLocation, and the values of the properties would then be tracked through the existing StorageLocation => Value mapping in the environment.

Benefits:

• As already noted, this makes properties on AggregateStorageLocations very similar to fields, which is what they are typically used to model
• If a particular analysis needs a storage location for a property, we already have one. For example, UncheckedOptionalAccessModel.cpp currently models the "value" property as a PointerValue so that it has a StorageLocation that it can use as the return value of optional::value(). Under the approach I'm proposing, the storage location would be available from the framework, and the model for a specific analysis wouldn't have to do anything extra.
• We don't need any additional logic for joins / widening; the existing join / widening logic for the StorageLocation => Value mapping would also handle properties.

So this is where I think we can go in the future, and I agree that refreshStructValue() should only be a stopgap as we evolve the framework.

[xazax.hun]

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

704–709

This sounds great, thanks!

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

public:
   /// `PointeeType` that are canonically equivalent will return the same result.
   PointerValue &getOrCreateNullPointerValue(QualType PointeeType);
 
   /// Creates a value appropriate for `Type`, if `Type` is supported, otherwise
   /// return null. If `Type` is a pointer or reference type, creates all the
   /// necessary storage locations and values for indirections until it finds a
   /// non-pointer/non-reference type.
   ///
+  /// If `Type` is one of the following types, this function will always return
+  /// a non-null pointer:
+  /// - `bool`
+  /// - Any integer type
+  /// - Any class, struct, or union type
+  ///
   /// Requirements:
   ///
   ///  `Type` must not be null.
   Value *createValue(QualType Type);
 
   /// Creates an object (i.e. a storage location with an associated value) of
   /// type `Ty`. If `InitExpr` is non-null and has a value associated with it,
   /// initializes the object with this value. Otherwise, initializes the object

 /// member expression was written using `->`.
 AggregateStorageLocation *getBaseObjectLocation(const MemberExpr &ME,
                                                 const Environment &Env);
 
 /// Returns the fields of `RD` that are initialized by an `InitListExpr`, in the
 /// order in which they appear in `InitListExpr::inits()`.
 std::vector<FieldDecl *> getFieldsForInitListExpr(const RecordDecl *RD);
 
+/// Associates a new `StructValue` with `Loc` and returns the new value.
+/// It is not defined whether the field values remain the same or not.
+///
+/// This function is primarily intended for use by checks that set custom
+/// properties on `StructValue`s to model the state of these values. Such checks
+/// should avoid modifying the properties of an existing `StructValue` because
+/// these changes would be visible to other `Environment`s that share the same
+/// `StructValue`. Instead, call `refreshStructValue()`, then set the properties
+/// on the new `StructValue` that it returns. Typical usage:
+///
+///   refreshStructValue(Loc, Env).setProperty("my_prop", MyPropValue);
+StructValue &refreshStructValue(AggregateStorageLocation &Loc,
+                                Environment &Env);
+
+/// Associates a new `StructValue` with `Expr` and returns the new value.
+/// See also documentation for the overload above.
+StructValue &refreshStructValue(const Expr &Expr, Environment &Env);
+
 } // namespace dataflow
 } // namespace clang
 
 #endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_DATAFLOWENVIRONMENT_H

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

std::vector<FieldDecl *> getFieldsForInitListExpr(const RecordDecl *RD) {
   // fields to avoid mapping inits to the wrongs fields.
   std::vector<FieldDecl *> Fields;
   llvm::copy_if(
       RD->fields(), std::back_inserter(Fields),
       [](const FieldDecl *Field) { return !Field->isUnnamedBitfield(); });
   return Fields;
 }
 
+StructValue &refreshStructValue(AggregateStorageLocation &Loc,
+                                Environment &Env) {
+  auto &NewVal = *cast<StructValue>(Env.createValue(Loc.getType()));
+  Env.setValue(Loc, NewVal);
+  return NewVal;
+}
+
+StructValue &refreshStructValue(const Expr &Expr, Environment &Env) {
+  assert(Expr.getType()->isRecordType());
+
+  auto &NewVal = *cast<StructValue>(Env.createValue(Expr.getType()));
+
+  if (Expr.isPRValue()) {
+    Env.setValueStrict(Expr, NewVal);
+  } else {
+    StorageLocation *Loc = Env.getStorageLocationStrict(Expr);
+    if (Loc == nullptr) {
+      Loc = &Env.createStorageLocation(Expr);
+    }
+    Env.setValue(*Loc, NewVal);
+  }
+
+  return NewVal;
+}
+
 } // namespace dataflow
 } // namespace clang

clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

std::string Code = R"(
     }
   )";
   runDataflow(Code, UnorderedElementsAre(IsStringMapEntry(
                         "p", HoldsFunctionCallLattice(HasCalledFunctions(
                                  UnorderedElementsAre("baz", "foo"))))));
   // FIXME: Called functions at point `p` should contain only "foo".
 }
 
-StructValue &createNewStructValue(AggregateStorageLocation &Loc,
-                                  Environment &Env) {
-  auto &Val = *cast<StructValue>(Env.createValue(Loc.getType()));
-  Env.setValue(Loc, Val);
-  return Val;
-}
-
 // Models an analysis that uses flow conditions.
 class SpecialBoolAnalysis final
     : public DataflowAnalysis<SpecialBoolAnalysis, NoopLattice> {
 public:
   explicit SpecialBoolAnalysis(ASTContext &Context)
       : DataflowAnalysis<SpecialBoolAnalysis, NoopLattice>(Context) {}
 
   static NoopLattice initialElement() { return {}; }
void transfer(const CFGElement &Elt, NoopLattice &, Environment &Env) {
     } else if (const auto *E = selectFirst<CXXMemberCallExpr>(
                    "call", match(cxxMemberCallExpr(callee(cxxMethodDecl(ofClass(
                                                        SpecialBoolRecordDecl))))
                                      .bind("call"),
                                  *S, getASTContext()))) {
       auto &ObjectLoc =
           *cast<AggregateStorageLocation>(getImplicitObjectLocation(*E, Env));
 
-      createNewStructValue(ObjectLoc, Env)
+      refreshStructValue(ObjectLoc, Env)
           .setProperty("is_set", Env.getBoolLiteralValue(true));
     }
   }
 
   ComparisonResult compare(QualType Type, const Value &Val1,
                            const Environment &Env1, const Value &Val2,
                            const Environment &Env2) override {
     const auto *Decl = Type->getAsCXXRecordDecl();
if (const auto *E = selectFirst<CXXConstructExpr>(
       cast<StructValue>(Env.getValueStrict(*E))
           ->setProperty("has_value", Env.getBoolLiteralValue(false));
     } else if (const auto *E =
                    selectFirst<CXXOperatorCallExpr>("operator", Matches)) {
       assert(E->getNumArgs() > 0);
       auto *Object = E->getArg(0);
       assert(Object != nullptr);
 
-      auto &ObjectLoc = *cast<AggregateStorageLocation>(
-          Env.getStorageLocation(*Object, SkipPast::Reference));
-
-      createNewStructValue(ObjectLoc, Env)
+      refreshStructValue(*Object, Env)
           .setProperty("has_value", Env.getBoolLiteralValue(true));
     }
   }
 
   ComparisonResult compare(QualType Type, const Value &Val1,
                            const Environment &Env1, const Value &Val2,
                            const Environment &Env2) override {
     // Nothing to say about a value that does not model an `OptionalInt`.