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 all 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
115 changes: 62 additions & 53 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,83 +551,92 @@ void transferCallReturningOptional(const CallExpr *E,
setHasValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
}

void handleConstMemberCall(const CallExpr *CE,
// Returns true if the const accessor is handled by caching.
// Returns false if we could not cache. We should perform default handling
// in that case.
bool handleConstMemberCall(const CallExpr *CE,
dataflow::RecordStorageLocation *RecordLoc,
const MatchFinder::MatchResult &Result,
LatticeTransferState &State) {
// If the const method returns an optional or reference to an optional.
if (RecordLoc != nullptr && isSupportedOptionalType(CE->getType())) {
const FunctionDecl *DirectCallee = CE->getDirectCallee();
if (DirectCallee == nullptr)
return;
StorageLocation &Loc =
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
*RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
setHasValue(cast<RecordStorageLocation>(Loc),
State.Env.makeAtomicBoolValue(), State.Env);
});
if (CE->isGLValue()) {
// If the call to the const method returns a reference to an optional,
// link the call expression to the cached StorageLocation.
State.Env.setStorageLocation(*CE, Loc);
} else {
// If the call to the const method returns an optional by value, we
// need to use CopyRecord to link the optional to the result object
// of the call expression.
auto &ResultLoc = State.Env.getResultObjectLocation(*CE);
copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
}
return;
}
if (RecordLoc == nullptr)
return false;

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

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

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

// 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())) {
// PRValue cases:
if (CE->getType()->isBooleanType() || CE->getType()->isPointerType()) {
// 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);
// If the const method returns an optional by value.
const FunctionDecl *DirectCallee = CE->getDirectCallee();
if (DirectCallee == nullptr)
return false;
StorageLocation &Loc =
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
*RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
setHasValue(cast<RecordStorageLocation>(Loc),
State.Env.makeAtomicBoolValue(), State.Env);
});
// Use copyRecord to link the optional to the result object of the call
// expression.
auto &ResultLoc = State.Env.getResultObjectLocation(*CE);
copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
return true;
}

return false;
}

void transferValue_ConstMemberCall(const CXXMemberCallExpr *MCE,
const MatchFinder::MatchResult &Result,
LatticeTransferState &State) {
handleConstMemberCall(
void handleConstMemberCallWithFallbacks(
const CallExpr *CE, dataflow::RecordStorageLocation *RecordLoc,
const MatchFinder::MatchResult &Result, LatticeTransferState &State) {
if (handleConstMemberCall(CE, RecordLoc, Result, State))
return;
// Perform default handling if the call returns an optional, but wasn't
// handled by caching.
if (isSupportedOptionalType(CE->getType()))
transferCallReturningOptional(CE, Result, State);
}

void transferConstMemberCall(const CXXMemberCallExpr *MCE,
const MatchFinder::MatchResult &Result,
LatticeTransferState &State) {
handleConstMemberCallWithFallbacks(
MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), 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);
handleConstMemberCallWithFallbacks(OCE, RecordLoc, Result, State);
}

void handleNonConstMemberCall(const CallExpr *CE,
Expand Down Expand Up @@ -1094,9 +1103,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
Loading