Skip to content

[clang][dataflow] Add test for crash repro and clean up const accessor handling #129930

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Mar 7, 2025
Merged

[clang][dataflow] Add test for crash repro and clean up const accessor handling #129930

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
81f5cfd
[clang][dataflow] Add test for crash repro and clean up const accesso…
jvoung Mar 5, 2025
03e6b35
format
jvoung Mar 5, 2025
f41c0a3
fix a rename missing from merge
jvoung Mar 5, 2025
5761260
Revert the init part so it is more of a no-op.
jvoung Mar 6, 2025
941df8d
Address review comments
jvoung Mar 6, 2025
a5d8e21
clean up else after return
jvoung Mar 6, 2025
e5037ab
format
jvoung Mar 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 39 additions & 31 deletions clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,15 +551,18 @@ void transferCallReturningOptional(const CallExpr *E,
setHasValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
}

void handleConstMemberCall(const CallExpr *CE,
bool handleConstMemberCall(const CallExpr *CE,
dataflow::RecordStorageLocation *RecordLoc,
const MatchFinder::MatchResult &Result,
LatticeTransferState &State) {
if (RecordLoc == nullptr)
return false;

// If the const method returns an optional or reference to an optional.
if (RecordLoc != nullptr && isSupportedOptionalType(CE->getType())) {
if (isSupportedOptionalType(CE->getType())) {
const FunctionDecl *DirectCallee = CE->getDirectCallee();
if (DirectCallee == nullptr)
return;
return false;
StorageLocation &Loc =
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
*RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
Expand All @@ -577,57 +580,62 @@ void handleConstMemberCall(const CallExpr *CE,
auto &ResultLoc = State.Env.getResultObjectLocation(*CE);
copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
}
return;
return true;
}

// Cache if the const method returns a reference
if (RecordLoc != nullptr && CE->isGLValue()) {
if (CE->isGLValue()) {
const FunctionDecl *DirectCallee = CE->getDirectCallee();
if (DirectCallee == nullptr)
return;
return false;

StorageLocation &Loc =
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
*RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
// no-op
// NOTE: if we want to support const ref to pointers or bools
// we should initialize their values here.
});

State.Env.setStorageLocation(*CE, Loc);
return;
}

// Cache if the const method returns a boolean or pointer type.
// We may decide to cache other return types in the future.
if (RecordLoc != nullptr &&
(CE->getType()->isBooleanType() || CE->getType()->isPointerType())) {
return true;
} else if (CE->getType()->isBooleanType() || CE->getType()->isPointerType()) {
// Cache if the const method returns a boolean or pointer type.
Value *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc, CE,
State.Env);
if (Val == nullptr)
return;
return false;
State.Env.setValue(*CE, *Val);
return;
return true;
}

// Perform default handling if the call returns an optional
// but wasn't handled above (if RecordLoc is nullptr).
if (isSupportedOptionalType(CE->getType())) {
transferCallReturningOptional(CE, Result, State);
}
return false;
}

void transferValue_ConstMemberCall(const CXXMemberCallExpr *MCE,
const MatchFinder::MatchResult &Result,
LatticeTransferState &State) {
handleConstMemberCall(
MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result, State);
void transferConstMemberCall(const CXXMemberCallExpr *MCE,
const MatchFinder::MatchResult &Result,
LatticeTransferState &State) {
if (!handleConstMemberCall(
MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result,
State)) {
// Perform default handling if the call returns an optional,
// but wasn't handled.
if (isSupportedOptionalType(MCE->getType()))
transferCallReturningOptional(MCE, Result, State);
}
}

void transferValue_ConstMemberOperatorCall(
const CXXOperatorCallExpr *OCE, const MatchFinder::MatchResult &Result,
LatticeTransferState &State) {
void transferConstMemberOperatorCall(const CXXOperatorCallExpr *OCE,
const MatchFinder::MatchResult &Result,
LatticeTransferState &State) {
auto *RecordLoc = cast_or_null<dataflow::RecordStorageLocation>(
State.Env.getStorageLocation(*OCE->getArg(0)));
handleConstMemberCall(OCE, RecordLoc, Result, State);
if (!handleConstMemberCall(OCE, RecordLoc, Result, State)) {
// Perform default handling if the call returns an optional,
// but wasn't handled.
if (isSupportedOptionalType(OCE->getType()))
transferCallReturningOptional(OCE, Result, State);
}
}

void handleNonConstMemberCall(const CallExpr *CE,
Expand Down Expand Up @@ -1094,9 +1102,9 @@ auto buildTransferMatchSwitch() {

// const accessor calls
.CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(),
transferValue_ConstMemberCall)
transferConstMemberCall)
.CaseOfCFGStmt<CXXOperatorCallExpr>(isZeroParamConstMemberOperatorCall(),
transferValue_ConstMemberOperatorCall)
transferConstMemberOperatorCall)
// non-const member calls that may modify the state of an object.
.CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
transferValue_NonConstMemberCall)
Expand Down
64 changes: 46 additions & 18 deletions clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3829,7 +3829,7 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessor) {
};

void target(A& a) {
std::optional<int> opt;
$ns::$optional<int> opt;
if (a.isFoo()) {
opt = 1;
}
Expand All @@ -3851,7 +3851,7 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessorWithModInBetween) {
};

void target(A& a) {
std::optional<int> opt;
$ns::$optional<int> opt;
if (a.isFoo()) {
opt = 1;
}
Expand All @@ -3870,13 +3870,13 @@ TEST_P(UncheckedOptionalAccessTest,

struct A {
const $ns::$optional<int>& get() const { return x; }

$ns::$optional<int> x;
};

struct B {
const A& getA() const { return a; }

A a;
};

Expand All @@ -3896,13 +3896,13 @@ TEST_P(

struct A {
const $ns::$optional<int>& get() const { return x; }

$ns::$optional<int> x;
};

struct B {
const A& getA() const { return a; }

A a;
};

Expand All @@ -3919,13 +3919,13 @@ TEST_P(UncheckedOptionalAccessTest,

struct A {
const $ns::$optional<int>& get() const { return x; }

$ns::$optional<int> x;
};

struct B {
const A& getA() const { return a; }

A a;
};

Expand All @@ -3945,13 +3945,13 @@ TEST_P(UncheckedOptionalAccessTest,

struct A {
const $ns::$optional<int>& get() const { return x; }

$ns::$optional<int> x;
};

struct B {
const A copyA() const { return a; }

A a;
};

Expand All @@ -3970,13 +3970,13 @@ TEST_P(UncheckedOptionalAccessTest,

struct A {
const $ns::$optional<int>& get() const { return x; }

$ns::$optional<int> x;
};

struct B {
A& getA() { return a; }

A a;
};

Expand Down Expand Up @@ -4031,23 +4031,23 @@ TEST_P(
ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithAnotherConstCallAfterCheck) {
ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"

struct A {
const $ns::$optional<int>& get() const { return x; }

$ns::$optional<int> x;
};

struct B {
const A& getA() const { return a; }

void callWithoutChanges() const {
// no-op
}

A a;
};

void target(B& b) {
if (b.getA().get().has_value()) {
b.callWithoutChanges(); // calling const method which cannot change A
Expand All @@ -4057,6 +4057,34 @@ TEST_P(
)cc");
}

TEST_P(UncheckedOptionalAccessTest, ConstPointerRefAccessor) {
// A crash reproducer for https://github.com/llvm/llvm-project/issues/125589
// NOTE: we currently cache const ref accessors's locations.
// If we want to support const ref to pointers or bools, we should initialize
// their values.
ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"

struct A {
$ns::$optional<int> x;
};

struct PtrWrapper {
A*& getPtrRef() const;
void reset(A*);
};

void target(PtrWrapper p) {
if (p.getPtrRef()->x) {
*p.getPtrRef()->x; // [[unsafe]]
p.reset(nullptr);
*p.getPtrRef()->x; // [[unsafe]]
}
}
)cc",
/*IgnoreSmartPointerDereference=*/false);
}

// FIXME: Add support for:
// - constructors (copy, move)
// - assignment operators (default, copy, move)
Expand Down