Skip to content

Commit 5245d7d

Browse files
fmayeraokblast
fmayer
authored and
aokblast
committed
[FlowSensitive] [StatusOr] [3/N] Support absl::Status ops (llvm#163868)
`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; } ```
1 parent b9816e0 commit 5245d7d

File tree

2 files changed

+235
-0
lines changed

2 files changed

+235
-0
lines changed

‎clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
2525
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
2626
#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
27+
#include "clang/Analysis/FlowSensitive/RecordOps.h"
2728
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
2829
#include "clang/Analysis/FlowSensitive/Value.h"
2930
#include "clang/Basic/LLVM.h"
@@ -95,6 +96,18 @@ static QualType getStatusOrValueType(ClassTemplateSpecializationDecl *TRD) {
9596
return TRD->getTemplateArgs().get(0).getAsType();
9697
}
9798

99+
static auto ofClassStatus() {
100+
using namespace ::clang::ast_matchers; // NOLINT: Too many names
101+
return ofClass(hasName("::absl::Status"));
102+
}
103+
104+
static auto isStatusMemberCallWithName(llvm::StringRef member_name) {
105+
using namespace ::clang::ast_matchers; // NOLINT: Too many names
106+
return cxxMemberCallExpr(
107+
on(expr(unless(cxxThisExpr()))),
108+
callee(cxxMethodDecl(hasName(member_name), ofClassStatus())));
109+
}
110+
98111
static auto isStatusOrMemberCallWithName(llvm::StringRef member_name) {
99112
using namespace ::clang::ast_matchers; // NOLINT: Too many names
100113
return cxxMemberCallExpr(
@@ -244,13 +257,74 @@ static void transferStatusOrOkCall(const CXXMemberCallExpr *Expr,
244257
State.Env.setValue(*Expr, OkVal);
245258
}
246259

260+
static void transferStatusCall(const CXXMemberCallExpr *Expr,
261+
const MatchFinder::MatchResult &,
262+
LatticeTransferState &State) {
263+
RecordStorageLocation *StatusOrLoc =
264+
getImplicitObjectLocation(*Expr, State.Env);
265+
if (StatusOrLoc == nullptr)
266+
return;
267+
268+
RecordStorageLocation &StatusLoc = locForStatus(*StatusOrLoc);
269+
270+
if (State.Env.getValue(locForOk(StatusLoc)) == nullptr)
271+
initializeStatusOr(*StatusOrLoc, State.Env);
272+
273+
if (Expr->isPRValue())
274+
copyRecord(StatusLoc, State.Env.getResultObjectLocation(*Expr), State.Env);
275+
else
276+
State.Env.setStorageLocation(*Expr, StatusLoc);
277+
}
278+
279+
static void transferStatusOkCall(const CXXMemberCallExpr *Expr,
280+
const MatchFinder::MatchResult &,
281+
LatticeTransferState &State) {
282+
RecordStorageLocation *StatusLoc =
283+
getImplicitObjectLocation(*Expr, State.Env);
284+
if (StatusLoc == nullptr)
285+
return;
286+
287+
if (Value *Val = State.Env.getValue(locForOk(*StatusLoc)))
288+
State.Env.setValue(*Expr, *Val);
289+
}
290+
291+
static void transferStatusUpdateCall(const CXXMemberCallExpr *Expr,
292+
const MatchFinder::MatchResult &,
293+
LatticeTransferState &State) {
294+
// S.Update(OtherS) sets S to the error code of OtherS if it is OK,
295+
// otherwise does nothing.
296+
assert(Expr->getNumArgs() == 1);
297+
auto *Arg = Expr->getArg(0);
298+
RecordStorageLocation *ArgRecord =
299+
Arg->isPRValue() ? &State.Env.getResultObjectLocation(*Arg)
300+
: State.Env.get<RecordStorageLocation>(*Arg);
301+
RecordStorageLocation *ThisLoc = getImplicitObjectLocation(*Expr, State.Env);
302+
if (ThisLoc == nullptr || ArgRecord == nullptr)
303+
return;
304+
305+
auto &ThisOkVal = valForOk(*ThisLoc, State.Env);
306+
auto &ArgOkVal = valForOk(*ArgRecord, State.Env);
307+
auto &A = State.Env.arena();
308+
auto &NewVal = State.Env.makeAtomicBoolValue();
309+
State.Env.assume(A.makeImplies(A.makeNot(ThisOkVal.formula()),
310+
A.makeNot(NewVal.formula())));
311+
State.Env.assume(A.makeImplies(NewVal.formula(), ArgOkVal.formula()));
312+
State.Env.setValue(locForOk(*ThisLoc), NewVal);
313+
}
314+
247315
CFGMatchSwitch<LatticeTransferState>
248316
buildTransferMatchSwitch(ASTContext &Ctx,
249317
CFGMatchSwitchBuilder<LatticeTransferState> Builder) {
250318
using namespace ::clang::ast_matchers; // NOLINT: Too many names
251319
return std::move(Builder)
252320
.CaseOfCFGStmt<CXXMemberCallExpr>(isStatusOrMemberCallWithName("ok"),
253321
transferStatusOrOkCall)
322+
.CaseOfCFGStmt<CXXMemberCallExpr>(isStatusOrMemberCallWithName("status"),
323+
transferStatusCall)
324+
.CaseOfCFGStmt<CXXMemberCallExpr>(isStatusMemberCallWithName("ok"),
325+
transferStatusOkCall)
326+
.CaseOfCFGStmt<CXXMemberCallExpr>(isStatusMemberCallWithName("Update"),
327+
transferStatusUpdateCall)
254328
.Build();
255329
}
256330

‎clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2453,6 +2453,167 @@ TEST_P(UncheckedStatusOrAccessModelTest, SubclassOperator) {
24532453
)cc");
24542454
}
24552455

2456+
TEST_P(UncheckedStatusOrAccessModelTest, UnwrapValueWithStatusCheck) {
2457+
ExpectDiagnosticsFor(R"cc(
2458+
#include "unchecked_statusor_access_test_defs.h"
2459+
2460+
void target(STATUSOR_INT sor) {
2461+
if (sor.status().ok())
2462+
sor.value();
2463+
else
2464+
sor.value(); // [[unsafe]]
2465+
}
2466+
)cc");
2467+
}
2468+
2469+
TEST_P(UncheckedStatusOrAccessModelTest, UnwrapValueWithStatusRefCheck) {
2470+
ExpectDiagnosticsFor(R"cc(
2471+
#include "unchecked_statusor_access_test_defs.h"
2472+
2473+
void target(STATUSOR_INT sor) {
2474+
const STATUS& s = sor.status();
2475+
if (s.ok())
2476+
sor.value();
2477+
else
2478+
sor.value(); // [[unsafe]]
2479+
}
2480+
)cc");
2481+
}
2482+
2483+
TEST_P(UncheckedStatusOrAccessModelTest, UnwrapValueWithStatusPtrCheck) {
2484+
ExpectDiagnosticsFor(R"cc(
2485+
#include "unchecked_statusor_access_test_defs.h"
2486+
2487+
void target(STATUSOR_INT sor) {
2488+
const STATUS* s = &sor.status();
2489+
if (s->ok())
2490+
sor.value();
2491+
else
2492+
sor.value(); // [[unsafe]]
2493+
}
2494+
)cc");
2495+
}
2496+
2497+
TEST_P(UncheckedStatusOrAccessModelTest, UnwrapValueWithMovedStatus) {
2498+
ExpectDiagnosticsFor(R"cc(
2499+
#include "unchecked_statusor_access_test_defs.h"
2500+
2501+
void target(STATUSOR_INT sor) {
2502+
if (std::move(sor.status()).ok())
2503+
sor.value();
2504+
else
2505+
sor.value(); // [[unsafe]]
2506+
}
2507+
)cc");
2508+
}
2509+
2510+
TEST_P(UncheckedStatusOrAccessModelTest, MembersUsedInsideStatus) {
2511+
ExpectDiagnosticsFor(R"cc(
2512+
namespace absl {
2513+
2514+
class Status {
2515+
public:
2516+
bool ok() const;
2517+
2518+
void target() const { ok(); }
2519+
};
2520+
2521+
} // namespace absl
2522+
)cc");
2523+
}
2524+
2525+
TEST_P(UncheckedStatusOrAccessModelTest, StatusUpdate) {
2526+
ExpectDiagnosticsFor(R"cc(
2527+
#include "unchecked_statusor_access_test_defs.h"
2528+
2529+
void target(STATUSOR_INT sor) {
2530+
STATUS s;
2531+
s.Update(sor.status());
2532+
if (s.ok())
2533+
sor.value();
2534+
else
2535+
sor.value(); // [[unsafe]]
2536+
}
2537+
)cc");
2538+
2539+
ExpectDiagnosticsFor(R"cc(
2540+
#include "unchecked_statusor_access_test_defs.h"
2541+
2542+
void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
2543+
STATUS s;
2544+
s.Update(sor1.status());
2545+
s.Update(sor2.status());
2546+
if (s.ok()) {
2547+
sor1.value();
2548+
sor2.value();
2549+
}
2550+
}
2551+
)cc");
2552+
2553+
ExpectDiagnosticsFor(R"cc(
2554+
#include "unchecked_statusor_access_test_defs.h"
2555+
2556+
void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
2557+
STATUS s;
2558+
s.Update(sor1.status());
2559+
CHECK(s.ok());
2560+
s.Update(sor2.status());
2561+
sor1.value();
2562+
sor2.value(); // [[unsafe]]
2563+
}
2564+
)cc");
2565+
2566+
ExpectDiagnosticsFor(R"cc(
2567+
#include "unchecked_statusor_access_test_defs.h"
2568+
2569+
void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
2570+
STATUS s;
2571+
s.Update(sor1.status());
2572+
CHECK(s.ok());
2573+
sor1.value();
2574+
sor2.value(); // [[unsafe]]
2575+
}
2576+
)cc");
2577+
2578+
ExpectDiagnosticsFor(R"cc(
2579+
#include "unchecked_statusor_access_test_defs.h"
2580+
2581+
void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
2582+
STATUS s;
2583+
STATUS sor1_status = sor1.status();
2584+
s.Update(std::move(sor1_status));
2585+
CHECK(s.ok());
2586+
sor1.value();
2587+
sor2.value(); // [[unsafe]]
2588+
}
2589+
)cc");
2590+
2591+
ExpectDiagnosticsFor(R"cc(
2592+
#include "unchecked_statusor_access_test_defs.h"
2593+
2594+
void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
2595+
STATUS s;
2596+
STATUS sor1_status = sor1.status();
2597+
sor1_status.Update(sor2.status());
2598+
s.Update(std::move(sor1_status));
2599+
CHECK(s.ok());
2600+
sor1.value();
2601+
sor2.value();
2602+
}
2603+
)cc");
2604+
ExpectDiagnosticsFor(R"cc(
2605+
#include "unchecked_statusor_access_test_defs.h"
2606+
2607+
const STATUS& OptStatus();
2608+
2609+
void target(STATUSOR_INT sor) {
2610+
auto s = sor.status();
2611+
s.Update(OptStatus());
2612+
if (s.ok()) sor.value();
2613+
}
2614+
)cc");
2615+
}
2616+
24562617
} // namespace
24572618

24582619
std::string

0 commit comments

Comments
 (0)