diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h --- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h +++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h @@ -59,6 +59,14 @@ void transfer(const Stmt *Stmt, SourceLocationsLattice &State, Environment &Env); + bool compareEquivalent(QualType Type, const Value &Val1, + const Environment &Env1, const Value &Val2, + const Environment &Env2) override; + + bool merge(QualType Type, const Value &Val1, const Environment &Env1, + const Value &Val2, const Environment &Env2, Value &MergedVal, + Environment &MergedEnv) override; + private: MatchSwitch> TransferMatchSwitch; }; diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -169,11 +169,17 @@ optionalOrAliasType(), referenceType(pointee(optionalOrAliasType())))))); } +/// Sets `HasValueVal` as the symbolic value that represents the "has_value" +/// property of the optional value `OptionalVal`. +void setHasValue(Value &OptionalVal, BoolValue &HasValueVal) { + OptionalVal.setProperty("has_value", HasValueVal); +} + /// Creates a symbolic value for an `optional` value using `HasValueVal` as the /// symbolic value of its "has_value" property. StructValue &createOptionalValue(Environment &Env, BoolValue &HasValueVal) { auto OptionalVal = std::make_unique(); - OptionalVal->setProperty("has_value", HasValueVal); + setHasValue(*OptionalVal, HasValueVal); return Env.takeOwnership(std::move(OptionalVal)); } @@ -274,11 +280,17 @@ if (auto *OptionalVal = State.Env.getValue(*OptionalExpr, SkipPast::Reference)) { if (OptionalVal->getProperty("has_value") == nullptr) { - OptionalVal->setProperty("has_value", State.Env.makeAtomicBoolValue()); + setHasValue(*OptionalVal, State.Env.makeAtomicBoolValue()); } } } +bool isEngagedOptionalValue(const Value &OptionalVal, const Environment &env) { + auto *HasValueVal = + cast_or_null(OptionalVal.getProperty("has_value")); + return HasValueVal != nullptr && env.flowConditionImplies(*HasValueVal); +} + void transferUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr, LatticeTransferState &State) { if (auto *OptionalVal = @@ -651,5 +663,32 @@ TransferMatchSwitch(*S, getASTContext(), State); } +bool UncheckedOptionalAccessModel::compareEquivalent(QualType Type, + const Value &Val1, + const Environment &Env1, + const Value &Val2, + const Environment &Env2) { + return isEngagedOptionalValue(Val1, Env1) == + isEngagedOptionalValue(Val2, Env2); +} + +bool UncheckedOptionalAccessModel::merge(QualType Type, const Value &Val1, + const Environment &Env1, + const Value &Val2, + const Environment &Env2, + Value &MergedVal, + Environment &MergedEnv) { + if (!IsOptionalType(Type)) + return true; + + auto &HasValueVal = MergedEnv.makeAtomicBoolValue(); + if (isEngagedOptionalValue(Val1, Env1) && + isEngagedOptionalValue(Val2, Env2)) { + MergedEnv.addToFlowCondition(HasValueVal); + } + setHasValue(MergedVal, HasValueVal); + return true; +} + } // namespace dataflow } // namespace clang diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -1861,7 +1861,26 @@ )", UnorderedElementsAre(Pair("check", "safe"))); - // FIXME: Add tests that call `emplace` in conditional branches. + // FIXME: Add tests that call `emplace` in conditional branches: + // ExpectLatticeChecksFor( + // R"( + // #include "unchecked_optional_access_test.h" + // + // void target($ns::$optional opt, bool b) { + // if (b) { + // opt.emplace(0); + // } + // if (b) { + // opt.value(); + // /*[[check-1]]*/ + // } else { + // opt.value(); + // /*[[check-2]]*/ + // } + // } + // )", + // UnorderedElementsAre(Pair("check-1", "safe"), + // Pair("check-2", "unsafe: input.cc:12:9"))); } TEST_P(UncheckedOptionalAccessTest, Reset) { @@ -1892,7 +1911,27 @@ )", UnorderedElementsAre(Pair("check", "unsafe: input.cc:7:9"))); - // FIXME: Add tests that call `reset` in conditional branches. + // FIXME: Add tests that call `reset` in conditional branches: + // ExpectLatticeChecksFor( + // R"( + // #include "unchecked_optional_access_test.h" + // + // void target(bool b) { + // $ns::$optional opt = $ns::make_optional(0); + // if (b) { + // opt.reset(); + // } + // if (b) { + // opt.value(); + // /*[[check-1]]*/ + // } else { + // opt.value(); + // /*[[check-2]]*/ + // } + // } + // )", + // UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:10:9"), + // Pair("check-2", "safe"))); } TEST_P(UncheckedOptionalAccessTest, ValueAssignment) { @@ -2347,6 +2386,265 @@ UnorderedElementsAre(Pair("check", "unsafe: input.cc:12:7"))); } +TEST_P(UncheckedOptionalAccessTest, CorrelatedBranches) { + ExpectLatticeChecksFor(R"code( + #include "unchecked_optional_access_test.h" + + void target(bool b, $ns::$optional opt) { + if (b || opt.has_value()) { + if (!b) { + opt.value(); + /*[[check-1]]*/ + } + } + } + )code", + UnorderedElementsAre(Pair("check-1", "safe"))); + + ExpectLatticeChecksFor(R"code( + #include "unchecked_optional_access_test.h" + + void target(bool b, $ns::$optional opt) { + if (b && !opt.has_value()) return; + if (b) { + opt.value(); + /*[[check-2]]*/ + } + } + )code", + UnorderedElementsAre(Pair("check-2", "safe"))); + + ExpectLatticeChecksFor( + R"code( + #include "unchecked_optional_access_test.h" + + void target(bool b, $ns::$optional opt) { + if (opt.has_value()) b = true; + if (b) { + opt.value(); + /*[[check-3]]*/ + } + } + )code", + UnorderedElementsAre(Pair("check-3", "unsafe: input.cc:7:9"))); + + ExpectLatticeChecksFor(R"code( + #include "unchecked_optional_access_test.h" + + void target(bool b, $ns::$optional opt) { + if (b) return; + if (opt.has_value()) b = true; + if (b) { + opt.value(); + /*[[check-4]]*/ + } + } + )code", + UnorderedElementsAre(Pair("check-4", "safe"))); + + ExpectLatticeChecksFor(R"( + #include "unchecked_optional_access_test.h" + + void target(bool b, $ns::$optional opt) { + if (opt.has_value() == b) { + if (b) { + opt.value(); + /*[[check-5]]*/ + } + } + } + )", + UnorderedElementsAre(Pair("check-5", "safe"))); + + ExpectLatticeChecksFor(R"( + #include "unchecked_optional_access_test.h" + + void target(bool b, $ns::$optional opt) { + if (opt.has_value() != b) { + if (!b) { + opt.value(); + /*[[check-6]]*/ + } + } + } + )", + UnorderedElementsAre(Pair("check-6", "safe"))); + + // FIXME: Add support for operator==. + // ExpectLatticeChecksFor(R"( + // #include "unchecked_optional_access_test.h" + // + // void target($ns::$optional opt1, $ns::$optional opt2) { + // if (opt1 == opt2) { + // if (opt1.has_value()) { + // opt2.value(); + // /*[[check-7]]*/ + // } + // } + // } + // )", + // UnorderedElementsAre(Pair("check-7", "safe"))); +} + +TEST_P(UncheckedOptionalAccessTest, JoinDistinctValues) { + ExpectLatticeChecksFor( + R"code( + #include "unchecked_optional_access_test.h" + + void target(bool b) { + $ns::$optional opt; + if (b) { + opt = Make<$ns::$optional>(); + } else { + opt = Make<$ns::$optional>(); + } + if (opt.has_value()) { + opt.value(); + /*[[check-1]]*/ + } else { + opt.value(); + /*[[check-2]]*/ + } + } + )code", + UnorderedElementsAre(Pair("check-1", "safe"), + Pair("check-2", "unsafe: input.cc:15:9"))); + + ExpectLatticeChecksFor(R"code( + #include "unchecked_optional_access_test.h" + + void target(bool b) { + $ns::$optional opt; + if (b) { + opt = Make<$ns::$optional>(); + if (!opt.has_value()) return; + } else { + opt = Make<$ns::$optional>(); + if (!opt.has_value()) return; + } + opt.value(); + /*[[check-3]]*/ + } + )code", + UnorderedElementsAre(Pair("check-3", "safe"))); + + ExpectLatticeChecksFor( + R"code( + #include "unchecked_optional_access_test.h" + + void target(bool b) { + $ns::$optional opt; + if (b) { + opt = Make<$ns::$optional>(); + if (!opt.has_value()) return; + } else { + opt = Make<$ns::$optional>(); + } + opt.value(); + /*[[check-4]]*/ + } + )code", + UnorderedElementsAre(Pair("check-4", "unsafe: input.cc:12:7"))); + + ExpectLatticeChecksFor( + R"code( + #include "unchecked_optional_access_test.h" + + void target(bool b) { + $ns::$optional opt; + if (b) { + opt = 1; + } else { + opt = 2; + } + opt.value(); + /*[[check-5]]*/ + } + )code", + UnorderedElementsAre(Pair("check-5", "safe"))); + + ExpectLatticeChecksFor( + R"code( + #include "unchecked_optional_access_test.h" + + void target(bool b) { + $ns::$optional opt; + if (b) { + opt = 1; + } else { + opt = Make<$ns::$optional>(); + } + opt.value(); + /*[[check-6]]*/ + } + )code", + UnorderedElementsAre(Pair("check-6", "unsafe: input.cc:11:7"))); +} + +TEST_P(UncheckedOptionalAccessTest, ReassignValueInLoop) { + ExpectLatticeChecksFor(R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt = 3; + while (Make()) { + opt.value(); + /*[[check-1]]*/ + } + } + )", + UnorderedElementsAre(Pair("check-1", "safe"))); + + ExpectLatticeChecksFor(R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt = 3; + while (Make()) { + opt.value(); + /*[[check-2]]*/ + + opt = Make<$ns::$optional>(); + if (!opt.has_value()) return; + } + } + )", + UnorderedElementsAre(Pair("check-2", "safe"))); + + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt = 3; + while (Make()) { + opt.value(); + /*[[check-3]]*/ + + opt = Make<$ns::$optional>(); + } + } + )", + UnorderedElementsAre(Pair("check-3", "unsafe: input.cc:7:9"))); + + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt = 3; + while (Make()) { + opt.value(); + /*[[check-4]]*/ + + opt = Make<$ns::$optional>(); + if (!opt.has_value()) continue; + } + } + )", + UnorderedElementsAre(Pair("check-4", "unsafe: input.cc:7:9"))); +} + // FIXME: Add support for: // - constructors (copy, move) // - assignment operators (default, copy, move)