diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp index 09494322a16c4..ecd083e4ce9a3 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp @@ -281,6 +281,76 @@ static auto isNonConstMemberOperatorCall() { return cxxOperatorCallExpr(callee(cxxMethodDecl(unless(isConst())))); } +static auto isMakePredicateFormatterFromIsOkMatcherCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return callExpr( + callee(functionDecl( + hasName("::testing::internal::MakePredicateFormatterFromMatcher"))), + hasArgument( + 0, hasType(cxxRecordDecl(hasAnyName( + "::testing::status::internal_status::IsOkMatcher", + "::absl_testing::status_internal::IsOkMatcher", + "::testing::status::internal_status::IsOkAndHoldsMatcher", + "::absl_testing::status_internal::IsOkAndHoldsMatcher"))))); +} + +static auto isStatusIsOkMatcherCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return callExpr(callee(functionDecl(hasAnyName( + "::testing::status::StatusIs", "absl_testing::StatusIs", + "::testing::status::CanonicalStatusIs", + "::absl_testing::CanonicalStatusIs"))), + hasArgument(0, declRefExpr(to(enumConstantDecl(hasAnyName( + "::absl::StatusCode::kOk", "OK")))))); +} + +static auto isMakePredicateFormatterFromStatusIsMatcherCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return callExpr( + callee(functionDecl( + hasName("::testing::internal::MakePredicateFormatterFromMatcher"))), + hasArgument(0, hasType(cxxRecordDecl(hasAnyName( + "::testing::status::internal_status::StatusIsMatcher", + "::testing::status::internal_status::" + "CanonicalStatusIsMatcher", + "::absl_testing::status_internal::StatusIsMatcher", + "::absl_testing::status_internal::" + "CanonicalStatusIsMatcher"))))); +} + +static auto isPredicateFormatterFromStatusMatcherCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxOperatorCallExpr( + hasOverloadedOperatorName("()"), + callee(cxxMethodDecl(ofClass( + hasName("testing::internal::PredicateFormatterFromMatcher")))), + hasArgument(2, hasType(cxxRecordDecl(hasName("absl::Status"))))); +} + +static auto isPredicateFormatterFromStatusOrMatcherCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxOperatorCallExpr( + hasOverloadedOperatorName("()"), + callee(cxxMethodDecl(ofClass( + hasName("testing::internal::PredicateFormatterFromMatcher")))), + hasArgument(2, hasType(statusOrType()))); +} + +static auto isAssertionResultOperatorBoolCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxMemberCallExpr( + on(expr(unless(cxxThisExpr()))), + callee(cxxMethodDecl(hasName("operator bool"), + ofClass(hasName("testing::AssertionResult"))))); +} + +static auto isAssertionResultConstructFromBoolCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxConstructExpr( + hasType(recordDecl(hasName("testing::AssertionResult"))), + hasArgument(0, hasType(booleanType()))); +} + static auto buildDiagnoseMatchSwitch(const UncheckedStatusOrAccessModelOptions &Options) { return CFGMatchSwitchBuilder getSyntheticFields(QualType Ty, QualType StatusType, const CXXRecordDecl &RD) { if (auto *TRD = getStatusOrBaseClass(Ty)) @@ -372,6 +462,12 @@ llvm::StringMap getSyntheticFields(QualType Ty, QualType StatusType, if (isStatusType(Ty) || (RD.hasDefinition() && RD.isDerivedFrom(StatusType->getAsCXXRecordDecl()))) return {{"ok", RD.getASTContext().BoolTy}}; + if (isAssertionResultType(Ty)) + return {{"ok", RD.getASTContext().BoolTy}}; + if (isPredicateFormatterFromMatcherType(Ty)) + return {{"ok_predicate", RD.getASTContext().BoolTy}}; + if (isStatusIsMatcherType(Ty)) + return {{"ok_matcher", RD.getASTContext().BoolTy}}; return {}; } @@ -388,6 +484,13 @@ BoolValue &valForOk(RecordStorageLocation &StatusLoc, Environment &Env) { return *Val; return initializeStatus(StatusLoc, Env); } +static StorageLocation &locForOkPredicate(RecordStorageLocation &StatusLoc) { + return StatusLoc.getSyntheticField("ok_predicate"); +} + +static StorageLocation &locForOkMatcher(RecordStorageLocation &StatusLoc) { + return StatusLoc.getSyntheticField("ok_matcher"); +} static void transferStatusOrOkCall(const CXXMemberCallExpr *Expr, const MatchFinder::MatchResult &, @@ -850,6 +953,97 @@ transferNonConstMemberOperatorCall(const CXXOperatorCallExpr *Expr, handleNonConstMemberCall(Expr, RecordLoc, Result, State); } +static void transferMakePredicateFormatterFromIsOkMatcherCall( + const CallExpr *Expr, const MatchFinder::MatchResult &, + LatticeTransferState &State) { + State.Env.setValue( + locForOkPredicate(State.Env.getResultObjectLocation(*Expr)), + State.Env.getBoolLiteralValue(true)); +} + +static void transferStatusIsOkMatcherCall(const CallExpr *Expr, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + BoolValue &OkMatcherVal = State.Env.getBoolLiteralValue(true); + State.Env.setValue(locForOkMatcher(State.Env.getResultObjectLocation(*Expr)), + OkMatcherVal); +} + +static void transferMakePredicateFormatterFromStatusIsMatcherCall( + const CallExpr *Expr, const MatchFinder::MatchResult &, + LatticeTransferState &State) { + assert(Expr->isPRValue()); + auto &Loc = State.Env.getResultObjectLocation(*Expr->getArg(0)); + auto &OkMatcherLoc = locForOkMatcher(Loc); + BoolValue *OkMatcherVal = State.Env.get(OkMatcherLoc); + if (OkMatcherVal == nullptr) + return; + State.Env.setValue( + locForOkPredicate(State.Env.getResultObjectLocation(*Expr)), + *OkMatcherVal); +} + +static void +transferPredicateFormatterMatcherCall(const CXXOperatorCallExpr *Expr, + LatticeTransferState &State, + bool IsStatusOr) { + auto *Loc = State.Env.get(*Expr->getArg(0)); + if (Loc == nullptr) + return; + + auto *ObjectLoc = State.Env.get(*Expr->getArg(2)); + if (ObjectLoc == nullptr) + return; + + auto &OkPredicateLoc = locForOkPredicate(*Loc); + BoolValue *OkPredicateVal = State.Env.get(OkPredicateLoc); + if (OkPredicateVal == nullptr) + return; + + if (IsStatusOr) + ObjectLoc = &locForStatus(*ObjectLoc); + auto &StatusOk = valForOk(*ObjectLoc, State.Env); + + auto &A = State.Env.arena(); + auto &Res = State.Env.makeAtomicBoolValue(); + State.Env.assume( + A.makeImplies(OkPredicateVal->formula(), + A.makeEquals(StatusOk.formula(), Res.formula()))); + State.Env.setValue(locForOk(State.Env.getResultObjectLocation(*Expr)), Res); +} + +static void +transferAssertionResultConstructFromBoolCall(const CXXConstructExpr *Expr, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + assert(Expr->getNumArgs() > 0); + + auto *StatusAdaptorLoc = State.Env.get(*Expr->getArg(0)); + if (StatusAdaptorLoc == nullptr) + return; + BoolValue *OkVal = State.Env.get(*StatusAdaptorLoc); + if (OkVal == nullptr) + return; + State.Env.setValue(locForOk(State.Env.getResultObjectLocation(*Expr)), + *OkVal); +} + +static void +transferAssertionResultOperatorBoolCall(const CXXMemberCallExpr *Expr, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + auto *RecordLoc = getImplicitObjectLocation(*Expr, State.Env); + if (RecordLoc == nullptr) + return; + BoolValue *OkVal = State.Env.get(locForOk(*RecordLoc)); + if (OkVal == nullptr) + return; + auto &A = State.Env.arena(); + auto &Res = State.Env.makeAtomicBoolValue(); + State.Env.assume(A.makeEquals(OkVal->formula(), Res.formula())); + State.Env.setValue(*Expr, Res); +} + static RecordStorageLocation * getSmartPtrLikeStorageLocation(const Expr &E, const Environment &Env) { if (!E.isPRValue()) @@ -865,6 +1059,33 @@ buildTransferMatchSwitch(ASTContext &Ctx, CFGMatchSwitchBuilder Builder) { using namespace ::clang::ast_matchers; // NOLINT: Too many names return std::move(Builder) + .CaseOfCFGStmt( + isMakePredicateFormatterFromIsOkMatcherCall(), + transferMakePredicateFormatterFromIsOkMatcherCall) + .CaseOfCFGStmt(isStatusIsOkMatcherCall(), + transferStatusIsOkMatcherCall) + .CaseOfCFGStmt( + isMakePredicateFormatterFromStatusIsMatcherCall(), + transferMakePredicateFormatterFromStatusIsMatcherCall) + .CaseOfCFGStmt( + isPredicateFormatterFromStatusOrMatcherCall(), + [](const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &, + LatticeTransferState &State) { + transferPredicateFormatterMatcherCall(Expr, State, + /*IsStatusOr=*/true); + }) + .CaseOfCFGStmt( + isPredicateFormatterFromStatusMatcherCall(), + [](const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &, + LatticeTransferState &State) { + transferPredicateFormatterMatcherCall(Expr, State, + /*IsStatusOr=*/false); + }) + .CaseOfCFGStmt( + isAssertionResultConstructFromBoolCall(), + transferAssertionResultConstructFromBoolCall) + .CaseOfCFGStmt(isAssertionResultOperatorBoolCall(), + transferAssertionResultOperatorBoolCall) .CaseOfCFGStmt(isStatusOrMemberCallWithName("ok"), transferStatusOrOkCall) .CaseOfCFGStmt(isStatusOrMemberCallWithName("status"), diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp index deaeea31523cb..ce929cfd9593b 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp @@ -2173,6 +2173,181 @@ TEST_P(UncheckedStatusOrAccessModelTest, ExpectOkMacro) { )cc"); } +TEST_P(UncheckedStatusOrAccessModelTest, AssertThatMacro) { + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + ASSERT_THAT(sor, testing::status::IsOk()); + + sor.value(); + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + ASSERT_THAT(sor, absl_testing::IsOk()); + + sor.value(); + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + ASSERT_THAT(sor.status(), testing::status::IsOk()); + + sor.value(); + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + void target() { + STATUSOR_INT sor = Make(); + ASSERT_THAT(sor, testing::status::IsOk()); + + sor.value(); + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + using absl::StatusCode::kOk; + ASSERT_THAT(sor, testing::status::StatusIs(kOk)); + + sor.value(); + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + ASSERT_THAT(sor, testing::status::StatusIs(absl::StatusCode::kOk)); + + sor.value(); + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + ASSERT_THAT(sor, testing::status::CanonicalStatusIs(absl::StatusCode::kOk)); + + sor.value(); + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + ASSERT_THAT(sor, absl_testing::CanonicalStatusIs(absl::StatusCode::kOk)); + + sor.value(); + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + auto matcher = testing::status::StatusIs(absl::StatusCode::kOk); + ASSERT_THAT(sor, matcher); + + sor.value(); + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + auto matcher = absl_testing::StatusIs(absl::StatusCode::kOk); + ASSERT_THAT(sor, matcher); + + sor.value(); + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + ASSERT_THAT( + sor, testing::status::StatusIs(absl::StatusCode::kInvalidArgument)); + + sor.value(); // [[unsafe]] + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + ASSERT_THAT(sor, absl_testing::StatusIs(absl::StatusCode::kInvalidArgument)); + + sor.value(); // [[unsafe]] + } + )cc"); + + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + ASSERT_THAT(sor, testing::status::IsOkAndHolds(testing::IsTrue())); + + sor.value(); + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + ASSERT_THAT(sor, absl_testing::IsOkAndHolds(testing::IsTrue())); + + sor.value(); + } + )cc"); +} + +TEST_P(UncheckedStatusOrAccessModelTest, AssertOkMacro) { + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + ASSERT_OK(sor); + + sor.value(); + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(STATUSOR_INT sor) { + ASSERT_OK(sor.status()); + + sor.value(); + } + )cc"); + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + void target() { + STATUSOR_INT sor = Make(); + ASSERT_OK(sor); + + sor.value(); + } + )cc"); + + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(std::optional sor_opt) { + STATUSOR_INT sor = *sor_opt; + ASSERT_OK(sor); + + sor.value(); + } + )cc"); +} + TEST_P(UncheckedStatusOrAccessModelTest, BreadthFirstBlockTraversalLoop) { // Evaluating the CFG blocks of the code below in breadth-first order results // in an infinite loop. Each iteration of the while loop below results in a