Skip to content

Commit

Permalink
[clang][dataflow] Track optional contents in optional model.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@ymand
ymand committed Jun 9, 2022
1 parent bc2c759 commit dd38caf
Show file tree
Hide file tree
Showing 2 changed files with 161 additions and 23 deletions.
94 changes: 73 additions & 21 deletions clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
View file
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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 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;
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) {
Expand All @@ -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.
Expand All @@ -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);
Expand All @@ -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));
Expand Down Expand Up @@ -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();
}

Expand Down
90 changes: 88 additions & 2 deletions clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -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 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

0 comments on commit dd38caf

Please sign in to comment.