[StatusOr] [10/N] Add support for absl CHECK macros

[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/169749.diff

2 Files Affected:

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

diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
index b42bfa3821c2e..0a1adde7bfe9d 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
@@ -211,6 +211,31 @@ static auto isStatusConstructor() {
   using namespace ::clang::ast_matchers; // NOLINT: Too many names
   return cxxConstructExpr(hasType(statusType()));
 }
+static auto isLoggingGetReferenceableValueCall() {
+  using namespace ::clang::ast_matchers; // NOLINT: Too many names
+  return callExpr(callee(
+      functionDecl(hasName("::absl::log_internal::GetReferenceableValue"))));
+}
+
+static auto isLoggingCheckEqImpl() {
+  using namespace ::clang::ast_matchers; // NOLINT: Too many names
+  return callExpr(
+      callee(functionDecl(hasName("::absl::log_internal::Check_EQImpl"))));
+}
+
+static auto isAsStatusCallWithStatus() {
+  using namespace ::clang::ast_matchers; // NOLINT: Too many names
+  return callExpr(
+      callee(functionDecl(hasName("::absl::log_internal::AsStatus"))),
+      hasArgument(0, hasType(statusClass())));
+}
+
+static auto isAsStatusCallWithStatusOr() {
+  using namespace ::clang::ast_matchers; // NOLINT: Too many names
+  return callExpr(
+      callee(functionDecl(hasName("::absl::log_internal::AsStatus"))),
+      hasArgument(0, hasType(statusOrType())));
+}
 
 static auto
 buildDiagnoseMatchSwitch(const UncheckedStatusOrAccessModelOptions &Options) {
@@ -602,6 +627,67 @@ static void transferStatusConstructor(const CXXConstructExpr *Expr,
   if (State.Env.getValue(locForOk(StatusLoc)) == nullptr)
     initializeStatus(StatusLoc, State.Env);
 }
+static void
+transferLoggingGetReferenceableValueCall(const CallExpr *Expr,
+                                         const MatchFinder::MatchResult &,
+                                         LatticeTransferState &State) {
+  assert(Expr->getNumArgs() == 1);
+  if (Expr->getArg(0)->isPRValue())
+    return;
+  auto *ArgLoc = State.Env.getStorageLocation(*Expr->getArg(0));
+  if (ArgLoc == nullptr)
+    return;
+
+  State.Env.setStorageLocation(*Expr, *ArgLoc);
+}
+
+static void transferLoggingCheckEqImpl(const CallExpr *Expr,
+                                       const MatchFinder::MatchResult &,
+                                       LatticeTransferState &State) {
+  assert(Expr->getNumArgs() > 2);
+
+  auto *EqVal = evaluateEquality(Expr->getArg(0), Expr->getArg(1), State.Env);
+  if (EqVal == nullptr)
+    return;
+
+  // TODO(sgatev): Model pointer nullability more accurately instead of
+  // assigning BoolValue as the value of an expression of pointer type.
+  State.Env.setValue(*Expr, State.Env.makeNot(*EqVal));
+}
+
+static void transferAsStatusCallWithStatus(const CallExpr *Expr,
+                                           const MatchFinder::MatchResult &,
+                                           LatticeTransferState &State) {
+  assert(Expr->getNumArgs() == 1);
+
+  auto *ArgLoc = State.Env.get<RecordStorageLocation>(*Expr->getArg(0));
+  if (ArgLoc == nullptr)
+    return;
+
+  if (State.Env.getValue(locForOk(*ArgLoc)) == nullptr)
+    initializeStatus(*ArgLoc, State.Env);
+
+  auto &ExprVal = State.Env.create<PointerValue>(*ArgLoc);
+  State.Env.setValue(*Expr, ExprVal);
+}
+
+static void transferAsStatusCallWithStatusOr(const CallExpr *Expr,
+                                             const MatchFinder::MatchResult &,
+                                             LatticeTransferState &State) {
+  assert(Expr->getNumArgs() == 1);
+
+  auto *ArgLoc = State.Env.get<RecordStorageLocation>(*Expr->getArg(0));
+  if (ArgLoc == nullptr)
+    return;
+
+  RecordStorageLocation &StatusLoc = locForStatus(*ArgLoc);
+
+  if (State.Env.getValue(locForOk(StatusLoc)) == nullptr)
+    initializeStatusOr(*ArgLoc, State.Env);
+
+  auto &ExprVal = State.Env.create<PointerValue>(StatusLoc);
+  State.Env.setValue(*Expr, ExprVal);
+}
 
 CFGMatchSwitch<LatticeTransferState>
 buildTransferMatchSwitch(ASTContext &Ctx,
@@ -652,6 +738,14 @@ buildTransferMatchSwitch(ASTContext &Ctx,
                                           transferValueAssignmentCall)
       .CaseOfCFGStmt<CXXConstructExpr>(isStatusOrValueConstructor(),
                                        transferValueConstructor)
+      .CaseOfCFGStmt<CallExpr>(isAsStatusCallWithStatus(),
+                               transferAsStatusCallWithStatus)
+      .CaseOfCFGStmt<CallExpr>(isAsStatusCallWithStatusOr(),
+                               transferAsStatusCallWithStatusOr)
+      .CaseOfCFGStmt<CallExpr>(isLoggingGetReferenceableValueCall(),
+                               transferLoggingGetReferenceableValueCall)
+      .CaseOfCFGStmt<CallExpr>(isLoggingCheckEqImpl(),
+                               transferLoggingCheckEqImpl)
       // 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
@@ -662,6 +756,14 @@ buildTransferMatchSwitch(ASTContext &Ctx,
                                        transferStatusOrConstructor)
       .CaseOfCFGStmt<CXXConstructExpr>(isStatusConstructor(),
                                        transferStatusConstructor)
+      .CaseOfCFGStmt<ImplicitCastExpr>(
+          implicitCastExpr(hasCastKind(CK_PointerToBoolean)),
+          [](const ImplicitCastExpr *Expr, const MatchFinder::MatchResult &,
+             LatticeTransferState &State) {
+            if (auto *SubExprVal = dyn_cast_or_null<BoolValue>(
+                    State.Env.getValue(*Expr->getSubExpr())))
+              State.Env.setValue(*Expr, *SubExprVal);
+          })
       .Build();
 }
 
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
index 5635ff4e01d36..13cde05df0a3f 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
@@ -1725,6 +1725,90 @@ TEST_P(UncheckedStatusOrAccessModelTest, QcheckMacro) {
   )cc");
 }
 
+TEST_P(UncheckedStatusOrAccessModelTest, CheckOkMacro) {
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor) {
+      CHECK_OK(sor.status());
+      sor.value();
+    }
+  )cc");
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor) {
+      CHECK_OK(sor);
+      sor.value();
+    }
+  )cc");
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target() {
+      STATUS s = Make<STATUS>();
+      CHECK_OK(s);
+    }
+  )cc");
+}
+
+TEST_P(UncheckedStatusOrAccessModelTest, QcheckOkMacro) {
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor) {
+      QCHECK_OK(sor.status());
+      sor.value();
+    }
+  )cc");
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor) {
+      QCHECK_OK(sor);
+      sor.value();
+    }
+  )cc");
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target() {
+      STATUS s = Make<STATUS>();
+      QCHECK_OK(s);
+    }
+  )cc");
+}
+
+TEST_P(UncheckedStatusOrAccessModelTest, CheckEqMacro) {
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor) {
+      CHECK_EQ(sor.status(), absl::OkStatus());
+      sor.value();
+    }
+  )cc");
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target() {
+      CHECK_EQ(Make<STATUS>(), absl::OkStatus());
+      CHECK_EQ(absl::OkStatus(), Make<STATUS>());
+    }
+  )cc");
+}
+
+TEST_P(UncheckedStatusOrAccessModelTest, QcheckEqMacro) {
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor) {
+      QCHECK_EQ(sor.status(), absl::OkStatus());
+      sor.value();
+    }
+  )cc");
+}
+
 TEST_P(UncheckedStatusOrAccessModelTest, CheckNeMacro) {
   ExpectDiagnosticsFor(R"cc(
 #include "unchecked_statusor_access_test_defs.h"

[jvoung]

Should the TODO be updated? (not sgatev ?)

Might also be good to explain a bit more what's going on here, e.g., to explain why the makeNot (the EQImpl returns a char* and when equal is ... nullptr?)

[fmayer]

done. explained better and did some minor restructuring to make it easier to explain (moved the cast transfer function out of line)