[llvm-branch-commits] [FlowSensitive] [StatusOr] [9/N] Make sure all StatusOr are initialized (PR #163898)
via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Thu Oct 16 17:28:14 PDT 2025
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
More information about the llvm-branch-commits
mailing list