ymandel updated this revision to Diff 429210.
ymandel marked 6 inline comments as done.
ymandel added a comment.
address reviewer comments and fix bug in how the nested value as _re_created --
now uses the loc's type rather than the expression's type (which doesn't always
match exactly).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124932/new/
https://reviews.llvm.org/D124932
Files:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2150,8 +2150,95 @@
UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe")));
}
+TEST_P(UncheckedOptionalAccessTest, OptionalValueOptional) {
+ // Basic test that nested values are populated. We nest an optional because
+ // its easy to use in a test, but the type of the nested value shouldn't
+ // matter.
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ using Foo = $ns::$optional<std::string>;
+
+ void target($ns::$optional<Foo> foo) {
+ if (foo && *foo) {
+ foo->value();
+ /*[[access]]*/
+ }
+ }
+ )",
+ UnorderedElementsAre(Pair("access", "safe")));
+
+ // Mutation is supported for nested values.
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ using Foo = $ns::$optional<std::string>;
+
+ void target($ns::$optional<Foo> foo) {
+ if (foo && *foo) {
+ foo->reset();
+ foo->value();
+ /*[[reset]]*/
+ }
+ }
+ )",
+ UnorderedElementsAre(Pair("reset", "unsafe: input.cc:9:9")));
+}
+
+// Tests that structs can be nested. We use an optional field because its easy
+// to use in a test, but the type of the field shouldn't matter.
+TEST_P(UncheckedOptionalAccessTest, OptionalValueStruct) {
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct Foo {
+ $ns::$optional<std::string> opt;
+ };
+
+ void target($ns::$optional<Foo> foo) {
+ if (foo && foo->opt) {
+ foo->opt.value();
+ /*[[access]]*/
+ }
+ }
+ )",
+ UnorderedElementsAre(Pair("access", "safe")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, OptionalValueInitialization) {
+ // FIXME: Fix when to initialize `value`. All unwrapping should be safe in
+ // this example, but `value` initialization is done multiple times during the
+ // fixpoint iterations and joining the environment won't correctly merge them.
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ using Foo = $ns::$optional<std::string>;
+
+ void target($ns::$optional<Foo> foo, bool b) {
+ if (!foo.has_value()) return;
+ if (b) {
+ if (!foo->has_value()) return;
+ // We have created `foo.value()`.
+ foo->value();
+ } else {
+ if (!foo->has_value()) return;
+ // We have created `foo.value()` again, in a different environment.
+ foo->value();
+ }
+ // Now we merge the two values. UncheckedOptionalAccessModel::merge() will
+ // throw away the "value" property.
+ foo->value();
+ /*[[merge]]*/
+ }
+ )",
+ UnorderedElementsAre(Pair("merge", "unsafe: input.cc:19:7")));
+}
+
// FIXME: Add support for:
// - constructors (copy, move)
// - assignment operators (default, copy, move)
// - invalidation (passing optional by non-const reference/pointer)
-// - nested `optional` values
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -167,10 +167,17 @@
}
/// 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.
+BoolValue *getHasValue(Environment &Env, Value *Val) {
if (auto *OptionalVal = cast_or_null<StructValue>(Val)) {
- return cast<BoolValue>(OptionalVal->getProperty("has_value"));
+ auto *HasValueVal =
+ cast_or_null<BoolValue>(OptionalVal->getProperty("has_value"));
+ if (HasValueVal == nullptr) {
+ HasValueVal = &Env.makeAtomicBoolValue();
+ OptionalVal->setProperty("has_value", *HasValueVal);
+ }
+ return HasValueVal;
}
return nullptr;
}
@@ -207,6 +214,51 @@
.getDesugaredType(ASTCtx));
}
+/// Tries to initialize the `optional`'s value (that is, contents), and return
+/// its location. Returns nullptr if the value can't be represented.
+StorageLocation *maybeInitializeOptionalValueMember(QualType Q,
+ StructValue &OptionalVal,
+ Environment &Env) {
+ // 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
+ // the property is set, it will be shared by all environments that access the
+ // `Value` representing the optional (here, `OptionalVal`).
+ if (auto *ValueProp = OptionalVal.getProperty("value")) {
+ auto *ValueRef = clang::cast<ReferenceValue>(ValueProp);
+ auto &ValueLoc = ValueRef->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.
+ // 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
+ // the check, like another optional or a boolean that influences control
+ // flow.
+ auto *ValueVal = Env.createValue(ValueLoc.getType());
+ if (ValueVal == nullptr)
+ return nullptr;
+ // FIXME: ensure Expr type aligns with Value type.
+ Env.setValue(ValueLoc, *ValueVal);
+ }
+ return &ValueLoc;
+ }
+
+ auto Ty = stripReference(Q);
+ auto *ValueVal = Env.createValue(Ty);
+ if (ValueVal == nullptr)
+ return nullptr;
+ auto &ValueLoc = Env.createStorageLocation(Ty);
+ Env.setValue(ValueLoc, *ValueVal);
+ auto ValueRef = std::make_unique<ReferenceValue>(ValueLoc);
+ OptionalVal.setProperty("value", Env.takeOwnership(std::move(ValueRef)));
+ return &ValueLoc;
+}
+
void initializeOptionalReference(const Expr *OptionalExpr,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
@@ -222,11 +274,16 @@
LatticeTransferState &State) {
if (auto *OptionalVal = cast_or_null<StructValue>(
State.Env.getValue(*ObjectExpr, SkipPast::ReferenceThenPointer))) {
- auto *HasValueVal = getHasValue(OptionalVal);
- assert(HasValueVal != nullptr);
-
- if (State.Env.flowConditionImplies(*HasValueVal))
- return;
+ if (State.Env.getStorageLocation(*UnwrapExpr, SkipPast::None) == nullptr)
+ if (auto *Loc = maybeInitializeOptionalValueMember(
+ UnwrapExpr->getType(), *OptionalVal, State.Env))
+ State.Env.setStorageLocation(*UnwrapExpr, *Loc);
+
+ auto *Prop = OptionalVal->getProperty("has_value");
+ if (auto *HasValueVal = cast_or_null<BoolValue>(Prop)) {
+ if (State.Env.flowConditionImplies(*HasValueVal))
+ return;
+ }
}
// Record that this unwrap is *not* provably safe.
@@ -247,12 +304,9 @@
void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
- if (auto *OptionalVal = cast_or_null<StructValue>(
- State.Env.getValue(*CallExpr->getImplicitObjectArgument(),
- SkipPast::ReferenceThenPointer))) {
- auto *HasValueVal = getHasValue(OptionalVal);
- assert(HasValueVal != nullptr);
-
+ if (auto *HasValueVal = getHasValue(
+ State.Env, State.Env.getValue(*CallExpr->getImplicitObjectArgument(),
+ SkipPast::ReferenceThenPointer))) {
auto &CallExprLoc = State.Env.createStorageLocation(*CallExpr);
State.Env.setValue(CallExprLoc, *HasValueVal);
State.Env.setStorageLocation(*CallExpr, CallExprLoc);
@@ -273,12 +327,11 @@
Result.Nodes.getNodeAs<clang::CXXMemberCallExpr>(ValueOrCallID)
->getImplicitObjectArgument();
- auto *OptionalVal = cast_or_null<StructValue>(
- Env.getValue(*ObjectArgumentExpr, SkipPast::ReferenceThenPointer));
- if (OptionalVal == nullptr)
+ auto *HasValueVal = getHasValue(
+ State.Env,
+ State.Env.getValue(*ObjectArgumentExpr, SkipPast::ReferenceThenPointer));
+ if (HasValueVal == nullptr)
return;
- auto *HasValueVal = getHasValue(OptionalVal);
- assert(HasValueVal != nullptr);
auto *ExprValue = cast_or_null<BoolValue>(
State.Env.getValue(*ValueOrPredExpr, SkipPast::None));
@@ -353,8 +406,9 @@
// This is a constructor/assignment call for `optional<T>` with argument of
// type `optional<U>` such that `T` is constructible from `U`.
- if (BoolValue *Val = getHasValue(State.Env.getValue(E, SkipPast::Reference)))
- return *Val;
+ if (auto *HasValueVal =
+ getHasValue(State.Env, State.Env.getValue(E, SkipPast::Reference)))
+ return *HasValueVal;
return State.Env.makeAtomicBoolValue();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
