Skip to content

Commit 725e7f4

Browse files
fmayeraokblast
fmayer
authored and
aokblast
committed
[FlowSensitive] [StatusOr] [4/N] Support comparisons (llvm#163871)
1 parent 211c0ba commit 725e7f4

File tree

2 files changed

+349
-0
lines changed

2 files changed

+349
-0
lines changed

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

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,19 @@ static auto valueOperatorCall() {
137137
isStatusOrOperatorCallWithName("->")));
138138
}
139139

140+
static clang::ast_matchers::TypeMatcher statusType() {
141+
using namespace ::clang::ast_matchers; // NOLINT: Too many names
142+
return hasCanonicalType(qualType(hasDeclaration(statusClass())));
143+
}
144+
145+
static auto isComparisonOperatorCall(llvm::StringRef operator_name) {
146+
using namespace ::clang::ast_matchers; // NOLINT: Too many names
147+
return cxxOperatorCallExpr(
148+
hasOverloadedOperatorName(operator_name), argumentCountIs(2),
149+
hasArgument(0, anyOf(hasType(statusType()), hasType(statusOrType()))),
150+
hasArgument(1, anyOf(hasType(statusType()), hasType(statusOrType()))));
151+
}
152+
140153
static auto
141154
buildDiagnoseMatchSwitch(const UncheckedStatusOrAccessModelOptions &Options) {
142155
return CFGMatchSwitchBuilder<const Environment,
@@ -312,6 +325,101 @@ static void transferStatusUpdateCall(const CXXMemberCallExpr *Expr,
312325
State.Env.setValue(locForOk(*ThisLoc), NewVal);
313326
}
314327

328+
static BoolValue *evaluateStatusEquality(RecordStorageLocation &LhsStatusLoc,
329+
RecordStorageLocation &RhsStatusLoc,
330+
Environment &Env) {
331+
auto &A = Env.arena();
332+
// Logically, a Status object is composed of an error code that could take one
333+
// of multiple possible values, including the "ok" value. We track whether a
334+
// Status object has an "ok" value and represent this as an `ok` bit. Equality
335+
// of Status objects compares their error codes. Therefore, merely comparing
336+
// the `ok` bits isn't sufficient: when two Status objects are assigned non-ok
337+
// error codes the equality of their respective error codes matters. Since we
338+
// only track the `ok` bits, we can't make any conclusions about equality when
339+
// we know that two Status objects have non-ok values.
340+
341+
auto &LhsOkVal = valForOk(LhsStatusLoc, Env);
342+
auto &RhsOkVal = valForOk(RhsStatusLoc, Env);
343+
344+
auto &Res = Env.makeAtomicBoolValue();
345+
346+
// lhs && rhs => res (a.k.a. !res => !lhs || !rhs)
347+
Env.assume(A.makeImplies(A.makeAnd(LhsOkVal.formula(), RhsOkVal.formula()),
348+
Res.formula()));
349+
// res => (lhs == rhs)
350+
Env.assume(A.makeImplies(
351+
Res.formula(), A.makeEquals(LhsOkVal.formula(), RhsOkVal.formula())));
352+
353+
return &Res;
354+
}
355+
356+
static BoolValue *
357+
evaluateStatusOrEquality(RecordStorageLocation &LhsStatusOrLoc,
358+
RecordStorageLocation &RhsStatusOrLoc,
359+
Environment &Env) {
360+
auto &A = Env.arena();
361+
// Logically, a StatusOr<T> object is composed of two values - a Status and a
362+
// value of type T. Equality of StatusOr objects compares both values.
363+
// Therefore, merely comparing the `ok` bits of the Status values isn't
364+
// sufficient. When two StatusOr objects are engaged, the equality of their
365+
// respective values of type T matters. Similarly, when two StatusOr objects
366+
// have Status values that have non-ok error codes, the equality of the error
367+
// codes matters. Since we only track the `ok` bits of the Status values, we
368+
// can't make any conclusions about equality when we know that two StatusOr
369+
// objects are engaged or when their Status values contain non-ok error codes.
370+
auto &LhsOkVal = valForOk(locForStatus(LhsStatusOrLoc), Env);
371+
auto &RhsOkVal = valForOk(locForStatus(RhsStatusOrLoc), Env);
372+
auto &res = Env.makeAtomicBoolValue();
373+
374+
// res => (lhs == rhs)
375+
Env.assume(A.makeImplies(
376+
res.formula(), A.makeEquals(LhsOkVal.formula(), RhsOkVal.formula())));
377+
return &res;
378+
}
379+
380+
static BoolValue *evaluateEquality(const Expr *LhsExpr, const Expr *RhsExpr,
381+
Environment &Env) {
382+
// Check the type of both sides in case an operator== is added that admits
383+
// different types.
384+
if (isStatusOrType(LhsExpr->getType()) &&
385+
isStatusOrType(RhsExpr->getType())) {
386+
auto *LhsStatusOrLoc = Env.get<RecordStorageLocation>(*LhsExpr);
387+
if (LhsStatusOrLoc == nullptr)
388+
return nullptr;
389+
auto *RhsStatusOrLoc = Env.get<RecordStorageLocation>(*RhsExpr);
390+
if (RhsStatusOrLoc == nullptr)
391+
return nullptr;
392+
393+
return evaluateStatusOrEquality(*LhsStatusOrLoc, *RhsStatusOrLoc, Env);
394+
}
395+
if (isStatusType(LhsExpr->getType()) && isStatusType(RhsExpr->getType())) {
396+
auto *LhsStatusLoc = Env.get<RecordStorageLocation>(*LhsExpr);
397+
if (LhsStatusLoc == nullptr)
398+
return nullptr;
399+
400+
auto *RhsStatusLoc = Env.get<RecordStorageLocation>(*RhsExpr);
401+
if (RhsStatusLoc == nullptr)
402+
return nullptr;
403+
404+
return evaluateStatusEquality(*LhsStatusLoc, *RhsStatusLoc, Env);
405+
}
406+
return nullptr;
407+
}
408+
409+
static void transferComparisonOperator(const CXXOperatorCallExpr *Expr,
410+
LatticeTransferState &State,
411+
bool IsNegative) {
412+
auto *LhsAndRhsVal =
413+
evaluateEquality(Expr->getArg(0), Expr->getArg(1), State.Env);
414+
if (LhsAndRhsVal == nullptr)
415+
return;
416+
417+
if (IsNegative)
418+
State.Env.setValue(*Expr, State.Env.makeNot(*LhsAndRhsVal));
419+
else
420+
State.Env.setValue(*Expr, *LhsAndRhsVal);
421+
}
422+
315423
CFGMatchSwitch<LatticeTransferState>
316424
buildTransferMatchSwitch(ASTContext &Ctx,
317425
CFGMatchSwitchBuilder<LatticeTransferState> Builder) {
@@ -325,6 +433,20 @@ buildTransferMatchSwitch(ASTContext &Ctx,
325433
transferStatusOkCall)
326434
.CaseOfCFGStmt<CXXMemberCallExpr>(isStatusMemberCallWithName("Update"),
327435
transferStatusUpdateCall)
436+
.CaseOfCFGStmt<CXXOperatorCallExpr>(
437+
isComparisonOperatorCall("=="),
438+
[](const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &,
439+
LatticeTransferState &State) {
440+
transferComparisonOperator(Expr, State,
441+
/*IsNegative=*/false);
442+
})
443+
.CaseOfCFGStmt<CXXOperatorCallExpr>(
444+
isComparisonOperatorCall("!="),
445+
[](const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &,
446+
LatticeTransferState &State) {
447+
transferComparisonOperator(Expr, State,
448+
/*IsNegative=*/true);
449+
})
328450
.Build();
329451
}
330452

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

Lines changed: 227 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2614,6 +2614,233 @@ TEST_P(UncheckedStatusOrAccessModelTest, StatusUpdate) {
26142614
)cc");
26152615
}
26162616

2617+
TEST_P(UncheckedStatusOrAccessModelTest, EqualityCheck) {
2618+
ExpectDiagnosticsFor(
2619+
R"cc(
2620+
#include "unchecked_statusor_access_test_defs.h"
2621+
2622+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2623+
if (x.ok()) {
2624+
if (x == y)
2625+
y.value();
2626+
else
2627+
y.value(); // [[unsafe]]
2628+
}
2629+
}
2630+
)cc");
2631+
ExpectDiagnosticsFor(
2632+
R"cc(
2633+
#include "unchecked_statusor_access_test_defs.h"
2634+
2635+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2636+
if (x.ok()) {
2637+
if (y == x)
2638+
y.value();
2639+
else
2640+
y.value(); // [[unsafe]]
2641+
}
2642+
}
2643+
)cc");
2644+
ExpectDiagnosticsFor(R"cc(
2645+
#include "unchecked_statusor_access_test_defs.h"
2646+
2647+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2648+
if (x.ok()) {
2649+
if (x != y)
2650+
y.value(); // [[unsafe]]
2651+
else
2652+
y.value();
2653+
}
2654+
}
2655+
)cc");
2656+
ExpectDiagnosticsFor(R"cc(
2657+
#include "unchecked_statusor_access_test_defs.h"
2658+
2659+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2660+
if (x.ok()) {
2661+
if (y != x)
2662+
y.value(); // [[unsafe]]
2663+
else
2664+
y.value();
2665+
}
2666+
}
2667+
)cc");
2668+
ExpectDiagnosticsFor(R"cc(
2669+
#include "unchecked_statusor_access_test_defs.h"
2670+
2671+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2672+
if (x.ok()) {
2673+
if (!(x == y))
2674+
y.value(); // [[unsafe]]
2675+
else
2676+
y.value();
2677+
}
2678+
}
2679+
)cc");
2680+
ExpectDiagnosticsFor(
2681+
R"cc(
2682+
#include "unchecked_statusor_access_test_defs.h"
2683+
2684+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2685+
if (x.ok()) {
2686+
if (!(x != y))
2687+
y.value();
2688+
else
2689+
y.value(); // [[unsafe]]
2690+
}
2691+
}
2692+
)cc");
2693+
ExpectDiagnosticsFor(R"cc(
2694+
#include "unchecked_statusor_access_test_defs.h"
2695+
2696+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2697+
if (x == y)
2698+
if (x.ok()) y.value();
2699+
}
2700+
)cc");
2701+
ExpectDiagnosticsFor(
2702+
R"cc(
2703+
#include "unchecked_statusor_access_test_defs.h"
2704+
2705+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2706+
if (x.ok()) {
2707+
if (x.status() == y.status())
2708+
y.value();
2709+
else
2710+
y.value(); // [[unsafe]]
2711+
}
2712+
}
2713+
)cc");
2714+
ExpectDiagnosticsFor(R"cc(
2715+
#include "unchecked_statusor_access_test_defs.h"
2716+
2717+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2718+
if (x.ok()) {
2719+
if (x.status() != y.status())
2720+
y.value(); // [[unsafe]]
2721+
else
2722+
y.value();
2723+
}
2724+
}
2725+
)cc");
2726+
ExpectDiagnosticsFor(
2727+
R"cc(
2728+
#include "unchecked_statusor_access_test_defs.h"
2729+
2730+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2731+
if (x.ok()) {
2732+
if (x.ok() == y.ok())
2733+
y.value();
2734+
else
2735+
y.value(); // [[unsafe]]
2736+
}
2737+
}
2738+
)cc");
2739+
ExpectDiagnosticsFor(R"cc(
2740+
#include "unchecked_statusor_access_test_defs.h"
2741+
2742+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2743+
if (x.ok()) {
2744+
if (x.ok() != y.ok())
2745+
y.value(); // [[unsafe]]
2746+
else
2747+
y.value();
2748+
}
2749+
}
2750+
)cc");
2751+
ExpectDiagnosticsFor(
2752+
R"cc(
2753+
#include "unchecked_statusor_access_test_defs.h"
2754+
2755+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2756+
if (x.ok()) {
2757+
if (x.status().ok() == y.status().ok())
2758+
y.value();
2759+
else
2760+
y.value(); // [[unsafe]]
2761+
}
2762+
}
2763+
)cc");
2764+
ExpectDiagnosticsFor(R"cc(
2765+
#include "unchecked_statusor_access_test_defs.h"
2766+
2767+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2768+
if (x.ok()) {
2769+
if (x.status().ok() != y.status().ok())
2770+
y.value(); // [[unsafe]]
2771+
else
2772+
y.value();
2773+
}
2774+
}
2775+
)cc");
2776+
ExpectDiagnosticsFor(
2777+
R"cc(
2778+
#include "unchecked_statusor_access_test_defs.h"
2779+
2780+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2781+
if (x.ok()) {
2782+
if (x.status().ok() == y.ok())
2783+
y.value();
2784+
else
2785+
y.value(); // [[unsafe]]
2786+
}
2787+
}
2788+
)cc");
2789+
ExpectDiagnosticsFor(R"cc(
2790+
#include "unchecked_statusor_access_test_defs.h"
2791+
2792+
void target(STATUSOR_INT x, STATUSOR_INT y) {
2793+
if (x.ok()) {
2794+
if (x.status().ok() != y.ok())
2795+
y.value(); // [[unsafe]]
2796+
else
2797+
y.value();
2798+
}
2799+
}
2800+
)cc");
2801+
ExpectDiagnosticsFor(R"cc(
2802+
#include "unchecked_statusor_access_test_defs.h"
2803+
2804+
void target(bool b, STATUSOR_INT sor) {
2805+
if (sor.ok() == b) {
2806+
if (b) sor.value();
2807+
}
2808+
}
2809+
)cc");
2810+
ExpectDiagnosticsFor(R"cc(
2811+
#include "unchecked_statusor_access_test_defs.h"
2812+
2813+
void target(STATUSOR_INT sor) {
2814+
if (sor.ok() == true) sor.value();
2815+
}
2816+
)cc");
2817+
ExpectDiagnosticsFor(R"cc(
2818+
#include "unchecked_statusor_access_test_defs.h"
2819+
2820+
void target(STATUSOR_INT sor) {
2821+
if (sor.ok() == false) sor.value(); // [[unsafe]]
2822+
}
2823+
)cc");
2824+
ExpectDiagnosticsFor(R"cc(
2825+
#include "unchecked_statusor_access_test_defs.h"
2826+
2827+
void target(bool b) {
2828+
STATUSOR_INT sor1;
2829+
STATUSOR_INT sor2 = Make<STATUSOR_INT>();
2830+
if (sor1 == sor2) sor2.value(); // [[unsafe]]
2831+
}
2832+
)cc");
2833+
ExpectDiagnosticsFor(R"cc(
2834+
#include "unchecked_statusor_access_test_defs.h"
2835+
2836+
void target(bool b) {
2837+
STATUSOR_INT sor1 = Make<STATUSOR_INT>();
2838+
STATUSOR_INT sor2;
2839+
if (sor1 == sor2) sor1.value(); // [[unsafe]]
2840+
}
2841+
)cc");
2842+
}
2843+
26172844
} // namespace
26182845

26192846
std::string

0 commit comments

Comments
 (0)