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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
