mboehme created this revision.
Herald added subscribers: martong, xazax.hun.
Herald added a reviewer: NoQ.
Herald added a project: All.
mboehme requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
While working on the ongoing migration to strict handling of value
categories (see https://discourse.llvm.org/t/70086), I ran into issues related
to losing the value associated with an optional.
This issue is hinted at in the existing comments, but the issue didn't become
sufficiently clear to me from those, so I thought it would be worth capturing
more details, along with ideas for how this issue might be fixed.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D152369
Files:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -315,11 +315,16 @@
auto &ValueLoc = ValuePtr->getPointeeLoc();
if (Env.getValue(ValueLoc) == nullptr) {
// 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.
+ // happen in various situations, for example:
+ // - Because of an environment merge (where the two environments mapped
+ // the property to different values, which resulted in them both being
+ // discarded).
+ // - When two blocks in the CFG, with neither a dominator of the other,
+ // visit the same optional value. (FIXME: This is something we can and
+ // should fix -- see also the lengthy FIXME below.)
+ // - Or even when a block is revisited during testing to collect
+ // per-statement state.
+ //
// FIXME: This situation means that the optional contents are not shared
// between branches and the like. Practically, this lack of sharing
// reduces the precision of the model when the contents are relevant to
@@ -340,6 +345,51 @@
auto &ValueLoc = Env.createStorageLocation(Ty);
Env.setValue(ValueLoc, *ValueVal);
auto &ValuePtr = Env.create<PointerValue>(ValueLoc);
+ // FIXME:
+ // The change we make to the `value` property below may become visible to
+ // other blocks that aren't successors of the current block and therefore
+ // don't see the change we made above mapping `ValueLoc` to `ValueVal`. For
+ // example:
+ //
+ // void target(optional<int> oo, bool b) {
+ // // `oo` is associated with a `StructValue` here, which we will call
+ // // `OptionalVal`.
+ //
+ // // The `has_value` property is set on `OptionalVal` (but not the
+ // // `value` property yet).
+ // if (!oo.has_value()) return;
+ //
+ // if (b) {
+ // // Let's assume we transfer the `if` branch first.
+ // //
+ // // This causes us to call `maybeInitializeOptionalValueMember()`,
+ // // which causes us to set the `value` property on `OptionalVal`
+ // // (which had not been set until this point). This `value` property
+ // // refers to a `PointerValue`, which in turn refers to a
+ // // StorageLocation` that is associated to an `IntegerValue`.
+ // oo.value();
+ // } else {
+ // // Let's assume we transfer the `else` branch after the `if` branch.
+ // //
+ // // We see the `value` property that the `if` branch set on
+ // // `OptionalVal`, but in the environment for this block, the
+ // // `StorageLocation` in the `PointerValue` is not associated with
any
+ // // `Value`.
+ // oo.value();
+ // }
+ // }
+ //
+ // This situation is currently "saved" by the code above that checks whether
+ // the `value` property is already set, and if, the `ValueLoc` is not
+ // associated with a `ValueVal`, creates a new `ValueVal`.
+ //
+ // However, what we should really do is to make sure that the change to the
+ // `value` property does not "leak" to other blocks that are not successors
+ // of this block. To do this, instead of simply setting the `value` property
+ // on the existing `OptionalVal`, we should create a new `Value` for the
+ // optional, set the property on that, and associate the storage location
that
+ // is currently associated with the existing `OptionalVal` with the newly
+ // created `Value` instead.
OptionalVal.setProperty("value", ValuePtr);
return &ValueLoc;
}
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -315,11 +315,16 @@
auto &ValueLoc = ValuePtr->getPointeeLoc();
if (Env.getValue(ValueLoc) == nullptr) {
// 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.
+ // happen in various situations, for example:
+ // - Because of an environment merge (where the two environments mapped
+ // the property to different values, which resulted in them both being
+ // discarded).
+ // - When two blocks in the CFG, with neither a dominator of the other,
+ // visit the same optional value. (FIXME: This is something we can and
+ // should fix -- see also the lengthy FIXME below.)
+ // - Or even when a block is revisited during testing to collect
+ // per-statement state.
+ //
// FIXME: This situation means that the optional contents are not shared
// between branches and the like. Practically, this lack of sharing
// reduces the precision of the model when the contents are relevant to
@@ -340,6 +345,51 @@
auto &ValueLoc = Env.createStorageLocation(Ty);
Env.setValue(ValueLoc, *ValueVal);
auto &ValuePtr = Env.create<PointerValue>(ValueLoc);
+ // FIXME:
+ // The change we make to the `value` property below may become visible to
+ // other blocks that aren't successors of the current block and therefore
+ // don't see the change we made above mapping `ValueLoc` to `ValueVal`. For
+ // example:
+ //
+ // void target(optional<int> oo, bool b) {
+ // // `oo` is associated with a `StructValue` here, which we will call
+ // // `OptionalVal`.
+ //
+ // // The `has_value` property is set on `OptionalVal` (but not the
+ // // `value` property yet).
+ // if (!oo.has_value()) return;
+ //
+ // if (b) {
+ // // Let's assume we transfer the `if` branch first.
+ // //
+ // // This causes us to call `maybeInitializeOptionalValueMember()`,
+ // // which causes us to set the `value` property on `OptionalVal`
+ // // (which had not been set until this point). This `value` property
+ // // refers to a `PointerValue`, which in turn refers to a
+ // // StorageLocation` that is associated to an `IntegerValue`.
+ // oo.value();
+ // } else {
+ // // Let's assume we transfer the `else` branch after the `if` branch.
+ // //
+ // // We see the `value` property that the `if` branch set on
+ // // `OptionalVal`, but in the environment for this block, the
+ // // `StorageLocation` in the `PointerValue` is not associated with any
+ // // `Value`.
+ // oo.value();
+ // }
+ // }
+ //
+ // This situation is currently "saved" by the code above that checks whether
+ // the `value` property is already set, and if, the `ValueLoc` is not
+ // associated with a `ValueVal`, creates a new `ValueVal`.
+ //
+ // However, what we should really do is to make sure that the change to the
+ // `value` property does not "leak" to other blocks that are not successors
+ // of this block. To do this, instead of simply setting the `value` property
+ // on the existing `OptionalVal`, we should create a new `Value` for the
+ // optional, set the property on that, and associate the storage location that
+ // is currently associated with the existing `OptionalVal` with the newly
+ // created `Value` instead.
OptionalVal.setProperty("value", ValuePtr);
return &ValueLoc;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
