[clang] 4ed494e - [FlowSensitive] [StatusOr] [9/N] Make sure all StatusOr are initialized

[via cfe-commits]

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