ymandel created this revision.
ymandel added reviewers: sgatev, xazax.hun.
Herald added subscribers: tschuett, steakhal, rnkovacs.
Herald added a project: All.
ymandel requested review of this revision.
Herald added a project: clang.
This patch adds limited modeling of the `value_or` method. Specifically, when
used in a particular idiom in a comparison to implicitly check whether the
optional holds a value.
Repository:
rG LLVM Github Monorepo
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,95 @@
UnorderedElementsAre(Pair("check", "safe")));
}
+TEST_P(UncheckedOptionalAccessTest, ValueOrComparison) {
+ // Pointers.
+ ExpectLatticeChecksFor(
+ R"code(
+ #include "unchecked_optional_access_test.h"
+
+ int* target() {
+ $ns::$optional<int*> opt;
+ if (opt.value_or(nullptr) != nullptr) {
+ return *opt;
+ /*[[check-ptrs-1]]*/
+ } else {
+ *opt;
+ /*[[check-ptrs-2]]*/
+ }
+ return nullptr;
+ }
+ )code",
+ UnorderedElementsAre(Pair("check-ptrs-1", "safe"),
+ Pair("check-ptrs-2", "unsafe: input.cc:10:10")));
+
+ // Integers.
+ ExpectLatticeChecksFor(
+ R"code(
+ #include "unchecked_optional_access_test.h"
+
+ int target() {
+ $ns::$optional<int> opt;
+ if (opt.value_or(0) != 0) {
+ return *opt;
+ /*[[check-ints-1]]*/
+ } else {
+ *opt;
+ /*[[check-ints-2]]*/
+ }
+ return 0;
+ }
+ )code",
+ UnorderedElementsAre(Pair("check-ints-1", "safe"),
+ Pair("check-ints-2", "unsafe: input.cc:10:10")));
+
+ // Strings.
+ ExpectLatticeChecksFor(
+ R"code(
+ #include "unchecked_optional_access_test.h"
+
+ namespace std {
+ struct string {
+ bool empty();
+ };
+ }
+
+ std::string target() {
+ $ns::$optional<std::string> opt;
+ if (!opt.value_or("").empty()) {
+ return *opt;
+ /*[[check-strings-1]]*/
+ } else {
+ *opt;
+ /*[[check-strings-2]]*/
+ }
+ return {};
+ }
+ )code",
+ UnorderedElementsAre(Pair("check-strings-1", "safe"),
+ Pair("check-strings-2", "unsafe: input.cc:16:10")));
+
+ // Pointer-to-optional.
+ ExpectLatticeChecksFor(
+ R"code(
+ #include "unchecked_optional_access_test.h"
+
+ int target() {
+ $ns::$optional<int> opt;
+ auto *opt_p = &opt;
+ if (opt_p->value_or(0) != 0) {
+ return opt_p->value();
+ /*[[check-pto-1]]*/
+ } else {
+ opt_p->value();
+ /*[[check-pto-2]]*/
+ }
+ return 0;
+ }
+ )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 +2097,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,41 @@
hasArgument(1, hasOptionalType()));
}
+auto isValueOrCondition(llvm::StringRef CallID) {
+ // `!opt.value_or("").empty()`.
+ auto NonEmptyStringOptional = unaryOperator(
+ hasOperatorName("!"),
+ hasUnaryOperand(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(CallID))))));
+
+ auto ComparesToSame =
+ [CallID](clang::ast_matchers::internal::Matcher<clang::Stmt> Arg) {
+ return hasOperands(
+ cxxMemberCallExpr(on(expr(unless(cxxThisExpr()))),
+ callee(cxxMethodDecl(hasName("value_or"),
+ ofClass(optionalClass()))),
+ hasArgument(0, Arg))
+ .bind(CallID),
+ ignoringImplicit(Arg));
+ };
+
+ return expr(
+ anyOf(NonEmptyStringOptional,
+ // `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.
+ 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 +247,30 @@
}
}
+void transferOptionalValueOrCall(const clang::Expr *ComparisonExpr,
+ const clang::Expr *ObjectArgumentExpr,
+ LatticeTransferState &State) {
+ auto *ComparisonExprValue = clang::cast_or_null<BoolValue>(
+ State.Env.getValue(*ComparisonExpr, SkipPast::None));
+ if (ComparisonExprValue == nullptr) {
+ auto &ComparisonExprLoc = State.Env.createStorageLocation(*ComparisonExpr);
+ ComparisonExprValue = &State.Env.makeAtomicBoolValue();
+ State.Env.setValue(ComparisonExprLoc, *ComparisonExprValue);
+ State.Env.setStorageLocation(*ComparisonExpr, ComparisonExprLoc);
+ }
+
+ if (auto *OptionalVal = cast_or_null<StructValue>(State.Env.getValue(
+ *ObjectArgumentExpr, SkipPast::ReferenceThenPointer))) {
+ auto *HasValueVal = getHasValue(OptionalVal);
+ assert(HasValueVal != nullptr);
+
+ // Connect the comparison expression to the optional's `hasValue` through
+ // the implication `(opt.value_or(X) != X) => opt.hasValue()`.
+ State.Env.addToFlowCondition(
+ State.Env.makeImplication(*ComparisonExprValue, *HasValueVal));
+ }
+}
+
void assignOptionalValue(const Expr &E, LatticeTransferState &State,
BoolValue &HasValueVal) {
if (auto *OptionalLoc =
@@ -418,6 +477,18 @@
// std::swap
.CaseOf<CallExpr>(isStdSwapCall(), transferStdSwapCall)
+ // opt.value_or(X) != X, !opt.value_or("").empty():
+ .CaseOf<Expr>(
+ isValueOrCondition("ValueOrCall"),
+ [](const clang::Expr *E, const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ transferOptionalValueOrCall(
+ E,
+ Result.Nodes.getNodeAs<clang::CXXMemberCallExpr>("ValueOrCall")
+ ->getImplicitObjectArgument(),
+ State);
+ })
+
.Build();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
