[clang] dd38caf - [clang][dataflow] Track `optional` contents in `optional` model.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Thu Jun 9 07:18:53 PDT 2022
Author: Yitzhak Mandelbaum
Date: 2022-06-09T14:17:39Z
New Revision: dd38caf3b5b724c1f84998c1ae105f709c3ed405
URL: https://github.com/llvm/llvm-project/commit/dd38caf3b5b724c1f84998c1ae105f709c3ed405
DIFF: https://github.com/llvm/llvm-project/commit/dd38caf3b5b724c1f84998c1ae105f709c3ed405.diff
LOG: [clang][dataflow] Track `optional` contents in `optional` model.
This patch adds partial support for tracking (i.e. modeling) the contents of an
optional value. Specifically, it supports tracking the value after it is
accessed. We leave tracking constructed/assigned contents to a future patch.
Differential Revision: https://reviews.llvm.org/D124932
Added:
Modified:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index cf93a07f832df..00d22fbe8bb80 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -179,9 +179,15 @@ StructValue &createOptionalValue(Environment &Env, BoolValue &HasValueVal) {
/// Returns the symbolic value that represents the "has_value" property of the
/// optional value `OptionalVal`. Returns null if `OptionalVal` is null.
-BoolValue *getHasValue(Value *OptionalVal) {
- if (OptionalVal) {
- return cast<BoolValue>(OptionalVal->getProperty("has_value"));
+BoolValue *getHasValue(Environment &Env, Value *OptionalVal) {
+ if (OptionalVal != nullptr) {
+ 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;
}
@@ -218,6 +224,50 @@ int countOptionalWrappers(const ASTContext &ASTCtx, QualType Type) {
.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,
+ Value &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
diff erent 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;
+ 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) {
@@ -233,11 +283,16 @@ void transferUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
LatticeTransferState &State) {
if (auto *OptionalVal =
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.
@@ -258,12 +313,9 @@ void transferMakeOptionalCall(const CallExpr *E,
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);
@@ -284,12 +336,11 @@ void transferValueOrImpl(const clang::Expr *ValueOrPredExpr,
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));
@@ -376,8 +427,9 @@ getValueOrConversionHasValue(const FunctionDecl &F, const Expr &E,
// 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();
}
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index c83cf49567c1d..72efc724776b3 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2165,7 +2165,6 @@ TEST_P(UncheckedOptionalAccessTest, CallReturningOptional) {
}
)",
UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:9:7")));
-
ExpectLatticeChecksFor(
R"(
#include "unchecked_optional_access_test.h"
@@ -2231,8 +2230,95 @@ TEST_P(UncheckedOptionalAccessTest, WithAlias) {
UnorderedElementsAre(Pair("check", "unsafe: input.cc:8:7")));
}
+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
diff erent 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
More information about the cfe-commits
mailing list