llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Florian Mayer (fmayer) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/163898.diff 2 Files Affected: - (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp (+39) - (modified) clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp (+160) ``````````diff diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp index 542c35433d3de..9e8d6210cf1d2 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp @@ -181,6 +181,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, @@ -570,6 +580,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) { @@ -619,6 +648,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 1a7aba0aa6ca5..ee700f706fc53 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp @@ -3135,6 +3135,166 @@ 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"); +} + +/* + +TEST_P(UncheckedStatusOrAccessModelTest, OperatorIndex) { + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + class Foo { + public: + const STATUSOR_INT& operator[](size_t n) const; + STATUSOR_INT& operator[](size_t n); + }; + + void target() { + Foo foo; + const auto sor1 = foo[0]; + const auto sor2 = foo[1]; + 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, OperatorAt) { + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + class Foo { + public: + const STATUSOR_INT& at(size_t n) const; + STATUSOR_INT& at(size_t n); + }; + + void target() { + Foo foo; + const auto sor1 = foo.at(0); + const auto sor2 = foo.at(1); + 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, OperatorIndexWithStatus) { + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + class Foo { + public: + const STATUSOR_INT& operator[](size_t n) const; + STATUSOR_INT& operator[](size_t n); + }; + class Bar { + public: + const STATUS& operator[](size_t n) const; + STATUS& operator[](size_t n); + }; + + void target() { + Foo foo; + const auto sor1 = foo[0]; + const auto sor2 = foo[1]; + Bar bar; + const auto s1 = bar[0]; + const auto s2 = bar[1]; + 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"); +} +TEST_P(UncheckedStatusOrAccessModelTest, OperatorAtWithStatus) { + ExpectDiagnosticsFor(R"cc( +#include "unchecked_statusor_access_test_defs.h" + + class Foo { + public: + const STATUSOR_INT& at(size_t n) const; + STATUSOR_INT& at(size_t n); + }; + + class Bar { + public: + const STATUS& at(size_t n) const; + STATUS& at(size_t n); + }; + + void target() { + Foo foo; + const auto sor1 = foo.at(0); + const auto sor2 = foo.at(1); + Bar bar; + const auto s1 = bar.at(0); + const auto s2 = bar.at(1); + 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 `````````` </details> https://github.com/llvm/llvm-project/pull/163898 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
