ymandel updated this revision to Diff 418209.
ymandel marked an inline comment as done.
ymandel added a comment.
adjust logical formula
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122231/new/
https://reviews.llvm.org/D122231
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
@@ -1710,6 +1710,91 @@
UnorderedElementsAre(Pair("check", "safe")));
}
+TEST_P(UncheckedOptionalAccessTest, ValueOrComparison) {
+ // Pointers.
+ ExpectLatticeChecksFor(
+ R"code(
+ #include "unchecked_optional_access_test.h"
+
+ void target() {
+ $ns::$optional<int*> opt;
+ if (opt.value_or(nullptr) != nullptr) {
+ opt.value();
+ /*[[check-ptrs-1]]*/
+ } else {
+ opt.value();
+ /*[[check-ptrs-2]]*/
+ }
+ }
+ )code",
+ UnorderedElementsAre(Pair("check-ptrs-1", "safe"),
+ Pair("check-ptrs-2", "unsafe: input.cc:10:9")));
+
+ // Integers.
+ ExpectLatticeChecksFor(
+ R"code(
+ #include "unchecked_optional_access_test.h"
+
+ void target() {
+ $ns::$optional<int> opt;
+ if (opt.value_or(0) != 0) {
+ opt.value();
+ /*[[check-ints-1]]*/
+ } else {
+ opt.value();
+ /*[[check-ints-2]]*/
+ }
+ }
+ )code",
+ UnorderedElementsAre(Pair("check-ints-1", "safe"),
+ Pair("check-ints-2", "unsafe: input.cc:10:9")));
+
+ // Strings.
+ ExpectLatticeChecksFor(
+ R"code(
+ #include "unchecked_optional_access_test.h"
+
+ namespace std {
+ struct string {
+ bool empty();
+ };
+ }
+
+ void target() {
+ $ns::$optional<std::string> opt;
+ if (!opt.value_or("").empty()) {
+ opt.value();
+ /*[[check-strings-1]]*/
+ } else {
+ opt.value();
+ /*[[check-strings-2]]*/
+ }
+ }
+ )code",
+ UnorderedElementsAre(Pair("check-strings-1", "safe"),
+ Pair("check-strings-2", "unsafe: input.cc:16:9")));
+
+ // Pointer-to-optional.
+ ExpectLatticeChecksFor(
+ R"code(
+ #include "unchecked_optional_access_test.h"
+
+ void target() {
+ $ns::$optional<int> opt;
+ auto *opt_p = &opt;
+ if (opt_p->value_or(0) != 0) {
+ opt_p->value();
+ /*[[check-pto-1]]*/
+ } else {
+ opt_p->value();
+ /*[[check-pto-2]]*/
+ }
+ }
+ )code",
+ UnorderedElementsAre(Pair("check-pto-1", "safe"),
+ Pair("check-pto-2", "unsafe: input.cc:11:9")));
+}
+
TEST_P(UncheckedOptionalAccessTest, Emplace) {
ExpectLatticeChecksFor(R"(
#include "unchecked_optional_access_test.h"
@@ -2008,5 +2093,4 @@
// - constructors (copy, move)
// - assignment operators (default, copy, move)
// - invalidation (passing optional by non-const reference/pointer)
-// - `value_or(nullptr) != nullptr`, `value_or(0) != 0`, `value_or("").empty()`
// - 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
@@ -113,6 +113,40 @@
hasArgument(1, hasOptionalType()));
}
+constexpr llvm::StringLiteral ValueOrCallID = "ValueOrCall";
+
+auto isValueOrStringEmptyCall() {
+ // `opt.value_or("").empty()`
+ return cxxMemberCallExpr(
+ callee(cxxMethodDecl(hasName("empty"))),
+ onImplicitObjectArgument(ignoringImplicit(
+ cxxMemberCallExpr(on(expr(unless(cxxThisExpr()))),
+ callee(cxxMethodDecl(hasName("value_or"),
+ ofClass(optionalClass()))),
+ hasArgument(0, stringLiteral(hasSize(0))))
+ .bind(ValueOrCallID))));
+}
+
+auto isValueOrCondition() {
+ auto ComparesToSame = [](ast_matchers::internal::Matcher<Stmt> Arg) {
+ return hasOperands(
+ cxxMemberCallExpr(on(expr(unless(cxxThisExpr()))),
+ callee(cxxMethodDecl(hasName("value_or"),
+ ofClass(optionalClass()))),
+ hasArgument(0, Arg))
+ .bind(ValueOrCallID),
+ ignoringImplicit(Arg));
+ };
+
+ // `opt.value_or(nullptr) != nullptr` and `opt.value_or(0) != 0`. Ideally,
+ // we'd support this pattern for any expression, but the AST does not have a
+ // generic expression comparison facility, so we specialize to common cases
+ // seen in practice.
+ return binaryOperation(hasOperatorName("!="),
+ anyOf(ComparesToSame(cxxNullPtrLiteralExpr()),
+ ComparesToSame(integerLiteral(equals(0)))));
+}
+
/// 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) {
@@ -212,6 +246,74 @@
}
}
+void transferValueOrImpl(const clang::Expr *ComparisonExpr,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State,
+ BoolValue &(*MakeValue)(Environment &Env,
+ BoolValue &HasValueVal)) {
+ auto &Env = State.Env;
+
+ const auto *ObjectArgumentExpr =
+ Result.Nodes.getNodeAs<clang::CXXMemberCallExpr>(ValueOrCallID)
+ ->getImplicitObjectArgument();
+
+ auto *OptionalVal = cast_or_null<StructValue>(
+ Env.getValue(*ObjectArgumentExpr, SkipPast::ReferenceThenPointer));
+ if (OptionalVal == nullptr)
+ return;
+
+ auto *HasValueVal = getHasValue(OptionalVal);
+ assert(HasValueVal != nullptr);
+
+ // Constrain the `BoolValue` representing the comparison, initializing as
+ // needed.
+ BoolValue &ComparisonValue = MakeValue(Env, *HasValueVal);
+ auto *ComparisonExprLoc =
+ Env.getStorageLocation(*ComparisonExpr, SkipPast::None);
+ if (ComparisonExprLoc == nullptr) {
+ ComparisonExprLoc = &Env.createStorageLocation(*ComparisonExpr);
+ Env.setStorageLocation(*ComparisonExpr, *ComparisonExprLoc);
+ Env.setValue(*ComparisonExprLoc, ComparisonValue);
+ } else if (auto *CurrentValue =
+ cast_or_null<BoolValue>(Env.getValue(*ComparisonExprLoc))) {
+ Env.setValue(*ComparisonExprLoc,
+ Env.makeAnd(*CurrentValue, ComparisonValue));
+ } else {
+ Env.setValue(*ComparisonExprLoc, ComparisonValue);
+ }
+}
+
+void transferValueOrStringEmptyCall(const clang::Expr *ComparisonExpr,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ return transferValueOrImpl(
+ ComparisonExpr, Result, State,
+ [](Environment &Env, BoolValue &HasValueVal) -> BoolValue & {
+ // We can't reason about strings, so we encode the constraint on the
+ // optional's contents (namely, that it holds the empty string) as an
+ // atom.
+ BoolValue &OptionalHoldsEmptyString = Env.makeAtomicBoolValue();
+ return Env.makeOr(Env.makeAnd(HasValueVal, OptionalHoldsEmptyString),
+ Env.makeNot(HasValueVal));
+ });
+}
+
+void transferValueOrNotEqX(const Expr *ComparisonExpr,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ transferValueOrImpl(
+ ComparisonExpr, Result, State,
+ [](Environment &Env, BoolValue &HasValueVal) -> BoolValue & {
+ // We can't reason about general vlaues, so we encode the constraint on
+ // the optional's contents (namely, that it is not equal to X) as an
+ // atom. The conjunction is important in case this is used in a
+ // conditinoal -- the negation is "!HasValue or ContentsEqx". Without
+ // the conjunction, we'd get the simpler, and incorrect, "!HasValue".
+ BoolValue &ContentsNotEqX = Env.makeAtomicBoolValue();
+ return Env.makeAnd(HasValueVal, ContentsNotEqX);
+ });
+}
+
void assignOptionalValue(const Expr &E, LatticeTransferState &State,
BoolValue &HasValueVal) {
if (auto *OptionalLoc =
@@ -418,6 +520,12 @@
// std::swap
.CaseOf<CallExpr>(isStdSwapCall(), transferStdSwapCall)
+ // opt.value_or("").empty()
+ .CaseOf<Expr>(isValueOrStringEmptyCall(), transferValueOrStringEmptyCall)
+
+ // opt.value_or(X) != X
+ .CaseOf<Expr>(isValueOrCondition(), transferValueOrNotEqX)
+
.Build();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
