ymandel updated this revision to Diff 419240.
ymandel marked 5 inline comments as done.
ymandel added a comment.
address comments.
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
@@ -496,6 +496,24 @@
#endif // ABSL_TYPE_TRAITS_H
)";
+static constexpr char StdStringHeader[] = R"(
+#ifndef STRING_H
+#define STRING_H
+
+namespace std {
+
+struct string {
+ string(const char*);
+ ~string();
+ bool empty();
+};
+bool operator!=(const string &LHS, const char *RHS);
+
+} // namespace std
+
+#endif // STRING_H
+)";
+
static constexpr char StdUtilityHeader[] = R"(
#ifndef UTILITY_H
#define UTILITY_H
@@ -1198,6 +1216,7 @@
std::vector<std::pair<std::string, std::string>> Headers;
Headers.emplace_back("cstddef.h", CSDtdDefHeader);
Headers.emplace_back("std_initializer_list.h", StdInitializerListHeader);
+ Headers.emplace_back("std_string.h", StdStringHeader);
Headers.emplace_back("std_type_traits.h", StdTypeTraitsHeader);
Headers.emplace_back("std_utility.h", StdUtilityHeader);
Headers.emplace_back("std_optional.h", StdOptionalHeader);
@@ -1209,6 +1228,7 @@
#include "base_optional.h"
#include "std_initializer_list.h"
#include "std_optional.h"
+ #include "std_string.h"
#include "std_utility.h"
template <typename T>
@@ -1712,6 +1732,102 @@
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:9: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:9:9")));
+
+ // Strings.
+ ExpectLatticeChecksFor(
+ R"code(
+ #include "unchecked_optional_access_test.h"
+
+ 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:9:9")));
+
+ ExpectLatticeChecksFor(
+ R"code(
+ #include "unchecked_optional_access_test.h"
+
+ void target($ns::$optional<std::string> opt) {
+ if (opt.value_or("") != "") {
+ opt.value();
+ /*[[check-strings-neq-1]]*/
+ } else {
+ opt.value();
+ /*[[check-strings-neq-2]]*/
+ }
+ }
+ )code",
+ UnorderedElementsAre(
+ Pair("check-strings-neq-1", "safe"),
+ Pair("check-strings-neq-2", "unsafe: input.cc:9:9")));
+
+ // Pointer-to-optional.
+ //
+ // FIXME: make `opt` a parameter directly, once we ensure that all `optional`
+ // values have a `has_value` property.
+ ExpectLatticeChecksFor(
+ R"code(
+ #include "unchecked_optional_access_test.h"
+
+ void target($ns::$optional<int> p) {
+ $ns::$optional<int> *opt = &p;
+ if (opt->value_or(0) != 0) {
+ opt->value();
+ /*[[check-pto-1]]*/
+ } else {
+ opt->value();
+ /*[[check-pto-2]]*/
+ }
+ }
+ )code",
+ UnorderedElementsAre(Pair("check-pto-1", "safe"),
+ Pair("check-pto-2", "unsafe: input.cc:10:9")));
+}
+
TEST_P(UncheckedOptionalAccessTest, Emplace) {
ExpectLatticeChecksFor(R"(
#include "unchecked_optional_access_test.h"
@@ -2038,5 +2154,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
@@ -121,6 +121,43 @@
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 isValueOrNotEqX() {
+ auto ComparesToSame = [](ast_matchers::internal::Matcher<Stmt> Arg) {
+ return hasOperands(
+ ignoringImplicit(
+ cxxMemberCallExpr(on(expr(unless(cxxThisExpr()))),
+ callee(cxxMethodDecl(hasName("value_or"),
+ ofClass(optionalClass()))),
+ hasArgument(0, Arg))
+ .bind(ValueOrCallID)),
+ ignoringImplicit(Arg));
+ };
+
+ // `opt.value_or(X) != X`, for X is `nullptr`, `""`, or `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. FIXME: define a matcher that compares values across
+ // nodes, which would let us generalize this to any `X`.
+ return binaryOperation(hasOperatorName("!="),
+ anyOf(ComparesToSame(cxxNullPtrLiteralExpr()),
+ ComparesToSame(stringLiteral(hasSize(0))),
+ 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) {
@@ -220,6 +257,70 @@
}
}
+/// `ModelPred` builds a logical formula relating the predicate in
+/// `ValueOrPredExpr` to the optional's `has_value` property.
+void transferValueOrImpl(const clang::Expr *ValueOrPredExpr,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State,
+ BoolValue &(*ModelPred)(Environment &Env,
+ BoolValue &ExprVal,
+ 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);
+
+ assert(ValueOrPredExpr);
+ auto *ExprValue = cast_or_null<BoolValue>(
+ State.Env.getValue(*ValueOrPredExpr, SkipPast::None));
+ if (ExprValue == nullptr) {
+ auto &ExprLoc = State.Env.createStorageLocation(*ValueOrPredExpr);
+ ExprValue = &State.Env.makeAtomicBoolValue();
+ State.Env.setValue(ExprLoc, *ExprValue);
+ State.Env.setStorageLocation(*ValueOrPredExpr, ExprLoc);
+ }
+
+ Env.addToFlowCondition(ModelPred(Env, *ExprValue, *HasValueVal));
+}
+
+void transferValueOrStringEmptyCall(const clang::Expr *ComparisonExpr,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ return transferValueOrImpl(ComparisonExpr, Result, State,
+ [](Environment &Env, BoolValue &ExprVal,
+ BoolValue &HasValueVal) -> BoolValue & {
+ // If the result is *not* empty, then we know the
+ // optional must have been holding a value. If
+ // `ExprVal` is true, though, we don't learn
+ // anything definite about `has_value`, so we
+ // don't add any corresponding implications to
+ // the flow condition.
+ return Env.makeImplication(Env.makeNot(ExprVal),
+ HasValueVal);
+ });
+}
+
+void transferValueOrNotEqX(const Expr *ComparisonExpr,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ transferValueOrImpl(ComparisonExpr, Result, State,
+ [](Environment &Env, BoolValue &ExprVal,
+ BoolValue &HasValueVal) -> BoolValue & {
+ // We know that if `(opt.value_or(X) != X)` then
+ // `opt.hasValue()`, even without knowing further
+ // details about the contents of `opt`.
+ return Env.makeImplication(ExprVal, HasValueVal);
+ });
+}
+
void assignOptionalValue(const Expr &E, LatticeTransferState &State,
BoolValue &HasValueVal) {
if (auto *OptionalLoc =
@@ -439,6 +540,12 @@
// std::swap
.CaseOf<CallExpr>(isStdSwapCall(), transferStdSwapCall)
+ // opt.value_or("").empty()
+ .CaseOf<Expr>(isValueOrStringEmptyCall(), transferValueOrStringEmptyCall)
+
+ // opt.value_or(X) != X
+ .CaseOf<Expr>(isValueOrNotEqX(), transferValueOrNotEqX)
+
.Build();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
