From b0fbe1aff379925c702af82db0706a6add1d12a6 Mon Sep 17 00:00:00 2001 From: Michael Lippautz Date: Mon, 2 May 2022 18:21:20 +0200 Subject: [PATCH] cppgc: Allow overriding heap object name at runtime Before this CL, the heap object name of unnamed objects(those not inheriting from NameProvider) would be solely determined by whether the build-time configuration cppgc_enable_object_names is enabled. This patch adds a way to override that value at runtime. This is useful for preserving default behavior with custom builds but at the same time allow them to still enable the feature. Bug: chromium:1321620 Change-Id: I3aa06db15e58d9ba9773be6797572f17f007e9ee Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3620279 Reviewed-by: Anton Bikineev Commit-Queue: Michael Lippautz Reviewed-by: Omer Katz Cr-Commit-Position: refs/heads/main@{#80338} --- include/cppgc/internal/name-trait.h | 25 ++++++-- include/cppgc/name-provider.h | 8 +-- src/heap/cppgc/gc-info.cc | 2 +- src/heap/cppgc/heap-base.h | 10 ++++ src/heap/cppgc/heap-object-header.cc | 5 +- src/heap/cppgc/heap-statistics-collector.cc | 6 +- .../unified-heap-snapshot-unittest.cc | 4 +- .../heap/cppgc/name-trait-unittest.cc | 59 +++++++++++++------ 8 files changed, 85 insertions(+), 34 deletions(-) diff --git a/include/cppgc/internal/name-trait.h b/include/cppgc/internal/name-trait.h index 32a3347859..ece49cbe75 100644 --- a/include/cppgc/internal/name-trait.h +++ b/include/cppgc/internal/name-trait.h @@ -6,6 +6,7 @@ #define INCLUDE_CPPGC_INTERNAL_NAME_TRAIT_H_ #include +#include #include #include "cppgc/name-provider.h" @@ -58,6 +59,11 @@ struct HeapObjectName { bool name_was_hidden; }; +enum class HeapObjectNameForUnnamedObject : uint8_t { + kUseClassNameIfSupported, + kUseHiddenName, +}; + class V8_EXPORT NameTraitBase { protected: static HeapObjectName GetNameFromTypeSignature(const char*); @@ -78,16 +84,24 @@ class NameTrait final : public NameTraitBase { #endif // !CPPGC_SUPPORTS_OBJECT_NAMES } - static HeapObjectName GetName(const void* obj) { - return GetNameFor(static_cast(obj)); + static HeapObjectName GetName( + const void* obj, HeapObjectNameForUnnamedObject name_retrieval_mode) { + return GetNameFor(static_cast(obj), name_retrieval_mode); } private: - static HeapObjectName GetNameFor(const NameProvider* name_provider) { + static HeapObjectName GetNameFor(const NameProvider* name_provider, + HeapObjectNameForUnnamedObject) { + // Objects inheriting from `NameProvider` are not considered unnamed as + // users already provided a name for them. return {name_provider->GetHumanReadableName(), false}; } - static HeapObjectName GetNameFor(...) { + static HeapObjectName GetNameFor( + const void*, HeapObjectNameForUnnamedObject name_retrieval_mode) { + if (name_retrieval_mode == HeapObjectNameForUnnamedObject::kUseHiddenName) + return {NameProvider::kHiddenName, true}; + #if CPPGC_SUPPORTS_COMPILE_TIME_TYPENAME return {GetTypename(), false}; #elif CPPGC_SUPPORTS_OBJECT_NAMES @@ -112,7 +126,8 @@ class NameTrait final : public NameTraitBase { } }; -using NameCallback = HeapObjectName (*)(const void*); +using NameCallback = HeapObjectName (*)(const void*, + HeapObjectNameForUnnamedObject); } // namespace internal } // namespace cppgc diff --git a/include/cppgc/name-provider.h b/include/cppgc/name-provider.h index 224dd4b5d6..216f6098d9 100644 --- a/include/cppgc/name-provider.h +++ b/include/cppgc/name-provider.h @@ -37,15 +37,15 @@ class V8_EXPORT NameProvider { static constexpr const char kNoNameDeducible[] = ""; /** - * Indicating whether internal names are hidden or not. + * Indicating whether the build supports extracting C++ names as object names. * * @returns true if C++ names should be hidden and represented by kHiddenName. */ - static constexpr bool HideInternalNames() { + static constexpr bool SupportsCppClassNamesAsObjectNames() { #if CPPGC_SUPPORTS_OBJECT_NAMES - return false; -#else // !CPPGC_SUPPORTS_OBJECT_NAMES return true; +#else // !CPPGC_SUPPORTS_OBJECT_NAMES + return false; #endif // !CPPGC_SUPPORTS_OBJECT_NAMES } diff --git a/src/heap/cppgc/gc-info.cc b/src/heap/cppgc/gc-info.cc index 4c555106fd..ddb294cb5c 100644 --- a/src/heap/cppgc/gc-info.cc +++ b/src/heap/cppgc/gc-info.cc @@ -13,7 +13,7 @@ namespace internal { namespace { -HeapObjectName GetHiddenName(const void*) { +HeapObjectName GetHiddenName(const void*, HeapObjectNameForUnnamedObject) { return {NameProvider::kHiddenName, true}; } diff --git a/src/heap/cppgc/heap-base.h b/src/heap/cppgc/heap-base.h index bfd1787675..9d644c1585 100644 --- a/src/heap/cppgc/heap-base.h +++ b/src/heap/cppgc/heap-base.h @@ -73,6 +73,8 @@ class PageBackend; class PreFinalizerHandler; class StatsCollector; +enum class HeapObjectNameForUnnamedObject : uint8_t; + // Base class for heap implementations. class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle { public: @@ -224,6 +226,14 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle { return supported; } + // Returns whether objects should derive their name from C++ class names. Also + // requires build-time support through `CPPGC_SUPPORTS_OBJECT_NAMES`. + HeapObjectNameForUnnamedObject name_of_unnamed_object() const { + // TODO(chromium:1321620): Wire up flag with heap snapshot if exposing + // internals is requested. + return HeapObjectNameForUnnamedObject::kUseClassNameIfSupported; + } + protected: enum class GenerationSupport : uint8_t { kSingleGeneration, diff --git a/src/heap/cppgc/heap-object-header.cc b/src/heap/cppgc/heap-object-header.cc index 5ff0e230e7..fd4422144f 100644 --- a/src/heap/cppgc/heap-object-header.cc +++ b/src/heap/cppgc/heap-object-header.cc @@ -8,6 +8,7 @@ #include "src/base/macros.h" #include "src/base/sanitizer/asan.h" #include "src/heap/cppgc/gc-info-table.h" +#include "src/heap/cppgc/heap-base.h" #include "src/heap/cppgc/heap-page.h" namespace cppgc { @@ -38,7 +39,9 @@ void HeapObjectHeader::Finalize() { HeapObjectName HeapObjectHeader::GetName() const { const GCInfo& gc_info = GlobalGCInfoTable::GCInfoFromIndex(GetGCInfoIndex()); - return gc_info.name(ObjectStart()); + return gc_info.name( + ObjectStart(), + BasePage::FromPayload(this)->heap().name_of_unnamed_object()); } } // namespace internal diff --git a/src/heap/cppgc/heap-statistics-collector.cc b/src/heap/cppgc/heap-statistics-collector.cc index 7c91bbd4f5..ac668ee4ac 100644 --- a/src/heap/cppgc/heap-statistics-collector.cc +++ b/src/heap/cppgc/heap-statistics-collector.cc @@ -76,7 +76,7 @@ void RecordObjectType( std::unordered_map& type_map, std::vector& object_statistics, HeapObjectHeader* header, size_t object_size) { - if (!NameProvider::HideInternalNames()) { + if (NameProvider::SupportsCppClassNamesAsObjectNames()) { // Tries to insert a new entry into the typemap with a running counter. If // the entry is already present, just returns the old one. const auto it = type_map.insert({header->GetName().value, type_map.size()}); @@ -97,7 +97,7 @@ HeapStatistics HeapStatisticsCollector::CollectDetailedStatistics( stats.detail_level = HeapStatistics::DetailLevel::kDetailed; current_stats_ = &stats; - if (!NameProvider::HideInternalNames()) { + if (NameProvider::SupportsCppClassNamesAsObjectNames()) { // Add a dummy type so that a type index of zero has a valid mapping but // shows an invalid type. type_name_to_index_map_.insert({"Invalid type", 0}); @@ -106,7 +106,7 @@ HeapStatistics HeapStatisticsCollector::CollectDetailedStatistics( Traverse(heap->raw_heap()); FinalizeSpace(current_stats_, ¤t_space_stats_, ¤t_page_stats_); - if (!NameProvider::HideInternalNames()) { + if (NameProvider::SupportsCppClassNamesAsObjectNames()) { stats.type_names.resize(type_name_to_index_map_.size()); for (auto& it : type_name_to_index_map_) { stats.type_names[it.second] = reinterpret_cast(it.first); diff --git a/test/unittests/heap/cppgc-js/unified-heap-snapshot-unittest.cc b/test/unittests/heap/cppgc-js/unified-heap-snapshot-unittest.cc index 34617cf443..30e4913db5 100644 --- a/test/unittests/heap/cppgc-js/unified-heap-snapshot-unittest.cc +++ b/test/unittests/heap/cppgc-js/unified-heap-snapshot-unittest.cc @@ -120,7 +120,7 @@ constexpr const char kExpectedCppCrossThreadRootsName[] = template constexpr const char* GetExpectedName() { if (std::is_base_of::value || - !cppgc::NameProvider::HideInternalNames()) { + cppgc::NameProvider::SupportsCppClassNamesAsObjectNames()) { return T::kExpectedName; } else { return cppgc::NameProvider::kHiddenName; @@ -157,7 +157,7 @@ TEST_F(UnifiedHeapSnapshotTest, RetainingUnnamedType) { cppgc::MakeGarbageCollected(allocation_handle()); const v8::HeapSnapshot* snapshot = TakeHeapSnapshot(); EXPECT_TRUE(IsValidSnapshot(snapshot)); - if (cppgc::NameProvider::HideInternalNames()) { + if (!cppgc::NameProvider::SupportsCppClassNamesAsObjectNames()) { EXPECT_FALSE(ContainsRetainingPath( *snapshot, {kExpectedCppRootsName, cppgc::NameProvider::kHiddenName})); } else { diff --git a/test/unittests/heap/cppgc/name-trait-unittest.cc b/test/unittests/heap/cppgc/name-trait-unittest.cc index 6ef0101efe..f197b140ce 100644 --- a/test/unittests/heap/cppgc/name-trait-unittest.cc +++ b/test/unittests/heap/cppgc/name-trait-unittest.cc @@ -50,20 +50,43 @@ TEST(NameTraitTest, InternalNamesHiddenInOfficialBuild) { } TEST(NameTraitTest, DefaultName) { - EXPECT_STREQ(NameProvider::HideInternalNames() - ? "InternalNode" - : "cppgc::internal::(anonymous namespace)::NoName", - NameTrait::GetName(nullptr).value); - EXPECT_STREQ(NameProvider::HideInternalNames() - ? "InternalNode" - : "cppgc::internal::(anonymous namespace)::OtherNoName", - NameTrait::GetName(nullptr).value); + EXPECT_STREQ( + NameProvider::SupportsCppClassNamesAsObjectNames() + ? "cppgc::internal::(anonymous namespace)::NoName" + : "InternalNode", + NameTrait::GetName( + nullptr, HeapObjectNameForUnnamedObject::kUseClassNameIfSupported) + .value); + EXPECT_STREQ( + NameProvider::SupportsCppClassNamesAsObjectNames() + ? "cppgc::internal::(anonymous namespace)::OtherNoName" + : "InternalNode", + NameTrait::GetName( + nullptr, HeapObjectNameForUnnamedObject::kUseClassNameIfSupported) + .value); + // The following ignores `NameProvider::HideInternalNames()` and just always + // returns the hidden name, independent of the build support. + EXPECT_STREQ("InternalNode", + NameTrait::GetName( + nullptr, HeapObjectNameForUnnamedObject::kUseHiddenName) + .value); + EXPECT_STREQ("InternalNode", + NameTrait::GetName( + nullptr, HeapObjectNameForUnnamedObject::kUseHiddenName) + .value); } TEST(NameTraitTest, CustomName) { ClassWithName with_name("CustomName"); - const char* name = NameTrait::GetName(&with_name).value; - EXPECT_STREQ("CustomName", name); + EXPECT_STREQ( + "CustomName", + NameTrait::GetName( + &with_name, HeapObjectNameForUnnamedObject::kUseClassNameIfSupported) + .value); + EXPECT_STREQ("CustomName", + NameTrait::GetName( + &with_name, HeapObjectNameForUnnamedObject::kUseHiddenName) + .value); } namespace { @@ -99,26 +122,26 @@ class HeapObjectHeaderNameTest : public testing::TestWithHeap {}; TEST_F(HeapObjectHeaderNameTest, LookupNameThroughGCInfo) { auto* no_name = MakeGarbageCollected(GetAllocationHandle()); auto no_name_tuple = HeapObjectHeader::FromObject(no_name).GetName(); - if (NameProvider::HideInternalNames()) { - EXPECT_STREQ(NameProvider::kHiddenName, no_name_tuple.value); - EXPECT_TRUE(no_name_tuple.name_was_hidden); - } else { + if (NameProvider::SupportsCppClassNamesAsObjectNames()) { EXPECT_STREQ("cppgc::internal::(anonymous namespace)::NoName", no_name_tuple.value); EXPECT_FALSE(no_name_tuple.name_was_hidden); + } else { + EXPECT_STREQ(NameProvider::kHiddenName, no_name_tuple.value); + EXPECT_TRUE(no_name_tuple.name_was_hidden); } auto* other_no_name = MakeGarbageCollected(GetAllocationHandle()); auto other_no_name_tuple = HeapObjectHeader::FromObject(other_no_name).GetName(); - if (NameProvider::HideInternalNames()) { - EXPECT_STREQ(NameProvider::kHiddenName, no_name_tuple.value); - EXPECT_TRUE(no_name_tuple.name_was_hidden); - } else { + if (NameProvider::SupportsCppClassNamesAsObjectNames()) { EXPECT_STREQ("cppgc::internal::(anonymous namespace)::OtherNoName", other_no_name_tuple.value); EXPECT_FALSE(other_no_name_tuple.name_was_hidden); + } else { + EXPECT_STREQ(NameProvider::kHiddenName, no_name_tuple.value); + EXPECT_TRUE(no_name_tuple.name_was_hidden); } auto* class_with_name =