From c2dc7cd31cea2a4732dfc811de85d169de712dec Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Tue, 4 May 2021 21:27:56 +0000 Subject: [PATCH] libutils: LightRefBase: incStrongRequireStrong Allow LightRefBase to be used with ANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION, mainly for libhwui. Bug: N/A Test: libutils_test Change-Id: I251c874a80f0a069572bc51da45f8f8e74ba6f5b --- libutils/Android.bp | 1 + libutils/LightRefBase.cpp | 29 ++++++++++++++ libutils/StrongPointer_test.cpp | 56 ++++++++++++++++++++------- libutils/include/utils/LightRefBase.h | 8 +++- 4 files changed, 78 insertions(+), 16 deletions(-) create mode 100644 libutils/LightRefBase.cpp diff --git a/libutils/Android.bp b/libutils/Android.bp index 62015693c..63955677d 100644 --- a/libutils/Android.bp +++ b/libutils/Android.bp @@ -141,6 +141,7 @@ cc_library { "Errors.cpp", "FileMap.cpp", "JenkinsHash.cpp", + "LightRefBase.cpp", "NativeHandle.cpp", "Printer.cpp", "RefBase.cpp", diff --git a/libutils/LightRefBase.cpp b/libutils/LightRefBase.cpp new file mode 100644 index 000000000..e08ffecfc --- /dev/null +++ b/libutils/LightRefBase.cpp @@ -0,0 +1,29 @@ +/* + * Copyright (C) 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#define LOG_TAG "LightRefBase" + +#include + +#include + +namespace android { + +void LightRefBase_reportIncStrongRequireStrongFailed(const void* thiz) { + LOG_ALWAYS_FATAL("incStrongRequireStrong() called on %p which isn't already owned", thiz); +} + +} // namespace android diff --git a/libutils/StrongPointer_test.cpp b/libutils/StrongPointer_test.cpp index 29f6bd4bb..f27c1f1cc 100644 --- a/libutils/StrongPointer_test.cpp +++ b/libutils/StrongPointer_test.cpp @@ -30,17 +30,34 @@ class SPFoo : virtual public RefBase { ~SPFoo() { *mDeleted = true; } -private: + + private: bool* mDeleted; }; -TEST(StrongPointer, move) { +class SPLightFoo : virtual public VirtualLightRefBase { + public: + explicit SPLightFoo(bool* deleted_check) : mDeleted(deleted_check) { *mDeleted = false; } + + ~SPLightFoo() { *mDeleted = true; } + + private: + bool* mDeleted; +}; + +template +class StrongPointer : public ::testing::Test {}; + +using RefBaseTypes = ::testing::Types; +TYPED_TEST_CASE(StrongPointer, RefBaseTypes); + +TYPED_TEST(StrongPointer, move) { bool isDeleted; - sp sp1 = sp::make(&isDeleted); - SPFoo* foo = sp1.get(); + sp sp1 = sp::make(&isDeleted); + TypeParam* foo = sp1.get(); ASSERT_EQ(1, foo->getStrongCount()); { - sp sp2 = std::move(sp1); + sp sp2 = std::move(sp1); ASSERT_EQ(1, foo->getStrongCount()) << "std::move failed, incremented refcnt"; ASSERT_EQ(nullptr, sp1.get()) << "std::move failed, sp1 is still valid"; // The strong count isn't increasing, let's double check the old object @@ -50,33 +67,42 @@ TEST(StrongPointer, move) { ASSERT_FALSE(isDeleted) << "deleted too early! still has a reference!"; { // Now let's double check it deletes on time - sp sp2 = std::move(sp1); + sp sp2 = std::move(sp1); } ASSERT_TRUE(isDeleted) << "foo was leaked!"; } -TEST(StrongPointer, NullptrComparison) { - sp foo; +TYPED_TEST(StrongPointer, NullptrComparison) { + sp foo; ASSERT_EQ(foo, nullptr); ASSERT_EQ(nullptr, foo); } -TEST(StrongPointer, PointerComparison) { +TYPED_TEST(StrongPointer, PointerComparison) { bool isDeleted; - sp foo = sp::make(&isDeleted); + sp foo = sp::make(&isDeleted); ASSERT_EQ(foo.get(), foo); ASSERT_EQ(foo, foo.get()); ASSERT_NE(nullptr, foo); ASSERT_NE(foo, nullptr); } -TEST(StrongPointer, AssertStrongRefExists) { - // uses some other refcounting method, or non at all +TYPED_TEST(StrongPointer, Deleted) { bool isDeleted; - SPFoo* foo = new SPFoo(&isDeleted); + sp foo = sp::make(&isDeleted); - // can only get a valid sp<> object when you construct it as an sp<> object - EXPECT_DEATH(sp::fromExisting(foo), ""); + auto foo2 = sp::fromExisting(foo.get()); + EXPECT_FALSE(isDeleted); + foo = nullptr; + EXPECT_FALSE(isDeleted); + foo2 = nullptr; + EXPECT_TRUE(isDeleted); +} + +TYPED_TEST(StrongPointer, AssertStrongRefExists) { + bool isDeleted; + TypeParam* foo = new TypeParam(&isDeleted); + EXPECT_DEATH(sp::fromExisting(foo), ""); delete foo; } diff --git a/libutils/include/utils/LightRefBase.h b/libutils/include/utils/LightRefBase.h index b04e5c155..40edf6718 100644 --- a/libutils/include/utils/LightRefBase.h +++ b/libutils/include/utils/LightRefBase.h @@ -28,6 +28,8 @@ namespace android { class ReferenceRenamer; +void LightRefBase_reportIncStrongRequireStrongFailed(const void* thiz); + template class LightRefBase { @@ -36,6 +38,11 @@ public: inline void incStrong(__attribute__((unused)) const void* id) const { mCount.fetch_add(1, std::memory_order_relaxed); } + inline void incStrongRequireStrong(__attribute__((unused)) const void* id) const { + if (0 == mCount.fetch_add(1, std::memory_order_relaxed)) { + LightRefBase_reportIncStrongRequireStrongFailed(this); + } + } inline void decStrong(__attribute__((unused)) const void* id) const { if (mCount.fetch_sub(1, std::memory_order_release) == 1) { std::atomic_thread_fence(std::memory_order_acquire); @@ -59,7 +66,6 @@ private: mutable std::atomic mCount; }; - // This is a wrapper around LightRefBase that simply enforces a virtual // destructor to eliminate the template requirement of LightRefBase class VirtualLightRefBase : public LightRefBase {