[PATCH] D155446: [clang][dataflow] Eliminate duplication between `AggregateStorageLocation` and `StructValue`.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

I got up to Transfer.cpp and figured I should send along what I have. Looks very good so far! I should have the rest of the review in a few hours...



================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:337-338
 
+  /// Returns the location of the result object that a prvalue `E` of record
+  /// type initializes.
+  ///
----------------
I found this sentence hard to parse. Lots of "of..." chained. I think it might be clearer with a bit of a preamble about record prvalues. Or, refer the reader to Value.h for details? Once I read that, this became a lot clearer.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:340
+  ///
+  /// TODO: Currently, this simply returns a stable storage location for `E`,
+  /// but this doesn't do the right thing in scenarios like the following:
----------------



================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:353
+  ///  `E` must be a prvalue of record type.
+  AggregateStorageLocation &getResultObjectLocation(const Expr &E);
+
----------------
Given that this is specific to record types, and that's not reflected in the argument type, should the name somehow reflect that?


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:406
   /// 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
+  /// return null.
+  ///
----------------



================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:97-99
+    for (auto [Field, Loc] : Children) {
+      assert(Field->getType()->isReferenceType() || Loc != nullptr);
+    }
----------------
When assertions are off this will be an odd for loop with no body. Will the optimizer delete it? If not, then consider putting the loop in a lambda inside the assert (or somesuch).


================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:132
+  /// All other fields cannot change their storage location; the final value of
+  /// the storage location must be initialized using the constructor.
+  ///
----------------
which constructor? Also, is this clause clarifying the "other fields cannot change" part of the sentence?


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:202-223
+/// In C++, prvalues of class type serve only a limited purpose: They can only
+/// be used to initialize a result object. It is not possible to access member
+/// variables or call member functions on a prvalue of class type.
+/// Correspondingly, `StructValue` also serves only two limited purposes:
+/// - It conveys a prvalue of class type from the place where the object is
+///   constructed to the result object that it initializes.
+///   When creating a prvalue of class type, we already need a storage location
----------------
These comments are fantastic!


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:206-207
+/// Correspondingly, `StructValue` also serves only two limited purposes:
+/// - It conveys a prvalue of class type from the place where the object is
+///   constructed to the result object that it initializes.
+///   When creating a prvalue of class type, we already need a storage location
----------------
nit: consider adding a simple example. Is this one right?
`MyStruct S = MyStruct(3);`
The intiializer is a prvalue which will result in a StructValue wrapping an ASL. Then, we'll model the decl `S` using that ASL.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:127
+
+    assert(&StructVal1->getAggregateLoc() == &StructVal2->getAggregateLoc());
+
----------------
Please add comment explaining why we can assume this invariant.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:129
+
+    MergedVal = &MergedEnv.create<StructValue>(StructVal1->getAggregateLoc());
+  } else {
----------------
Why do we need a new StructValue rather than reusing one of the existing ones?


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:682-684
   if (auto *StructVal = dyn_cast<StructValue>(&Val)) {
-    auto &AggregateLoc = *cast<AggregateStorageLocation>(&Loc);
-
-    const QualType Type = AggregateLoc.getType();
-    assert(Type->isRecordType());
-
-    for (const FieldDecl *Field : DACtx->getModeledFields(Type)) {
-      assert(Field != nullptr);
-      StorageLocation &FieldLoc = AggregateLoc.getChild(*Field);
-      MemberLocToStruct[&FieldLoc] = std::make_pair(StructVal, Field);
-      if (auto *FieldVal = StructVal->getChild(*Field))
-        setValue(FieldLoc, *FieldVal);
-    }
+    assert(&StructVal->getAggregateLoc() == &Loc);
   }
----------------
Maybe just an assert?
```
assert(!isa<StructValue>(&Val) || &cast<StructValue>(&Val)->getAggregateLoc() == &Loc);


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:690
 void Environment::clearValue(const StorageLocation &Loc) {
   LocToVal.erase(&Loc);
 }
----------------
is it worth exposing this in the header (inline) now that it's just one line?


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:704
+      setStorageLocation(E, StructVal->getAggregateLoc());
+    assert(getValue(StructVal->getAggregateLoc()) == &Val);
+    return;
----------------
How does this work, given that you never called `setValue`?


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:807-809
+    // As we already have a storage location for the `StructValue`, we can and
+    // should associate them in the environment.
+    setValue(Loc, StructVal);
----------------
how does this interact with the fact that multiple StructValues can share the same ASL?


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:817
 
+StorageLocation &
+Environment::createFieldOrPointeeLoc(QualType Ty,
----------------
Consider adding a comment for this given that there's none in the header.  the name is a bit confusing on its own since the function also creates a corresponding value, not just the loc.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:353
     return nullptr;
-  auto &ValueLoc = Env.createStorageLocation(Ty);
-  Env.setValue(ValueLoc, *ValueVal);
+  auto &ValueLoc = Env.createObject(Ty);
   auto &ValuePtr = Env.create<PointerValue>(ValueLoc);
----------------
What happens to `ValueVal` if we use `createObject`?


================
Comment at: clang/lib/Analysis/FlowSensitive/RecordOps.cpp:91-92
+        return false;
     } else {
-      if (Env1.getValue(*FieldLoc1) != Env2.getValue(FieldLoc2))
+      if (Env1.getValue(*FieldLoc1) != Env2.getValue(*FieldLoc2))
         return false;
----------------
nit: use `else if` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155446