https://github.com/jvoung updated https://github.com/llvm/llvm-project/pull/129930
>From 81f5cfde1029b3c9ddd62eb0587ed370d67cccab Mon Sep 17 00:00:00 2001 From: Jan Voung <jvo...@gmail.com> Date: Wed, 5 Mar 2025 20:15:48 +0000 Subject: [PATCH 1/4] [clang][dataflow] Add test for crash repro and clean up const accessor handling Add test for https://github.com/llvm/llvm-project/issues/125589 The crash is actually incidentally fixed by https://github.com/llvm/llvm-project/pull/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 Add some init for the reference to pointer/bool cases. --- .../Models/UncheckedOptionalAccessModel.cpp | 70 +++++++++++-------- .../UncheckedOptionalAccessModelTest.cpp | 24 +++++++ 2 files changed, 64 insertions(+), 30 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 9381c5c42e566..914f29c243248 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -551,15 +551,17 @@ 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) { @@ -577,57 +579,65 @@ void handleConstMemberCall(const CallExpr *CE, auto &ResultLoc = State.Env.getResultObjectLocation(*CE); copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env); } - return; + return true; } + bool IsBooleanOrPointer = CE->getType()->isBooleanType() || + CE->getType()->isPointerType(); + // 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 + if (IsBooleanOrPointer) { + State.Env.setValue(Loc, *State.Env.createValue(CE->getType())); + } }); 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 (IsBooleanOrPointer) { + // 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, diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 5031e17188e17..da09dfe9ea522 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -3771,6 +3771,30 @@ TEST_P(UncheckedOptionalAccessTest, ConstPointerAccessorWithModInBetween) { /*IgnoreSmartPointerDereference=*/false); } +TEST_P(UncheckedOptionalAccessTest, ConstPointerRefAccessor) { + 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; + p.reset(nullptr); + *p.getPtrRef()->x; // [[unsafe]] + } + } + )cc", + /*IgnoreSmartPointerDereference=*/false); +} + TEST_P(UncheckedOptionalAccessTest, SmartPointerAccessorMixed) { ExpectDiagnosticsFor(R"cc( #include "unchecked_optional_access_test.h" >From 03e6b35588a49133c010f0c9ecbae833e0bcfb29 Mon Sep 17 00:00:00 2001 From: Jan Voung <jvo...@gmail.com> Date: Wed, 5 Mar 2025 20:24:41 +0000 Subject: [PATCH 2/4] format --- .../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 914f29c243248..e1f51853b1931 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -555,7 +555,8 @@ bool handleConstMemberCall(const CallExpr *CE, dataflow::RecordStorageLocation *RecordLoc, const MatchFinder::MatchResult &Result, LatticeTransferState &State) { - if (RecordLoc == nullptr) return false; + if (RecordLoc == nullptr) + return false; // If the const method returns an optional or reference to an optional. if (isSupportedOptionalType(CE->getType())) { @@ -582,8 +583,8 @@ bool handleConstMemberCall(const CallExpr *CE, return true; } - bool IsBooleanOrPointer = CE->getType()->isBooleanType() || - CE->getType()->isPointerType(); + bool IsBooleanOrPointer = + CE->getType()->isBooleanType() || CE->getType()->isPointerType(); // Cache if the const method returns a reference if (CE->isGLValue()) { >From f41c0a3391fedb3aa4b8a7f12b5fed0edd1937fd Mon Sep 17 00:00:00 2001 From: Jan Voung <jvo...@gmail.com> Date: Wed, 5 Mar 2025 21:20:32 +0000 Subject: [PATCH 3/4] fix a rename missing from merge --- .../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index e1f51853b1931..6a26008f855d4 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -1105,9 +1105,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) >From 5761260f24eac21e2273074bc177371ef5fa8776 Mon Sep 17 00:00:00 2001 From: Jan Voung <jvo...@gmail.com> Date: Thu, 6 Mar 2025 02:54:29 +0000 Subject: [PATCH 4/4] Revert the init part so it is more of a no-op. More whitespace/test cleanup --- .../Models/UncheckedOptionalAccessModel.cpp | 11 +-- .../UncheckedOptionalAccessModelTest.cpp | 88 ++++++++++--------- 2 files changed, 50 insertions(+), 49 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 6a26008f855d4..9c873d2e7e1f4 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -583,9 +583,6 @@ bool handleConstMemberCall(const CallExpr *CE, return true; } - bool IsBooleanOrPointer = - CE->getType()->isBooleanType() || CE->getType()->isPointerType(); - // Cache if the const method returns a reference if (CE->isGLValue()) { const FunctionDecl *DirectCallee = CE->getDirectCallee(); @@ -595,14 +592,14 @@ bool handleConstMemberCall(const CallExpr *CE, StorageLocation &Loc = State.Lattice.getOrCreateConstMethodReturnStorageLocation( *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) { - if (IsBooleanOrPointer) { - State.Env.setValue(Loc, *State.Env.createValue(CE->getType())); - } + // 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 true; - } else if (IsBooleanOrPointer) { + } 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); diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index da09dfe9ea522..46fad6b655c4d 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -3771,30 +3771,6 @@ TEST_P(UncheckedOptionalAccessTest, ConstPointerAccessorWithModInBetween) { /*IgnoreSmartPointerDereference=*/false); } -TEST_P(UncheckedOptionalAccessTest, ConstPointerRefAccessor) { - 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; - p.reset(nullptr); - *p.getPtrRef()->x; // [[unsafe]] - } - } - )cc", - /*IgnoreSmartPointerDereference=*/false); -} - TEST_P(UncheckedOptionalAccessTest, SmartPointerAccessorMixed) { ExpectDiagnosticsFor(R"cc( #include "unchecked_optional_access_test.h" @@ -3853,7 +3829,7 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessor) { }; void target(A& a) { - std::optional<int> opt; + $ns::$optional<int> opt; if (a.isFoo()) { opt = 1; } @@ -3875,7 +3851,7 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessorWithModInBetween) { }; void target(A& a) { - std::optional<int> opt; + $ns::$optional<int> opt; if (a.isFoo()) { opt = 1; } @@ -3894,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; }; @@ -3920,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; }; @@ -3943,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; }; @@ -3969,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; }; @@ -3994,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; }; @@ -4055,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 @@ -4081,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) _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
