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

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 16 inline comments as done.
mboehme added inline comments.


================
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.
+  ///
----------------
ymandel wrote:
> 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.
Reworded.

I've tried to avoid referring to Value.h because `StructValue` itself may be going away entirely in the not-too-distant future. Instead, I've adapted some material from there (which also means people don't need to cross-reference Value.h).

WDYT?


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


================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:97-99
+    for (auto [Field, Loc] : Children) {
+      assert(Field->getType()->isReferenceType() || Loc != nullptr);
+    }
----------------
ymandel wrote:
> 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).
Hm -- I'm not sure. Changed to a lambda.


================
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.
+  ///
----------------
ymandel wrote:
> which constructor? Also, is this clause clarifying the "other fields cannot change" part of the sentence?
Reworded. Is this clearer now?


================
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
----------------
ymandel wrote:
> 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.
Yes, this looks right. Added this as an example below.


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


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


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

The intent here was to check a _pre_condition of the function (i.e. that the caller must already have associated `StructVal->getAggregateLoc()` with `Val` in the environment before calling this function) rather than a _post_condition of the function. But looking at this again, this doesn't seem like a good contract. All current callers of the function satisfy this precondition, but it's not documented anywhere, and it's also just simply a strange and unnecessary precondition. So instead, I've simply replaced this with a `setValue()` that makes sure this is the case if it wasn't before.


================
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);
----------------
ymandel wrote:
> how does this interact with the fact that multiple StructValues can share the same ASL?
Not sure exactly what you're asking?

First of all, I think the wording on the documentation for `StructValue` may have been confusing, so I've reworded that to make it explicit that when I say that multiple `StructValue`s can share the same `AggregateStorageLocation`, what I mean, more precisely, is that the same `AggregateStorageLocation` can be associated with different `StructValue`s in different environments.

Here, however, we've only just created the `AggregateStorageLocation` (and a `StructValue` to go with it), so it definitely makes sense to associate the two through a call to `setValue(Loc, StructVal)`.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:817
 
+StorageLocation &
+Environment::createFieldOrPointeeLoc(QualType Ty,
----------------
ymandel wrote:
> 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.
Yes, the name was bad. Renamed and added a comment in the header.


================
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);
----------------
ymandel wrote:
> What happens to `ValueVal` if we use `createObject`?
Oops -- this was left over from refactoring and was superfluous now. Thanks for catching. Removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155446