diff --git a/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel index 77ed2309f3..0de5fa0f81 100644 --- a/src/google/protobuf/BUILD.bazel +++ b/src/google/protobuf/BUILD.bazel @@ -810,6 +810,7 @@ cc_library( deps = [ ":cc_lite_test_protos", ":test_util2", + "@com_google_absl//absl/container:flat_hash_set", "@com_google_absl//absl/log:absl_check", "@com_google_googletest//:gtest", ], @@ -1296,9 +1297,13 @@ cc_test( }), deps = [ ":cc_test_protos", + ":lite_test_util", ":protobuf", + ":protobuf_lite", + "//src/google/protobuf/io", "//src/google/protobuf/stubs", "//src/google/protobuf/testing", + "@com_google_absl//absl/types:span", "@com_google_googletest//:gtest", "@com_google_googletest//:gtest_main", ], diff --git a/src/google/protobuf/arena_test_util.h b/src/google/protobuf/arena_test_util.h index 1065b62828..a5b4f93acf 100644 --- a/src/google/protobuf/arena_test_util.h +++ b/src/google/protobuf/arena_test_util.h @@ -31,6 +31,9 @@ #ifndef GOOGLE_PROTOBUF_ARENA_TEST_UTIL_H__ #define GOOGLE_PROTOBUF_ARENA_TEST_UTIL_H__ +#include + +#include "absl/container/flat_hash_set.h" #include "absl/log/absl_check.h" #include "google/protobuf/arena.h" #include "google/protobuf/io/coded_stream.h" @@ -84,6 +87,25 @@ struct ArenaTestPeer { } }; +struct CleanupGrowthInfo { + size_t space_used; + absl::flat_hash_set cleanups; +}; + +template +CleanupGrowthInfo CleanupGrowth(Arena& arena, Func f) { + auto old_space_used = arena.SpaceUsed(); + auto old_cleanups = ArenaTestPeer::PeekCleanupListForTesting(&arena); + f(); + auto new_space_used = arena.SpaceUsed(); + auto new_cleanups = ArenaTestPeer::PeekCleanupListForTesting(&arena); + CleanupGrowthInfo res; + res.space_used = new_space_used - old_space_used; + res.cleanups.insert(new_cleanups.begin(), new_cleanups.end()); + for (auto p : old_cleanups) res.cleanups.erase(p); + return res; +} + class NoHeapChecker { public: NoHeapChecker() { capture_alloc.Hook(); } diff --git a/src/google/protobuf/map_test.cc b/src/google/protobuf/map_test.cc index 7dffe21e7b..0f4a4f008c 100644 --- a/src/google/protobuf/map_test.cc +++ b/src/google/protobuf/map_test.cc @@ -28,6 +28,7 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +#include #include #include diff --git a/src/google/protobuf/repeated_field.h b/src/google/protobuf/repeated_field.h index d5e531a197..4acbe87b75 100644 --- a/src/google/protobuf/repeated_field.h +++ b/src/google/protobuf/repeated_field.h @@ -137,6 +137,16 @@ void memswap(char* a, char* b) { template class RepeatedIterator; +// We can't skip the destructor for, e.g., arena allocated RepeatedField. +template ::value> +struct RepeatedFieldDestructorSkippableBase {}; + +template +struct RepeatedFieldDestructorSkippableBase { + using DestructorSkippable_ = void; +}; + } // namespace internal // RepeatedField is used to represent repeated fields of a primitive type (in @@ -148,7 +158,8 @@ class RepeatedIterator; // We have to specialize several methods in the Cord case to get the memory // management right; e.g. swapping when appropriate, etc. template -class RepeatedField final { +class RepeatedField final + : private internal::RepeatedFieldDestructorSkippableBase { static_assert( alignof(Arena) >= alignof(Element), "We only support types that have an alignment smaller than Arena"); diff --git a/src/google/protobuf/repeated_field_unittest.cc b/src/google/protobuf/repeated_field_unittest.cc index f3b4f23b4b..bcac69ab9c 100644 --- a/src/google/protobuf/repeated_field_unittest.cc +++ b/src/google/protobuf/repeated_field_unittest.cc @@ -38,6 +38,8 @@ #include "google/protobuf/repeated_field.h" #include +#include +#include #include #include #include @@ -45,14 +47,23 @@ #include #include #include +#include #include +#include "google/protobuf/arena.h" #include #include #include "absl/log/absl_check.h" #include "absl/numeric/bits.h" #include "absl/strings/cord.h" #include "absl/strings/str_cat.h" +#include "absl/types/span.h" +#include "google/protobuf/arena_test_util.h" +#include "google/protobuf/io/coded_stream.h" +#include "google/protobuf/io/zero_copy_stream_impl_lite.h" +#include "google/protobuf/parse_context.h" +#include "google/protobuf/repeated_field.h" +#include "google/protobuf/repeated_ptr_field.h" #include "google/protobuf/unittest.pb.h" @@ -1211,6 +1222,19 @@ TEST(RepeatedField, PoisonsMemoryOnAssign) { #endif +TEST(RepeatedField, Cleanups) { + Arena arena; + auto growth = internal::CleanupGrowth( + arena, [&] { Arena::CreateMessage>(&arena); }); + EXPECT_THAT(growth.cleanups, testing::IsEmpty()); + + void* ptr; + growth = internal::CleanupGrowth(arena, [&] { + ptr = Arena::CreateMessage>(&arena); + }); + EXPECT_THAT(growth.cleanups, testing::UnorderedElementsAre(ptr)); +} + // =================================================================== // RepeatedPtrField tests. These pretty much just mirror the RepeatedField // tests above. @@ -1992,6 +2016,19 @@ TEST(RepeatedPtrField, DeleteSubrange) { // DeleteSubrange is a trivial extension of ExtendSubrange. } +TEST(RepeatedPtrField, Cleanups) { + Arena arena; + auto growth = internal::CleanupGrowth(arena, [&] { + Arena::CreateMessage>(&arena); + }); + EXPECT_THAT(growth.cleanups, testing::IsEmpty()); + + growth = internal::CleanupGrowth(arena, [&] { + Arena::CreateMessage>(&arena); + }); + EXPECT_THAT(growth.cleanups, testing::IsEmpty()); +} + // =================================================================== // Iterator tests stolen from net/proto/proto-array_unittest. diff --git a/src/google/protobuf/repeated_ptr_field.h b/src/google/protobuf/repeated_ptr_field.h index 9a7dc60a45..b454a442ce 100644 --- a/src/google/protobuf/repeated_ptr_field.h +++ b/src/google/protobuf/repeated_ptr_field.h @@ -1224,7 +1224,8 @@ class RepeatedPtrField final : private internal::RepeatedPtrFieldBase { template friend struct WeakRepeatedPtrField; - typedef void InternalArenaConstructable_; + using InternalArenaConstructable_ = void; + using DestructorSkippable_ = void; };