[FlowSensitive] [StatusOr] [6/N] Support pointer comparison

[fmayer]

No description provided.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Florian Mayer (fmayer)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/163875.diff

2 Files Affected:

• (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp (+80)
• (modified) clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp (+57)

diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
index 76f93d5a88c3e..dc0a8f7e4cfd6 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
@@ -146,6 +146,20 @@ static auto isNotOkStatusCall() {
       "::absl::UnimplementedError", "::absl::UnknownError"))));
 }
 
+static auto isPointerComparisonOperatorCall(std::string operator_name) {
+  using namespace ::clang::ast_matchers; // NOLINT: Too many names
+  return binaryOperator(
+      hasOperatorName(operator_name),
+      hasLHS(
+          anyOf(hasType(hasCanonicalType(pointerType(pointee(statusOrType())))),
+                hasType(hasCanonicalType(
+                    pointerType(pointee(possiblyAliasedStatusType())))))),
+      hasRHS(
+          anyOf(hasType(hasCanonicalType(pointerType(pointee(statusOrType())))),
+                hasType(hasCanonicalType(
+                    pointerType(pointee(possiblyAliasedStatusType())))))));
+}
+
 static auto
 buildDiagnoseMatchSwitch(const UncheckedStatusOrAccessModelOptions &Options) {
   return CFGMatchSwitchBuilder<const Environment,
@@ -433,6 +447,58 @@ static void transferComparisonOperator(const CXXOperatorCallExpr *Expr,
     State.Env.setValue(*Expr, *LhsAndRhsVal);
 }
 
+static RecordStorageLocation *getPointeeLocation(const Expr &Expr,
+                                                 Environment &Env) {
+  if (auto *PointerVal = Env.get<PointerValue>(Expr))
+    return dyn_cast<RecordStorageLocation>(&PointerVal->getPointeeLoc());
+  return nullptr;
+}
+
+static BoolValue *evaluatePointerEquality(const Expr *LhsExpr,
+                                          const Expr *RhsExpr,
+                                          Environment &Env) {
+  assert(LhsExpr->getType()->isPointerType());
+  assert(RhsExpr->getType()->isPointerType());
+  RecordStorageLocation *LhsStatusLoc = nullptr;
+  RecordStorageLocation *RhsStatusLoc = nullptr;
+  if (isStatusOrType(LhsExpr->getType()->getPointeeType()) &&
+      isStatusOrType(RhsExpr->getType()->getPointeeType())) {
+    auto *LhsStatusOrLoc = getPointeeLocation(*LhsExpr, Env);
+    auto *RhsStatusOrLoc = getPointeeLocation(*RhsExpr, Env);
+    if (LhsStatusOrLoc == nullptr || RhsStatusOrLoc == nullptr)
+      return nullptr;
+    LhsStatusLoc = &locForStatus(*LhsStatusOrLoc);
+    RhsStatusLoc = &locForStatus(*RhsStatusOrLoc);
+  } else if (isStatusType(LhsExpr->getType()->getPointeeType()) &&
+             isStatusType(RhsExpr->getType()->getPointeeType())) {
+    LhsStatusLoc = getPointeeLocation(*LhsExpr, Env);
+    RhsStatusLoc = getPointeeLocation(*RhsExpr, Env);
+  }
+  if (LhsStatusLoc == nullptr || RhsStatusLoc == nullptr)
+    return nullptr;
+  auto &LhsOkVal = valForOk(*LhsStatusLoc, Env);
+  auto &RhsOkVal = valForOk(*RhsStatusLoc, Env);
+  auto &Res = Env.makeAtomicBoolValue();
+  auto &A = Env.arena();
+  Env.assume(A.makeImplies(
+      Res.formula(), A.makeEquals(LhsOkVal.formula(), RhsOkVal.formula())));
+  return &Res;
+}
+
+static void transferPointerComparisonOperator(const BinaryOperator *Expr,
+                                              LatticeTransferState &State,
+                                              bool IsNegative) {
+  auto *LhsAndRhsVal =
+      evaluatePointerEquality(Expr->getLHS(), Expr->getRHS(), State.Env);
+  if (LhsAndRhsVal == nullptr)
+    return;
+
+  if (IsNegative)
+    State.Env.setValue(*Expr, State.Env.makeNot(*LhsAndRhsVal));
+  else
+    State.Env.setValue(*Expr, *LhsAndRhsVal);
+}
+
 static void transferOkStatusCall(const CallExpr *Expr,
                                  const MatchFinder::MatchResult &,
                                  LatticeTransferState &State) {
@@ -477,6 +543,20 @@ buildTransferMatchSwitch(ASTContext &Ctx,
             transferComparisonOperator(Expr, State,
                                        /*IsNegative=*/true);
           })
+      .CaseOfCFGStmt<BinaryOperator>(
+          isPointerComparisonOperatorCall("=="),
+          [](const BinaryOperator *Expr, const MatchFinder::MatchResult &,
+             LatticeTransferState &State) {
+            transferPointerComparisonOperator(Expr, State,
+                                              /*IsNegative=*/false);
+          })
+      .CaseOfCFGStmt<BinaryOperator>(
+          isPointerComparisonOperatorCall("!="),
+          [](const BinaryOperator *Expr, const MatchFinder::MatchResult &,
+             LatticeTransferState &State) {
+            transferPointerComparisonOperator(Expr, State,
+                                              /*IsNegative=*/true);
+          })
       .CaseOfCFGStmt<CallExpr>(isOkStatusCall(), transferOkStatusCall)
       .CaseOfCFGStmt<CallExpr>(isNotOkStatusCall(), transferNotOkStatusCall)
       .Build();
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
index 2676dab7fd904..62e456ad07bdb 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
@@ -2858,6 +2858,63 @@ TEST_P(UncheckedStatusOrAccessModelTest, EqualityCheck) {
   )cc");
 }
 
+TEST_P(UncheckedStatusOrAccessModelTest, PointerEqualityCheck) {
+  ExpectDiagnosticsFor(
+      R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+        void target(STATUSOR_INT* x, STATUSOR_INT* y) {
+          if (x->ok()) {
+            if (x == y)
+              y->value();
+            else
+              y->value();  // [[unsafe]]
+          }
+        }
+      )cc");
+  ExpectDiagnosticsFor(
+      R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+        void target(STATUSOR_INT* x, STATUSOR_INT* y) {
+          if (x->ok()) {
+            if (x != y)
+              y->value();  // [[unsafe]]
+            else
+              y->value();
+          }
+        }
+      )cc");
+  ExpectDiagnosticsFor(
+      R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+        void target(STATUS* x, STATUS* y) {
+          auto sor = Make<STATUSOR_INT>();
+          if (x->ok()) {
+            if (x == y && sor.status() == *y)
+              sor.value();
+            else
+              sor.value();  // [[unsafe]]
+          }
+        }
+      )cc");
+  ExpectDiagnosticsFor(
+      R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+        void target(STATUS* x, STATUS* y) {
+          auto sor = Make<STATUSOR_INT>();
+          if (x->ok()) {
+            if (x != y)
+              sor.value();  // [[unsafe]]
+            else if (sor.status() == *y)
+              sor.value();
+          }
+        }
+      )cc");
+}
+
 } // namespace
 
 std::string

[fmayer]

@BaLiKfromUA CC

[jvoung]

otherwise LGTM

[jvoung]

can the anyOf be deeper for just the (statusOrType, statusType) part? (if that helps matching performance)?

[fmayer]

done