Author: Florian Mayer Date: 2025-10-27T09:29:24-07:00 New Revision: 4ed494e7282c3b36a35b8e0930fd2e14b7038167
URL: https://github.com/llvm/llvm-project/commit/4ed494e7282c3b36a35b8e0930fd2e14b7038167 DIFF: https://github.com/llvm/llvm-project/commit/4ed494e7282c3b36a35b8e0930fd2e14b7038167.diff LOG: [FlowSensitive] [StatusOr] [9/N] Make sure all StatusOr are initialized This is important if the first use of a StatusOr (or Status) is in a conditional statement, we need a stable value for `ok` from outside of the conditional statement to make sure we don't use a different variable in every branch. Reviewers: jvoung, Xazax-hun Reviewed By: jvoung Pull Request: https://github.com/llvm/llvm-project/pull/163898 Added: Modified: clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp Removed: ################################################################################ diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp index c6a680d8cf252..b42bfa3821c2e 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp @@ -202,6 +202,16 @@ static auto isStatusOrValueConstructor() { "std::in_place_t")))))); } +static auto isStatusOrConstructor() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxConstructExpr(hasType(statusOrType())); +} + +static auto isStatusConstructor() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxConstructExpr(hasType(statusType())); +} + static auto buildDiagnoseMatchSwitch(const UncheckedStatusOrAccessModelOptions &Options) { return CFGMatchSwitchBuilder<const Environment, @@ -574,6 +584,25 @@ static void transferValueConstructor(const CXXConstructExpr *Expr, State.Env.assume(OkVal.formula()); } +static void transferStatusOrConstructor(const CXXConstructExpr *Expr, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + RecordStorageLocation &StatusOrLoc = State.Env.getResultObjectLocation(*Expr); + RecordStorageLocation &StatusLoc = locForStatus(StatusOrLoc); + + if (State.Env.getValue(locForOk(StatusLoc)) == nullptr) + initializeStatusOr(StatusOrLoc, State.Env); +} + +static void transferStatusConstructor(const CXXConstructExpr *Expr, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + RecordStorageLocation &StatusLoc = State.Env.getResultObjectLocation(*Expr); + + if (State.Env.getValue(locForOk(StatusLoc)) == nullptr) + initializeStatus(StatusLoc, State.Env); +} + CFGMatchSwitch<LatticeTransferState> buildTransferMatchSwitch(ASTContext &Ctx, CFGMatchSwitchBuilder<LatticeTransferState> Builder) { @@ -623,6 +652,16 @@ buildTransferMatchSwitch(ASTContext &Ctx, transferValueAssignmentCall) .CaseOfCFGStmt<CXXConstructExpr>(isStatusOrValueConstructor(), transferValueConstructor) + // N.B. These need to come after all other CXXConstructExpr. + // These are there to make sure that every Status and StatusOr object + // have their ok boolean initialized when constructed. If we were to + // lazily initialize them when we first access them, we can produce + // false positives if that first access is in a control flow statement. + // You can comment out these two constructors and see tests fail. + .CaseOfCFGStmt<CXXConstructExpr>(isStatusOrConstructor(), + transferStatusOrConstructor) + .CaseOfCFGStmt<CXXConstructExpr>(isStatusConstructor(), + transferStatusConstructor) .Build(); } diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp index 452062587ce72..5635ff4e01d36 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp @@ -3147,6 +3147,45 @@ TEST_P(UncheckedStatusOrAccessModelTest, InPlaceConstruct) { )cc"); } +TEST_P(UncheckedStatusOrAccessModelTest, ConstructStatusOrFromReference) { + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + void target() { + const auto sor1 = Make<STATUSOR_INT&>(); + const auto sor2 = Make<STATUSOR_INT&>(); + if (!sor1.ok() && !sor2.ok()) return; + if (sor1.ok() && !sor2.ok()) { + } else if (!sor1.ok() && sor2.ok()) { + } else { + sor1.value(); + sor2.value(); + } + } + )cc"); +} + +TEST_P(UncheckedStatusOrAccessModelTest, ConstructStatusFromReference) { + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + void target() { + const auto sor1 = Make<STATUSOR_INT&>(); + const auto sor2 = Make<STATUSOR_INT&>(); + const auto s1 = Make<STATUS&>(); + const auto s2 = Make<STATUS&>(); + + if (!s1.ok() && !s2.ok()) return; + if (s1.ok() && !s2.ok()) { + } else if (!s1.ok() && s2.ok()) { + } else { + if (s1 != sor1.status() || s2 != sor2.status()) return; + sor1.value(); + sor2.value(); + } + } + )cc"); +} + } // namespace std::string _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
