https://github.com/BaLiKfromUA created https://github.com/llvm/llvm-project/pull/128437
Fixes https://github.com/llvm/llvm-project/issues/126283 Extending https://github.com/llvm/llvm-project/pull/112605 to cache const getters which return references. This should fix false positive cases when we check optional via the chain of const getter calls. >From 319ad0b803b8c6c6c5405178335bd1f2258be4b8 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <valentin.yukhyme...@gmail.com> Date: Sun, 23 Feb 2025 12:08:02 +0000 Subject: [PATCH 1/3] first implementation and basic tests --- .../Models/UncheckedOptionalAccessModel.cpp | 20 +++++++ .../UncheckedOptionalAccessModelTest.cpp | 59 +++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index e1394e28cd49a..993967e0c3edd 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -580,6 +580,26 @@ void handleConstMemberCall(const CallExpr *CE, return; } + // if const method returns a reference + if (CE->isGLValue()) { + const FunctionDecl *DirectCallee = CE->getDirectCallee(); + if (DirectCallee == nullptr) + return; + + QualType DeclaredReturnType = DirectCallee->getReturnType(); + + if (DeclaredReturnType.getTypePtr()->isReferenceType()) { + StorageLocation &Loc = + State.Lattice.getOrCreateConstMethodReturnStorageLocation( + *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) { + // No-op + }); + + 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 && diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 19c3ff49eab27..7140040022794 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -3863,6 +3863,65 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessorWithModInBetween) { )cc"); } + +TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObject) { + ExpectDiagnosticsFor(R"cc( + #include "unchecked_optional_access_test.h" + + class A { + public: + const $ns::$optional<int>& get() const { return x; } + + private: + $ns::$optional<int> x; + }; + + class B { + public: + const A& getA() const { return a; } + + private: + A a; + }; + + void target(B& b) { + if (b.getA().get().has_value()) { + b.getA().get().value(); + } + } + )cc"); +} + +TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithoutValueCheck) { + ExpectDiagnosticsFor(R"cc( + #include "unchecked_optional_access_test.h" + + class A { + public: + const $ns::$optional<int>& get() const { return x; } + + private: + $ns::$optional<int> x; + }; + + class B { + public: + const A& getA() const { return a; } + + private: + A a; + }; + + void target(B& b) { + b.getA().get().value(); // [[unsafe]] + } + )cc"); +} + +// todo: non const accessor +// todo: different accessor in between +// todo: const copy + // FIXME: Add support for: // - constructors (copy, move) // - assignment operators (default, copy, move) >From d7e3105087d5347fe100f0a567c1538c1a3673c0 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <valentin.yukhyme...@gmail.com> Date: Sun, 23 Feb 2025 21:37:54 +0000 Subject: [PATCH 2/3] more tests --- .../Models/UncheckedOptionalAccessModel.cpp | 11 +- .../UncheckedOptionalAccessModelTest.cpp | 126 +++++++++++++++++- 2 files changed, 128 insertions(+), 9 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 993967e0c3edd..a35ac09b15502 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -580,19 +580,18 @@ void handleConstMemberCall(const CallExpr *CE, return; } - // if const method returns a reference - if (CE->isGLValue()) { + // Cache if the const method returns a reference + if (RecordLoc != nullptr && CE->isGLValue()) { const FunctionDecl *DirectCallee = CE->getDirectCallee(); if (DirectCallee == nullptr) return; - QualType DeclaredReturnType = DirectCallee->getReturnType(); - - if (DeclaredReturnType.getTypePtr()->isReferenceType()) { + bool isReference = DirectCallee->getReturnType().getTypePtr()->isReferenceType(); + if (isReference) { StorageLocation &Loc = State.Lattice.getOrCreateConstMethodReturnStorageLocation( *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) { - // No-op + // no-op }); State.Env.setStorageLocation(*CE, Loc); diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 7140040022794..4cec24829885c 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -3918,9 +3918,129 @@ TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstRefAccesso )cc"); } -// todo: non const accessor -// todo: different accessor in between -// todo: const copy +TEST_P(UncheckedOptionalAccessTest, ConstRefToOptionalSavedAsTemporaryVariable) { + ExpectDiagnosticsFor(R"cc( + #include "unchecked_optional_access_test.h" + + class A { + public: + const $ns::$optional<int>& get() const { return x; } + + private: + $ns::$optional<int> x; + }; + + class B { + public: + const A& getA() const { return a; } + + private: + A a; + }; + + void target(B& b) { + const auto& opt = b.getA().get(); + if (opt.has_value()) { + opt.value(); + } + } + )cc"); +} + +TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstValueAccessorToHoldingObject) { + ExpectDiagnosticsFor(R"cc( + #include "unchecked_optional_access_test.h" + + class A { + public: + const $ns::$optional<int>& get() const { return x; } + + private: + $ns::$optional<int> x; + }; + + class B { + public: + const A copyA() const { return a; } + + private: + A a; + }; + + void target(B& b) { + if (b.copyA().get().has_value()) { + b.copyA().get().value(); // [[unsafe]] + } + } + )cc"); +} + +TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObject) { + ExpectDiagnosticsFor(R"cc( + #include "unchecked_optional_access_test.h" + + class A { + public: + const $ns::$optional<int>& get() const { return x; } + + private: + $ns::$optional<int> x; + }; + + class B { + public: + A& getA() { return a; } + + private: + A a; + }; + + void target(B& b) { + if (b.getA().get().has_value()) { + b.getA().get().value(); // [[unsafe]] + } + } + )cc"); +} + +TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObjectWithModAfterCheck) { + ExpectDiagnosticsFor(R"cc( + #include "unchecked_optional_access_test.h" + + class A { + public: + const $ns::$optional<int>& get() const { return x; } + private: + $ns::$optional<int> x; + }; + + class B { + public: + const A& getA() const { return a; } + + A& getA() { return a; } + + void clear() { a = A{}; }; + + private: + A a; + }; + + void target(B& b) { + // changing field A via non-const getter after const getter check + if (b.getA().get().has_value()) { + b.getA() = A{}; + b.getA().get().value(); // [[unsafe]] + } + + // calling non-const method which might change field A + if (b.getA().get().has_value()) { + b.clear(); + b.getA().get().value(); // [[unsafe]] + } + } + )cc"); +} // FIXME: Add support for: // - constructors (copy, move) >From 9608e954136b6cd8ee51ce5a301b828caadb314e Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <valentin.yukhyme...@gmail.com> Date: Sun, 23 Feb 2025 21:42:36 +0000 Subject: [PATCH 3/3] format --- .../Models/UncheckedOptionalAccessModel.cpp | 13 ++++++------ .../UncheckedOptionalAccessModelTest.cpp | 21 ++++++++++++------- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index a35ac09b15502..dccf5ee7f94c2 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -586,14 +586,15 @@ void handleConstMemberCall(const CallExpr *CE, if (DirectCallee == nullptr) return; - bool isReference = DirectCallee->getReturnType().getTypePtr()->isReferenceType(); + bool isReference = + DirectCallee->getReturnType().getTypePtr()->isReferenceType(); if (isReference) { StorageLocation &Loc = - State.Lattice.getOrCreateConstMethodReturnStorageLocation( - *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) { - // no-op - }); - + State.Lattice.getOrCreateConstMethodReturnStorageLocation( + *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) { + // no-op + }); + State.Env.setStorageLocation(*CE, Loc); return; } diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 4cec24829885c..ddecab3af449d 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -3863,8 +3863,8 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessorWithModInBetween) { )cc"); } - -TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObject) { +TEST_P(UncheckedOptionalAccessTest, + ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObject) { ExpectDiagnosticsFor(R"cc( #include "unchecked_optional_access_test.h" @@ -3892,7 +3892,9 @@ TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstRefAccesso )cc"); } -TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithoutValueCheck) { +TEST_P( + UncheckedOptionalAccessTest, + ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithoutValueCheck) { ExpectDiagnosticsFor(R"cc( #include "unchecked_optional_access_test.h" @@ -3918,7 +3920,8 @@ TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstRefAccesso )cc"); } -TEST_P(UncheckedOptionalAccessTest, ConstRefToOptionalSavedAsTemporaryVariable) { +TEST_P(UncheckedOptionalAccessTest, + ConstRefToOptionalSavedAsTemporaryVariable) { ExpectDiagnosticsFor(R"cc( #include "unchecked_optional_access_test.h" @@ -3947,7 +3950,8 @@ TEST_P(UncheckedOptionalAccessTest, ConstRefToOptionalSavedAsTemporaryVariable) )cc"); } -TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstValueAccessorToHoldingObject) { +TEST_P(UncheckedOptionalAccessTest, + ConstRefAccessorToOptionalViaConstValueAccessorToHoldingObject) { ExpectDiagnosticsFor(R"cc( #include "unchecked_optional_access_test.h" @@ -3975,7 +3979,8 @@ TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstValueAcces )cc"); } -TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObject) { +TEST_P(UncheckedOptionalAccessTest, + ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObject) { ExpectDiagnosticsFor(R"cc( #include "unchecked_optional_access_test.h" @@ -4003,7 +4008,9 @@ TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaNonConstRefAcce )cc"); } -TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObjectWithModAfterCheck) { +TEST_P( + UncheckedOptionalAccessTest, + ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObjectWithModAfterCheck) { ExpectDiagnosticsFor(R"cc( #include "unchecked_optional_access_test.h" _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
