ymandel updated this revision to Diff 427029.
ymandel added a comment.
remove stray newline
Repository:
rG LLVM Github Monorepo
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,195 @@
UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe")));
}
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalInOptional) {
+ 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")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalInOptionalStruct) {
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct Foo {
+ $ns::$optional<std::string> opt;
+ };
+
+ void target(const $ns::$optional<Foo>& foo) {
+ if (foo && foo->opt) {
+ foo->opt.value();
+ /*[[access]]*/
+ }
+ }
+ )",
+ UnorderedElementsAre(Pair("access", "safe")));
+
+ // `reset` changes the state.
+ 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.reset();
+ foo->opt.value();
+ /*[[reset]]*/
+ }
+ }
+ )",
+ UnorderedElementsAre(Pair("reset", "unsafe: input.cc:11:9")));
+
+ // `has_value` and `operator*`.
+ 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.has_value()) {
+ *foo->opt;
+ /*[[other-ops]]*/
+ }
+ }
+ )",
+ UnorderedElementsAre(Pair("other-ops", "safe")));
+
+ // `operator->`.
+ 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.has_value()) {
+ foo->opt->empty();
+ /*[[arrow]]*/
+ }
+ }
+ )",
+ UnorderedElementsAre(Pair("arrow", "safe")));
+
+ // Accessing the nested optional in the wrong branch.
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct Foo {
+ $ns::$optional<std::string> opt;
+ };
+
+ void target($ns::$optional<Foo>& foo) {
+ if (!foo.has_value()) return;
+ if (!foo->opt.has_value()) {
+ foo->opt.value();
+ /*[[not-set]]*/
+ }
+ }
+ )",
+ UnorderedElementsAre(Pair("not-set", "unsafe: input.cc:11:9")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalInOptionalStructField) {
+ // Non-standard assignment.
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct Foo {
+ $ns::$optional<std::string> opt;
+ };
+
+ struct Bar {
+ $ns::$optional<Foo> member;
+ };
+
+ Bar createBar();
+
+ void target() {
+ Bar bar = createBar();
+ bar.member->opt = "a";
+ /*[[ns-assign]]*/
+ }
+ )",
+ UnorderedElementsAre(Pair("ns-assign", "unsafe: input.cc:16:7")));
+
+ // Resetting the nested optional.
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct Member {
+ $ns::$optional<std::string> opt;
+ };
+
+ struct Foo {
+ $ns::$optional<Member> member;
+ };
+
+ Foo createFoo();
+
+ void target() {
+ Foo foo = createFoo();
+ foo.member->opt.reset();
+ /*[[nested-reset]]*/
+ }
+ )",
+ UnorderedElementsAre(Pair("nested-reset", "unsafe: input.cc:16:7")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalInitialization) {
+ // 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,47 @@
.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 *initializeUnwrappedValue(const Expr &Expr,
+ 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.
+ 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.
+ auto *ValueVal = Env.createValue(stripReference(Expr.getType()));
+ if (ValueVal == nullptr)
+ return nullptr;
+ Env.setValue(ValueLoc, *ValueVal);
+ }
+ return &ValueLoc;
+ }
+
+ auto Ty = stripReference(Expr.getType());
+ auto *ValueVal = Env.createValue(Ty);
+ if (ValueVal == nullptr)
+ return nullptr;
+ // FIXME: the `StorageLocation` of the "value" property should be set
+ // eagerly when the optional is constructed, so that it can be shared by all
+ // environments that use the optional's value. (The creation of the
+ // underlying value can still be populated lazily).
+ 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 +270,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 =
+ initializeUnwrappedValue(*UnwrapExpr, *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.
@@ -245,12 +298,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);
@@ -271,12 +321,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));
@@ -351,8 +400,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
