Skip to content

Commit 81168e2

Browse files
jvoungjvoung
authored
[clang][dataflow] Add test for crash repro and clean up const accessor handling (#129930)
Add test for #125589 The crash is actually incidentally fixed by #128437 since it added a branch for the reference case and would no longer fall through when the return type is a reference to a pointer. Clean up a bit as well: - make the fallback for early returns more consistent (check if returning optional and call transfer function for that case) - check RecordLoc == nullptr in one place - clean up extra spaces in test - clean up parameterization in test of `std::` vs `$ns::$`
1 parent 9d191f1 commit 81168e2

File tree

2 files changed

+108
-71
lines changed

2 files changed

+108
-71
lines changed

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

+62-53
Original file line numberDiff line numberDiff line change
@@ -551,83 +551,92 @@ void transferCallReturningOptional(const CallExpr *E,
551551
setHasValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
552552
}
553553

554-
void handleConstMemberCall(const CallExpr *CE,
554+
// Returns true if the const accessor is handled by caching.
555+
// Returns false if we could not cache. We should perform default handling
556+
// in that case.
557+
bool handleConstMemberCall(const CallExpr *CE,
555558
dataflow::RecordStorageLocation *RecordLoc,
556559
const MatchFinder::MatchResult &Result,
557560
LatticeTransferState &State) {
558-
// If the const method returns an optional or reference to an optional.
559-
if (RecordLoc != nullptr && isSupportedOptionalType(CE->getType())) {
560-
const FunctionDecl *DirectCallee = CE->getDirectCallee();
561-
if (DirectCallee == nullptr)
562-
return;
563-
StorageLocation &Loc =
564-
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
565-
*RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
566-
setHasValue(cast<RecordStorageLocation>(Loc),
567-
State.Env.makeAtomicBoolValue(), State.Env);
568-
});
569-
if (CE->isGLValue()) {
570-
// If the call to the const method returns a reference to an optional,
571-
// link the call expression to the cached StorageLocation.
572-
State.Env.setStorageLocation(*CE, Loc);
573-
} else {
574-
// If the call to the const method returns an optional by value, we
575-
// need to use CopyRecord to link the optional to the result object
576-
// of the call expression.
577-
auto &ResultLoc = State.Env.getResultObjectLocation(*CE);
578-
copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
579-
}
580-
return;
581-
}
561+
if (RecordLoc == nullptr)
562+
return false;
582563

583-
// Cache if the const method returns a reference
584-
if (RecordLoc != nullptr && CE->isGLValue()) {
564+
// Cache if the const method returns a reference.
565+
if (CE->isGLValue()) {
585566
const FunctionDecl *DirectCallee = CE->getDirectCallee();
586567
if (DirectCallee == nullptr)
587-
return;
568+
return false;
588569

570+
// Initialize the optional's "has_value" property to true if the type is
571+
// optional, otherwise no-op. If we want to support const ref to pointers or
572+
// bools we should initialize their values here too.
573+
auto Init = [&](StorageLocation &Loc) {
574+
if (isSupportedOptionalType(CE->getType()))
575+
setHasValue(cast<RecordStorageLocation>(Loc),
576+
State.Env.makeAtomicBoolValue(), State.Env);
577+
};
589578
StorageLocation &Loc =
590579
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
591-
*RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
592-
// no-op
593-
});
580+
*RecordLoc, DirectCallee, State.Env, Init);
594581

595582
State.Env.setStorageLocation(*CE, Loc);
596-
return;
583+
return true;
597584
}
598-
599-
// Cache if the const method returns a boolean or pointer type.
600-
// We may decide to cache other return types in the future.
601-
if (RecordLoc != nullptr &&
602-
(CE->getType()->isBooleanType() || CE->getType()->isPointerType())) {
585+
// PRValue cases:
586+
if (CE->getType()->isBooleanType() || CE->getType()->isPointerType()) {
587+
// If the const method returns a boolean or pointer type.
603588
Value *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc, CE,
604589
State.Env);
605590
if (Val == nullptr)
606-
return;
591+
return false;
607592
State.Env.setValue(*CE, *Val);
608-
return;
593+
return true;
609594
}
610-
611-
// Perform default handling if the call returns an optional
612-
// but wasn't handled above (if RecordLoc is nullptr).
613595
if (isSupportedOptionalType(CE->getType())) {
614-
transferCallReturningOptional(CE, Result, State);
596+
// If the const method returns an optional by value.
597+
const FunctionDecl *DirectCallee = CE->getDirectCallee();
598+
if (DirectCallee == nullptr)
599+
return false;
600+
StorageLocation &Loc =
601+
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
602+
*RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
603+
setHasValue(cast<RecordStorageLocation>(Loc),
604+
State.Env.makeAtomicBoolValue(), State.Env);
605+
});
606+
// Use copyRecord to link the optional to the result object of the call
607+
// expression.
608+
auto &ResultLoc = State.Env.getResultObjectLocation(*CE);
609+
copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
610+
return true;
615611
}
612+
613+
return false;
616614
}
617615

618-
void transferValue_ConstMemberCall(const CXXMemberCallExpr *MCE,
619-
const MatchFinder::MatchResult &Result,
620-
LatticeTransferState &State) {
621-
handleConstMemberCall(
616+
void handleConstMemberCallWithFallbacks(
617+
const CallExpr *CE, dataflow::RecordStorageLocation *RecordLoc,
618+
const MatchFinder::MatchResult &Result, LatticeTransferState &State) {
619+
if (handleConstMemberCall(CE, RecordLoc, Result, State))
620+
return;
621+
// Perform default handling if the call returns an optional, but wasn't
622+
// handled by caching.
623+
if (isSupportedOptionalType(CE->getType()))
624+
transferCallReturningOptional(CE, Result, State);
625+
}
626+
627+
void transferConstMemberCall(const CXXMemberCallExpr *MCE,
628+
const MatchFinder::MatchResult &Result,
629+
LatticeTransferState &State) {
630+
handleConstMemberCallWithFallbacks(
622631
MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result, State);
623632
}
624633

625-
void transferValue_ConstMemberOperatorCall(
626-
const CXXOperatorCallExpr *OCE, const MatchFinder::MatchResult &Result,
627-
LatticeTransferState &State) {
634+
void transferConstMemberOperatorCall(const CXXOperatorCallExpr *OCE,
635+
const MatchFinder::MatchResult &Result,
636+
LatticeTransferState &State) {
628637
auto *RecordLoc = cast_or_null<dataflow::RecordStorageLocation>(
629638
State.Env.getStorageLocation(*OCE->getArg(0)));
630-
handleConstMemberCall(OCE, RecordLoc, Result, State);
639+
handleConstMemberCallWithFallbacks(OCE, RecordLoc, Result, State);
631640
}
632641

633642
void handleNonConstMemberCall(const CallExpr *CE,
@@ -1094,9 +1103,9 @@ auto buildTransferMatchSwitch() {
10941103

10951104
// const accessor calls
10961105
.CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(),
1097-
transferValue_ConstMemberCall)
1106+
transferConstMemberCall)
10981107
.CaseOfCFGStmt<CXXOperatorCallExpr>(isZeroParamConstMemberOperatorCall(),
1099-
transferValue_ConstMemberOperatorCall)
1108+
transferConstMemberOperatorCall)
11001109
// non-const member calls that may modify the state of an object.
11011110
.CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
11021111
transferValue_NonConstMemberCall)

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

+46-18
Original file line numberDiff line numberDiff line change
@@ -3829,7 +3829,7 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessor) {
38293829
};
38303830
38313831
void target(A& a) {
3832-
std::optional<int> opt;
3832+
$ns::$optional<int> opt;
38333833
if (a.isFoo()) {
38343834
opt = 1;
38353835
}
@@ -3851,7 +3851,7 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessorWithModInBetween) {
38513851
};
38523852
38533853
void target(A& a) {
3854-
std::optional<int> opt;
3854+
$ns::$optional<int> opt;
38553855
if (a.isFoo()) {
38563856
opt = 1;
38573857
}
@@ -3870,13 +3870,13 @@ TEST_P(UncheckedOptionalAccessTest,
38703870
38713871
struct A {
38723872
const $ns::$optional<int>& get() const { return x; }
3873-
3873+
38743874
$ns::$optional<int> x;
38753875
};
38763876
38773877
struct B {
38783878
const A& getA() const { return a; }
3879-
3879+
38803880
A a;
38813881
};
38823882
@@ -3896,13 +3896,13 @@ TEST_P(
38963896
38973897
struct A {
38983898
const $ns::$optional<int>& get() const { return x; }
3899-
3899+
39003900
$ns::$optional<int> x;
39013901
};
39023902
39033903
struct B {
39043904
const A& getA() const { return a; }
3905-
3905+
39063906
A a;
39073907
};
39083908
@@ -3919,13 +3919,13 @@ TEST_P(UncheckedOptionalAccessTest,
39193919
39203920
struct A {
39213921
const $ns::$optional<int>& get() const { return x; }
3922-
3922+
39233923
$ns::$optional<int> x;
39243924
};
39253925
39263926
struct B {
39273927
const A& getA() const { return a; }
3928-
3928+
39293929
A a;
39303930
};
39313931
@@ -3945,13 +3945,13 @@ TEST_P(UncheckedOptionalAccessTest,
39453945
39463946
struct A {
39473947
const $ns::$optional<int>& get() const { return x; }
3948-
3948+
39493949
$ns::$optional<int> x;
39503950
};
39513951
39523952
struct B {
39533953
const A copyA() const { return a; }
3954-
3954+
39553955
A a;
39563956
};
39573957
@@ -3970,13 +3970,13 @@ TEST_P(UncheckedOptionalAccessTest,
39703970
39713971
struct A {
39723972
const $ns::$optional<int>& get() const { return x; }
3973-
3973+
39743974
$ns::$optional<int> x;
39753975
};
39763976
39773977
struct B {
39783978
A& getA() { return a; }
3979-
3979+
39803980
A a;
39813981
};
39823982
@@ -4031,23 +4031,23 @@ TEST_P(
40314031
ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithAnotherConstCallAfterCheck) {
40324032
ExpectDiagnosticsFor(R"cc(
40334033
#include "unchecked_optional_access_test.h"
4034-
4034+
40354035
struct A {
40364036
const $ns::$optional<int>& get() const { return x; }
4037-
4037+
40384038
$ns::$optional<int> x;
40394039
};
4040-
4040+
40414041
struct B {
40424042
const A& getA() const { return a; }
4043-
4043+
40444044
void callWithoutChanges() const {
40454045
// no-op
40464046
}
4047-
4047+
40484048
A a;
40494049
};
4050-
4050+
40514051
void target(B& b) {
40524052
if (b.getA().get().has_value()) {
40534053
b.callWithoutChanges(); // calling const method which cannot change A
@@ -4057,6 +4057,34 @@ TEST_P(
40574057
)cc");
40584058
}
40594059

4060+
TEST_P(UncheckedOptionalAccessTest, ConstPointerRefAccessor) {
4061+
// A crash reproducer for https://github.com/llvm/llvm-project/issues/125589
4062+
// NOTE: we currently cache const ref accessors's locations.
4063+
// If we want to support const ref to pointers or bools, we should initialize
4064+
// their values.
4065+
ExpectDiagnosticsFor(R"cc(
4066+
#include "unchecked_optional_access_test.h"
4067+
4068+
struct A {
4069+
$ns::$optional<int> x;
4070+
};
4071+
4072+
struct PtrWrapper {
4073+
A*& getPtrRef() const;
4074+
void reset(A*);
4075+
};
4076+
4077+
void target(PtrWrapper p) {
4078+
if (p.getPtrRef()->x) {
4079+
*p.getPtrRef()->x; // [[unsafe]]
4080+
p.reset(nullptr);
4081+
*p.getPtrRef()->x; // [[unsafe]]
4082+
}
4083+
}
4084+
)cc",
4085+
/*IgnoreSmartPointerDereference=*/false);
4086+
}
4087+
40604088
// FIXME: Add support for:
40614089
// - constructors (copy, move)
40624090
// - assignment operators (default, copy, move)

0 commit comments

Comments
 (0)