llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Florian Mayer (fmayer) <details> <summary>Changes</summary> --- Full diff: https://github.com/llvm/llvm-project/pull/170947.diff 2 Files Affected: - (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp (+221) - (modified) clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp (+175) ``````````diff diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp index 3e56094fcbc32..1b68d704239e8 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<const Environment, @@ -365,6 +435,26 @@ bool isStatusType(QualType Type) { return isTypeNamed(Type, {"absl"}, "Status"); } +static bool isPredicateFormatterFromMatcherType(QualType Type) { + return isTypeNamed(Type, {"testing", "internal"}, + "PredicateFormatterFromMatcher"); +} + +static bool isAssertionResultType(QualType Type) { + return isTypeNamed(Type, {"testing"}, "AssertionResult"); +} + +static bool isStatusIsMatcherType(QualType Type) { + return isTypeNamed(Type, {"testing", "status", "internal_status"}, + "StatusIsMatcher") || + isTypeNamed(Type, {"testing", "status", "internal_status"}, + "CanonicalStatusIsMatcher") || + isTypeNamed(Type, {"absl_testing", "status_internal"}, + "StatusIsMatcher") || + isTypeNamed(Type, {"absl_testing", "status_internal"}, + "CanonicalStatusIsMatcher"); +} + llvm::StringMap<QualType> getSyntheticFields(QualType Ty, QualType StatusType, const CXXRecordDecl &RD) { if (auto *TRD = getStatusOrBaseClass(Ty)) @@ -372,6 +462,12 @@ llvm::StringMap<QualType> 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 &, @@ -843,6 +946,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<BoolValue>(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<RecordStorageLocation>(*Expr->getArg(0)); + if (Loc == nullptr) + return; + + auto *ObjectLoc = State.Env.get<RecordStorageLocation>(*Expr->getArg(2)); + if (ObjectLoc == nullptr) + return; + + auto &OkPredicateLoc = locForOkPredicate(*Loc); + BoolValue *OkPredicateVal = State.Env.get<BoolValue>(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<StorageLocation>(*Expr->getArg(0)); + if (StatusAdaptorLoc == nullptr) + return; + BoolValue *OkVal = State.Env.get<BoolValue>(*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<BoolValue>(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()) @@ -858,6 +1052,33 @@ buildTransferMatchSwitch(ASTContext &Ctx, CFGMatchSwitchBuilder<LatticeTransferState> Builder) { using namespace ::clang::ast_matchers; // NOLINT: Too many names return std::move(Builder) + .CaseOfCFGStmt<CallExpr>( + isMakePredicateFormatterFromIsOkMatcherCall(), + transferMakePredicateFormatterFromIsOkMatcherCall) + .CaseOfCFGStmt<CallExpr>(isStatusIsOkMatcherCall(), + transferStatusIsOkMatcherCall) + .CaseOfCFGStmt<CallExpr>( + isMakePredicateFormatterFromStatusIsMatcherCall(), + transferMakePredicateFormatterFromStatusIsMatcherCall) + .CaseOfCFGStmt<CXXOperatorCallExpr>( + isPredicateFormatterFromStatusOrMatcherCall(), + [](const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &, + LatticeTransferState &State) { + transferPredicateFormatterMatcherCall(Expr, State, + /*IsStatusOr=*/true); + }) + .CaseOfCFGStmt<CXXOperatorCallExpr>( + isPredicateFormatterFromStatusMatcherCall(), + [](const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &, + LatticeTransferState &State) { + transferPredicateFormatterMatcherCall(Expr, State, + /*IsStatusOr=*/false); + }) + .CaseOfCFGStmt<CXXConstructExpr>( + isAssertionResultConstructFromBoolCall(), + transferAssertionResultConstructFromBoolCall) + .CaseOfCFGStmt<CXXMemberCallExpr>(isAssertionResultOperatorBoolCall(), + transferAssertionResultOperatorBoolCall) .CaseOfCFGStmt<CXXMemberCallExpr>(isStatusOrMemberCallWithName("ok"), transferStatusOrOkCall) .CaseOfCFGStmt<CXXMemberCallExpr>(isStatusOrMemberCallWithName("status"), diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp index dcb1cc13146bd..48e61abf09f19 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<STATUSOR_INT>(); + 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<STATUSOR_INT>(); + ASSERT_OK(sor); + + sor.value(); + } + )cc"); + + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target(std::optional<STATUSOR_INT> 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 `````````` </details> https://github.com/llvm/llvm-project/pull/170947 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
