llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Florian Mayer (fmayer)
<details>
<summary>Changes</summary>
`absl::StatusOr::status` allows extraction of the status associated with
a StatusOr value. That can also be used to check whether the StatusOr
has a value or not.
This makes sure code like this is checked properly:
```cpp
int target(absl::StatusOr<int> SOR) {
if (SOR.status().ok())
return *SOR;
return 0;
}
```
---
Full diff: https://github.com/llvm/llvm-project/pull/163868.diff
2 Files Affected:
- (modified)
clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp (+72)
- (modified)
clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
(+148)
``````````diff
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
index 9ea962abc24ee..11c4ad90293d9 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
@@ -24,6 +24,7 @@
#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/RecordOps.h"
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
#include "clang/Analysis/FlowSensitive/Value.h"
#include "clang/Basic/LLVM.h"
@@ -73,6 +74,18 @@ static QualType
getStatusOrValueType(ClassTemplateSpecializationDecl *TRD) {
return TRD->getTemplateArgs().get(0).getAsType();
}
+static auto ofClassStatus() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return ofClass(hasName("::absl::Status"));
+}
+
+static auto isStatusMemberCallWithName(llvm::StringRef member_name) {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return cxxMemberCallExpr(
+ on(expr(unless(cxxThisExpr()))),
+ callee(cxxMethodDecl(hasName(member_name), ofClassStatus())));
+}
+
static auto isStatusOrMemberCallWithName(llvm::StringRef member_name) {
using namespace ::clang::ast_matchers; // NOLINT: Too many names
return cxxMemberCallExpr(
@@ -238,6 +251,59 @@ static void transferStatusOrOkCall(const CXXMemberCallExpr
*Expr,
State.Env.setValue(*Expr, OkVal);
}
+static void transferStatusCall(const CXXMemberCallExpr *Expr,
+ const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ RecordStorageLocation *StatusOrLoc =
+ getImplicitObjectLocation(*Expr, State.Env);
+ if (StatusOrLoc == nullptr)
+ return;
+
+ RecordStorageLocation &StatusLoc = locForStatus(*StatusOrLoc);
+
+ if (State.Env.getValue(locForOk(StatusLoc)) == nullptr)
+ initializeStatusOr(*StatusOrLoc, State.Env);
+
+ if (Expr->isPRValue())
+ copyRecord(StatusLoc, State.Env.getResultObjectLocation(*Expr), State.Env);
+ else
+ State.Env.setStorageLocation(*Expr, StatusLoc);
+}
+
+static void transferStatusOkCall(const CXXMemberCallExpr *Expr,
+ const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ RecordStorageLocation *StatusLoc =
+ getImplicitObjectLocation(*Expr, State.Env);
+ if (StatusLoc == nullptr)
+ return;
+
+ if (Value *Val = State.Env.getValue(locForOk(*StatusLoc)))
+ State.Env.setValue(*Expr, *Val);
+}
+
+static void transferStatusUpdateCall(const CXXMemberCallExpr *Expr,
+ const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ assert(Expr->getNumArgs() == 1);
+ auto *Arg = Expr->getArg(0);
+ RecordStorageLocation *ArgRecord =
+ Arg->isPRValue() ? &State.Env.getResultObjectLocation(*Arg)
+ : State.Env.get<RecordStorageLocation>(*Arg);
+ RecordStorageLocation *ThisLoc = getImplicitObjectLocation(*Expr, State.Env);
+ if (ThisLoc == nullptr || ArgRecord == nullptr)
+ return;
+
+ auto &ThisOkVal = valForOk(*ThisLoc, State.Env);
+ auto &ArgOkVal = valForOk(*ArgRecord, State.Env);
+ auto &A = State.Env.arena();
+ auto &NewVal = State.Env.makeAtomicBoolValue();
+ State.Env.assume(A.makeImplies(A.makeNot(ThisOkVal.formula()),
+ A.makeNot(NewVal.formula())));
+ State.Env.assume(A.makeImplies(NewVal.formula(), ArgOkVal.formula()));
+ State.Env.setValue(locForOk(*ThisLoc), NewVal);
+}
+
CFGMatchSwitch<LatticeTransferState>
buildTransferMatchSwitch(ASTContext &Ctx,
CFGMatchSwitchBuilder<LatticeTransferState> Builder) {
@@ -245,6 +311,12 @@ buildTransferMatchSwitch(ASTContext &Ctx,
return std::move(Builder)
.CaseOfCFGStmt<CXXMemberCallExpr>(isStatusOrMemberCallWithName("ok"),
transferStatusOrOkCall)
+ .CaseOfCFGStmt<CXXMemberCallExpr>(isStatusOrMemberCallWithName("status"),
+ transferStatusCall)
+ .CaseOfCFGStmt<CXXMemberCallExpr>(isStatusMemberCallWithName("ok"),
+ transferStatusOkCall)
+ .CaseOfCFGStmt<CXXMemberCallExpr>(isStatusMemberCallWithName("Update"),
+ transferStatusUpdateCall)
.Build();
}
diff --git
a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
index 4827cc1d0a7e9..d6e326cae11fd 100644
---
a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
+++
b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
@@ -2453,6 +2453,154 @@ TEST_P(UncheckedStatusOrAccessModelTest,
SubclassOperator) {
)cc");
}
+TEST_P(UncheckedStatusOrAccessModelTest, UnwrapValueWithStatusCheck) {
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ if (sor.status().ok())
+ sor.value();
+ else
+ sor.value(); // [[unsafe]]
+ }
+ )cc");
+}
+
+TEST_P(UncheckedStatusOrAccessModelTest, UnwrapValueWithStatusRefCheck) {
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ const STATUS& s = sor.status();
+ if (s.ok())
+ sor.value();
+ else
+ sor.value(); // [[unsafe]]
+ }
+ )cc");
+}
+
+TEST_P(UncheckedStatusOrAccessModelTest, UnwrapValueWithStatusPtrCheck) {
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ const STATUS* s = &sor.status();
+ if (s->ok())
+ sor.value();
+ else
+ sor.value(); // [[unsafe]]
+ }
+ )cc");
+}
+
+TEST_P(UncheckedStatusOrAccessModelTest, MembersUsedInsideStatus) {
+ ExpectDiagnosticsFor(R"cc(
+ namespace absl {
+
+ class Status {
+ public:
+ bool ok() const;
+
+ void target() const { ok(); }
+ };
+
+ } // namespace absl
+ )cc");
+}
+
+TEST_P(UncheckedStatusOrAccessModelTest, StatusUpdate) {
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ STATUS s;
+ s.Update(sor.status());
+ if (s.ok())
+ sor.value();
+ else
+ sor.value(); // [[unsafe]]
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
+ STATUS s;
+ s.Update(sor1.status());
+ s.Update(sor2.status());
+ if (s.ok()) {
+ sor1.value();
+ sor2.value();
+ }
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
+ STATUS s;
+ s.Update(sor1.status());
+ CHECK(s.ok());
+ s.Update(sor2.status());
+ sor1.value();
+ sor2.value(); // [[unsafe]]
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
+ STATUS s;
+ s.Update(sor1.status());
+ CHECK(s.ok());
+ sor1.value();
+ sor2.value(); // [[unsafe]]
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
+ STATUS s;
+ STATUS sor1_status = sor1.status();
+ s.Update(std::move(sor1_status));
+ CHECK(s.ok());
+ sor1.value();
+ sor2.value(); // [[unsafe]]
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
+ STATUS s;
+ STATUS sor1_status = sor1.status();
+ sor1_status.Update(sor2.status());
+ s.Update(std::move(sor1_status));
+ CHECK(s.ok());
+ sor1.value();
+ sor2.value();
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ const STATUS& OptStatus();
+
+ void target(STATUSOR_INT sor) {
+ auto s = sor.status();
+ s.Update(OptStatus());
+ if (s.ok()) sor.value();
+ }
+ )cc");
+}
+
} // namespace
std::string
``````````
</details>
https://github.com/llvm/llvm-project/pull/163868
_______________________________________________
llvm-branch-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
