[PATCH] D124932: [clang][dataflow] Track `optional` contents in `optional` model.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 8 inline comments as done.
ymandel added a comment.

I'm having trouble with arcanist -- `arc diff` is crashing, but wanted to respond in the meantime to your questions. I hope I'll be able to actually upload the new diff soon...



================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:170
 /// Returns the symbolic value that represents the "has_value" property of the
-/// optional value `Val`. Returns null if `Val` is null.
-BoolValue *getHasValue(Value *Val) {
+/// optional value `Val`, creating a fresh one if none is set. Returns null if
+/// `Val` is null.
----------------
sgatev wrote:
> Why is this necessary? Do we really need to create a new value on each `getHasValue` call? Can we centralize initialization to avoid this? I'm worried that "has_value" and "value" initialization isn't coordinated.
> Why is this necessary? Do we really need to create a new value on each `getHasValue` call? Can we centralize initialization to avoid this? I'm worried that "has_value" and "value" initialization isn't coordinated.

We don't create a new one on each call, only when none is already populated. As for centralizing -- we could, but I'd like to move to lazy initialization anyhow, so it seemed less than ideal to put effort into centralizing at this point.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:222-224
+  // The "value" property represents a synthetic field. As such, it needs
+  // `StorageLocation`, like normal fields (and other variables). So, we model
+  // it with a `ReferenceValue`, since that includes a storage location.  Once
----------------
sgatev wrote:
> It's unclear to me why we "need" a `StorageLocation`. Representing the value as a `ReferenceValue` seems good to me, but there are other options. What issues are we running into if we remove the indirection?
> It's unclear to me why we "need" a `StorageLocation`. Representing the value as a `ReferenceValue` seems good to me, but there are other options. What issues are we running into if we remove the indirection?

We can't address it. Compare with a normal field, where the corresponding `AggregateStorageLocation` has a child that maps to a storage location for the field.  We don't support properties on `AggregateStorageLocation`, so the location has to exist somewhere.

What other options do you see?


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:231-236
+      // The property was previously set, but the value has been lost. This can
+      // happen, for example, because of an environment merge (where the two
+      // environments mapped the property to different values, which resulted in
+      // them both being discarded), or when two blocks in the CFG, with neither
+      // a dominator of the other, visit the same optional value, or even when a
+      // block is revisited during testing to collect per-statement state.
----------------
sgatev wrote:
> So we don't lose the `ReferenceValue`, but we do lose its pointee? Is that the behavior we want when merging?
> So we don't lose the `ReferenceValue`, but we do lose its pointee? Is that the behavior we want when merging?

Correct. No, that's not the behavior we want, hence the FIXME that follows. :)  But, it at least accounts for the other two situations, so its a win overall.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:281
+
+    auto *Prop = OptionalVal->getProperty("has_value");
+    if (auto *HasValueVal = cast_or_null<BoolValue>(Prop)) {
----------------
sgatev wrote:
> Shouldn't we use `getHasValue` here?
>Shouldn't we use getHasValue here?

Not quite:

1. its a bit a redundant since we already need to check whether the value is a `StructValue`, line 274-5. I tried to keep it to where its use would not cause unnecessary additional checks. 
2. We don't want to create a new "has_value" here, given that the access is illegal in that case. We want the condition to fail in that case (line 282) and result in recording an error.


================
Comment at: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:2188-2260
+  // `reset` changes the state.
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    struct Foo {
+      $ns::$optional<std::string> opt;
----------------
sgatev wrote:
> Do we really need tests for all members that check/access/reset the content? These are already covered in tests above.
Great point. I rethought the tests in general -- I'd mostly been copying them over, without carefully rethinking them. I've reduced the number of tests and added comments explaining their purposes. Essentially, we need to test that the synthetic value can be used like a normal field. 


================
Comment at: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:2280
+    void target() {
+      Bar bar = createBar();
+      bar.member->opt = "a";
----------------
sgatev wrote:
> 
No longer relevant.


================
Comment at: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:2287
+
+  // Resetting the nested optional.
+  ExpectLatticeChecksFor(
----------------
sgatev wrote:
> Do we really need this one or is it already covered by one of the tests in this file?
Covered.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124932