From 719a00df3a5d5696417acdb8883e9baa9a9a0446 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Fri, 22 Sep 2023 02:04:16 +0200 Subject: [PATCH] LibJS: Add source location for Handle nodes in GC graph dumper output With this change JS::Handle root nodes will contain source location where they were constructed like: ``` "94675029575744": { "root": "Handle activate_event_handler \ serenity/Userland/Libraries/LibWeb/DOM/EventTarget.cpp:564", "class_name": "HTMLButtonElement", "edges": [ "94675025955904", "94675026899520", "94675030831168", ``` --- Userland/Libraries/LibJS/Heap/Handle.cpp | 3 +- Userland/Libraries/LibJS/Heap/Handle.h | 54 ++++++----- Userland/Libraries/LibJS/Heap/Heap.cpp | 93 +++++++++---------- Userland/Libraries/LibJS/Heap/Heap.h | 10 +- Userland/Libraries/LibJS/Heap/HeapRoot.h | 28 ++++++ .../LibJS/Heap/HeapRootTypeOrLocation.h | 24 ----- Userland/Libraries/LibJS/Heap/MarkedVector.h | 10 +- Userland/Libraries/LibJS/Runtime/VM.cpp | 26 +++--- Userland/Libraries/LibJS/Runtime/VM.h | 2 +- 9 files changed, 129 insertions(+), 121 deletions(-) create mode 100644 Userland/Libraries/LibJS/Heap/HeapRoot.h delete mode 100644 Userland/Libraries/LibJS/Heap/HeapRootTypeOrLocation.h diff --git a/Userland/Libraries/LibJS/Heap/Handle.cpp b/Userland/Libraries/LibJS/Heap/Handle.cpp index c440374e4d..76dcdf7c0b 100644 --- a/Userland/Libraries/LibJS/Heap/Handle.cpp +++ b/Userland/Libraries/LibJS/Heap/Handle.cpp @@ -10,8 +10,9 @@ namespace JS { -HandleImpl::HandleImpl(Cell* cell) +HandleImpl::HandleImpl(Cell* cell, SourceLocation location) : m_cell(cell) + , m_location(location) { m_cell->heap().did_create_handle({}, *this); } diff --git a/Userland/Libraries/LibJS/Heap/Handle.h b/Userland/Libraries/LibJS/Heap/Handle.h index f816aa9ef8..36be3bf0ae 100644 --- a/Userland/Libraries/LibJS/Heap/Handle.h +++ b/Userland/Libraries/LibJS/Heap/Handle.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -26,12 +27,15 @@ public: Cell* cell() { return m_cell; } Cell const* cell() const { return m_cell; } + SourceLocation const& source_location() const { return m_location; } + private: template friend class Handle; - explicit HandleImpl(Cell*); + explicit HandleImpl(Cell*, SourceLocation location); GCPtr m_cell; + SourceLocation m_location; IntrusiveListNode m_list_node; @@ -44,29 +48,29 @@ class Handle { public: Handle() = default; - static Handle create(T* cell) + static Handle create(T* cell, SourceLocation location = SourceLocation::current()) { - return Handle(adopt_ref(*new HandleImpl(const_cast*>(cell)))); + return Handle(adopt_ref(*new HandleImpl(const_cast*>(cell), location))); } - Handle(T* cell) + Handle(T* cell, SourceLocation location = SourceLocation::current()) { if (cell) - m_impl = adopt_ref(*new HandleImpl(cell)); + m_impl = adopt_ref(*new HandleImpl(cell, location)); } - Handle(T& cell) - : m_impl(adopt_ref(*new HandleImpl(&cell))) + Handle(T& cell, SourceLocation location = SourceLocation::current()) + : m_impl(adopt_ref(*new HandleImpl(&cell, location))) { } - Handle(GCPtr cell) - : Handle(cell.ptr()) + Handle(GCPtr cell, SourceLocation location = SourceLocation::current()) + : Handle(cell.ptr(), location) { } - Handle(NonnullGCPtr cell) - : Handle(*cell) + Handle(NonnullGCPtr cell, SourceLocation location = SourceLocation::current()) + : Handle(*cell, location) { } @@ -118,31 +122,31 @@ private: }; template -inline Handle make_handle(T* cell) +inline Handle make_handle(T* cell, SourceLocation location = SourceLocation::current()) { if (!cell) return Handle {}; - return Handle::create(cell); + return Handle::create(cell, location); } template -inline Handle make_handle(T& cell) +inline Handle make_handle(T& cell, SourceLocation location = SourceLocation::current()) { - return Handle::create(&cell); + return Handle::create(&cell, location); } template -inline Handle make_handle(GCPtr cell) +inline Handle make_handle(GCPtr cell, SourceLocation location = SourceLocation::current()) { if (!cell) return Handle {}; - return Handle::create(cell.ptr()); + return Handle::create(cell.ptr(), location); } template -inline Handle make_handle(NonnullGCPtr cell) +inline Handle make_handle(NonnullGCPtr cell, SourceLocation location = SourceLocation::current()) { - return Handle::create(cell.ptr()); + return Handle::create(cell.ptr(), location); } template<> @@ -150,10 +154,10 @@ class Handle { public: Handle() = default; - static Handle create(Value value) + static Handle create(Value value, SourceLocation location) { if (value.is_cell()) - return Handle(value, &value.as_cell()); + return Handle(value, &value.as_cell(), location); return Handle(value); } @@ -171,9 +175,9 @@ private: { } - explicit Handle(Value value, Cell* cell) + explicit Handle(Value value, Cell* cell, SourceLocation location) : m_value(value) - , m_handle(Handle::create(cell)) + , m_handle(Handle::create(cell, location)) { } @@ -181,9 +185,9 @@ private: Handle m_handle; }; -inline Handle make_handle(Value value) +inline Handle make_handle(Value value, SourceLocation location = SourceLocation::current()) { - return Handle::create(value); + return Handle::create(value, location); } } diff --git a/Userland/Libraries/LibJS/Heap/Heap.cpp b/Userland/Libraries/LibJS/Heap/Heap.cpp index ba98c73f33..14234b6d72 100644 --- a/Userland/Libraries/LibJS/Heap/Heap.cpp +++ b/Userland/Libraries/LibJS/Heap/Heap.cpp @@ -95,7 +95,7 @@ Cell* Heap::allocate_cell(size_t size) return allocator.allocate_cell(*this); } -static void add_possible_value(HashMap& possible_pointers, FlatPtr data, HeapRootTypeOrLocation origin) +static void add_possible_value(HashMap& possible_pointers, FlatPtr data, HeapRoot origin) { if constexpr (sizeof(FlatPtr*) == sizeof(Value)) { // Because Value stores pointers in non-canonical form we have to check if the top bytes @@ -114,7 +114,7 @@ static void add_possible_value(HashMap& possibl } template -static void for_each_cell_among_possible_pointers(HashTable const& all_live_heap_blocks, HashMap& possible_pointers, Callback callback) +static void for_each_cell_among_possible_pointers(HashTable const& all_live_heap_blocks, HashMap& possible_pointers, Callback callback) { for (auto possible_pointer : possible_pointers.keys()) { if (!possible_pointer) @@ -130,7 +130,7 @@ static void for_each_cell_among_possible_pointers(HashTable const& a class GraphConstructorVisitor final : public Cell::Visitor { public: - explicit GraphConstructorVisitor(Heap& heap, HashMap const& roots) + explicit GraphConstructorVisitor(Heap& heap, HashMap const& roots) : m_heap(heap) { m_heap.for_each_block([&](auto& block) { @@ -159,11 +159,11 @@ public: virtual void visit_possible_values(ReadonlyBytes bytes) override { - HashMap possible_pointers; + HashMap possible_pointers; auto* raw_pointer_sized_values = reinterpret_cast(bytes.data()); for (size_t i = 0; i < (bytes.size() / sizeof(FlatPtr)); ++i) - add_possible_value(possible_pointers, raw_pointer_sized_values[i], HeapRootType::HeapFunctionCapturedPointer); + add_possible_value(possible_pointers, raw_pointer_sized_values[i], HeapRoot { .type = HeapRoot::Type::HeapFunctionCapturedPointer }); for_each_cell_among_possible_pointers(m_all_live_heap_blocks, possible_pointers, [&](Cell* cell, FlatPtr) { if (m_node_being_visited) @@ -197,31 +197,30 @@ public: auto node = AK::JsonObject(); if (it.value.root_origin.has_value()) { - it.value.root_origin->visit( - [&](HeapRootType location) { - switch (location) { - case HeapRootType::Handle: - node.set("root"sv, "Handle"sv); - return; - case HeapRootType::MarkedVector: - node.set("root"sv, "MarkedVector"); - return; - case HeapRootType::RegisterPointer: - node.set("root"sv, "RegisterPointer"); - return; - case HeapRootType::StackPointer: - node.set("root"sv, "StackPointer"); - return; - case HeapRootType::VM: - node.set("root"sv, "VM"); - return; - default: - VERIFY_NOT_REACHED(); - } - }, - [&](SourceLocation* location) { - node.set("root", DeprecatedString::formatted("SafeFunction {} {}:{}", location->function_name(), location->filename(), location->line_number())); - }); + auto type = it.value.root_origin->type; + auto location = it.value.root_origin->location; + switch (type) { + case HeapRoot::Type::Handle: + node.set("root"sv, DeprecatedString::formatted("Handle {} {}:{}", location->function_name(), location->filename(), location->line_number())); + break; + case HeapRoot::Type::MarkedVector: + node.set("root"sv, "MarkedVector"); + break; + case HeapRoot::Type::RegisterPointer: + node.set("root"sv, "RegisterPointer"); + break; + case HeapRoot::Type::StackPointer: + node.set("root"sv, "StackPointer"); + break; + case HeapRoot::Type::VM: + node.set("root"sv, "VM"); + break; + case HeapRoot::Type::SafeFunction: + node.set("root"sv, DeprecatedString::formatted("SafeFunction {} {}:{}", location->function_name(), location->filename(), location->line_number())); + break; + default: + VERIFY_NOT_REACHED(); + } } node.set("class_name"sv, it.value.class_name); node.set("edges"sv, edges); @@ -233,7 +232,7 @@ public: private: struct GraphNode { - Optional root_origin; + Optional root_origin; StringView class_name; HashTable edges {}; }; @@ -248,7 +247,7 @@ private: void Heap::dump_graph() { - HashMap roots; + HashMap roots; gather_roots(roots); GraphConstructorVisitor visitor(*this, roots); vm().bytecode_interpreter().visit_edges(visitor); @@ -275,7 +274,7 @@ void Heap::collect_garbage(CollectionType collection_type, bool print_report) m_should_gc_when_deferral_ends = true; return; } - HashMap roots; + HashMap roots; gather_roots(roots); mark_live_cells(roots); } @@ -283,13 +282,13 @@ void Heap::collect_garbage(CollectionType collection_type, bool print_report) sweep_dead_cells(print_report, collection_measurement_timer); } -void Heap::gather_roots(HashMap& roots) +void Heap::gather_roots(HashMap& roots) { vm().gather_roots(roots); gather_conservative_roots(roots); for (auto& handle : m_handles) - roots.set(handle.cell(), HeapRootType::Handle); + roots.set(handle.cell(), HeapRoot { .type = HeapRoot::Type::Handle, .location = &handle.source_location() }); for (auto& vector : m_marked_vectors) vector.gather_roots(roots); @@ -302,7 +301,7 @@ void Heap::gather_roots(HashMap& roots) } #ifdef HAS_ADDRESS_SANITIZER -__attribute__((no_sanitize("address"))) void Heap::gather_asan_fake_stack_roots(HashMap& possible_pointers, FlatPtr addr) +__attribute__((no_sanitize("address"))) void Heap::gather_asan_fake_stack_roots(HashMap& possible_pointers, FlatPtr addr) { void* begin = nullptr; void* end = nullptr; @@ -313,17 +312,17 @@ __attribute__((no_sanitize("address"))) void Heap::gather_asan_fake_stack_roots( void const* real_address = *real_stack_addr; if (real_address == nullptr) continue; - add_possible_value(possible_pointers, reinterpret_cast(real_address), HeapRootType::StackPointer); + add_possible_value(possible_pointers, reinterpret_cast(real_address), HeapRoot { .type = HeapRoot::Type::StackPointer }); } } } #else -void Heap::gather_asan_fake_stack_roots(HashMap&, FlatPtr) +void Heap::gather_asan_fake_stack_roots(HashMap&, FlatPtr) { } #endif -__attribute__((no_sanitize("address"))) void Heap::gather_conservative_roots(HashMap& roots) +__attribute__((no_sanitize("address"))) void Heap::gather_conservative_roots(HashMap& roots) { FlatPtr dummy; @@ -332,19 +331,19 @@ __attribute__((no_sanitize("address"))) void Heap::gather_conservative_roots(Has jmp_buf buf; setjmp(buf); - HashMap possible_pointers; + HashMap possible_pointers; auto* raw_jmp_buf = reinterpret_cast(buf); for (size_t i = 0; i < ((size_t)sizeof(buf)) / sizeof(FlatPtr); ++i) - add_possible_value(possible_pointers, raw_jmp_buf[i], HeapRootType::RegisterPointer); + add_possible_value(possible_pointers, raw_jmp_buf[i], HeapRoot { .type = HeapRoot::Type::RegisterPointer }); auto stack_reference = bit_cast(&dummy); auto& stack_info = m_vm.stack_info(); for (FlatPtr stack_address = stack_reference; stack_address < stack_info.top(); stack_address += sizeof(FlatPtr)) { auto data = *reinterpret_cast(stack_address); - add_possible_value(possible_pointers, data, HeapRootType::StackPointer); + add_possible_value(possible_pointers, data, HeapRoot { .type = HeapRoot::Type::StackPointer }); gather_asan_fake_stack_roots(possible_pointers, data); } @@ -354,7 +353,7 @@ __attribute__((no_sanitize("address"))) void Heap::gather_conservative_roots(Has for (auto& custom_range : *s_custom_ranges_for_conservative_scan) { for (size_t i = 0; i < (custom_range.value / sizeof(FlatPtr)); ++i) { auto safe_function_location = s_safe_function_locations->get(custom_range.key); - add_possible_value(possible_pointers, custom_range.key[i], *safe_function_location); + add_possible_value(possible_pointers, custom_range.key[i], HeapRoot { .type = HeapRoot::Type::SafeFunction, .location = *safe_function_location }); } } } @@ -377,7 +376,7 @@ __attribute__((no_sanitize("address"))) void Heap::gather_conservative_roots(Has class MarkingVisitor final : public Cell::Visitor { public: - explicit MarkingVisitor(Heap& heap, HashMap const& roots) + explicit MarkingVisitor(Heap& heap, HashMap const& roots) : m_heap(heap) { m_heap.for_each_block([&](auto& block) { @@ -402,11 +401,11 @@ public: virtual void visit_possible_values(ReadonlyBytes bytes) override { - HashMap possible_pointers; + HashMap possible_pointers; auto* raw_pointer_sized_values = reinterpret_cast(bytes.data()); for (size_t i = 0; i < (bytes.size() / sizeof(FlatPtr)); ++i) - add_possible_value(possible_pointers, raw_pointer_sized_values[i], HeapRootType::HeapFunctionCapturedPointer); + add_possible_value(possible_pointers, raw_pointer_sized_values[i], HeapRoot { .type = HeapRoot::Type::HeapFunctionCapturedPointer }); for_each_cell_among_possible_pointers(m_all_live_heap_blocks, possible_pointers, [&](Cell* cell, FlatPtr) { if (cell->is_marked()) @@ -431,7 +430,7 @@ private: HashTable m_all_live_heap_blocks; }; -void Heap::mark_live_cells(HashMap const& roots) +void Heap::mark_live_cells(HashMap const& roots) { dbgln_if(HEAP_DEBUG, "mark_live_cells:"); diff --git a/Userland/Libraries/LibJS/Heap/Heap.h b/Userland/Libraries/LibJS/Heap/Heap.h index b32a35dd4b..c92e68a4c9 100644 --- a/Userland/Libraries/LibJS/Heap/Heap.h +++ b/Userland/Libraries/LibJS/Heap/Heap.h @@ -19,7 +19,7 @@ #include #include #include -#include +#include #include #include #include @@ -88,10 +88,10 @@ private: Cell* allocate_cell(size_t); - void gather_roots(HashMap&); - void gather_conservative_roots(HashMap&); - void gather_asan_fake_stack_roots(HashMap&, FlatPtr); - void mark_live_cells(HashMap const& live_cells); + void gather_roots(HashMap&); + void gather_conservative_roots(HashMap&); + void gather_asan_fake_stack_roots(HashMap&, FlatPtr); + void mark_live_cells(HashMap const& live_cells); void finalize_unmarked_cells(); void sweep_dead_cells(bool print_report, Core::ElapsedTimer const&); diff --git a/Userland/Libraries/LibJS/Heap/HeapRoot.h b/Userland/Libraries/LibJS/Heap/HeapRoot.h new file mode 100644 index 0000000000..1d88a58097 --- /dev/null +++ b/Userland/Libraries/LibJS/Heap/HeapRoot.h @@ -0,0 +1,28 @@ +/* + * Copyright (c) 2023, Aliaksandr Kalenik + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#pragma once + +#include + +namespace JS { + +struct HeapRoot { + enum class Type { + HeapFunctionCapturedPointer, + Handle, + MarkedVector, + RegisterPointer, + SafeFunction, + StackPointer, + VM, + }; + + Type type; + SourceLocation const* location { nullptr }; +}; + +} diff --git a/Userland/Libraries/LibJS/Heap/HeapRootTypeOrLocation.h b/Userland/Libraries/LibJS/Heap/HeapRootTypeOrLocation.h deleted file mode 100644 index 2e1c2e5b1f..0000000000 --- a/Userland/Libraries/LibJS/Heap/HeapRootTypeOrLocation.h +++ /dev/null @@ -1,24 +0,0 @@ -/* - * Copyright (c) 2023, Aliaksandr Kalenik - * - * SPDX-License-Identifier: BSD-2-Clause - */ - -#pragma once - -#include - -namespace JS { - -enum class HeapRootType { - HeapFunctionCapturedPointer, - Handle, - MarkedVector, - RegisterPointer, - StackPointer, - VM, -}; - -using HeapRootTypeOrLocation = Variant; - -} diff --git a/Userland/Libraries/LibJS/Heap/MarkedVector.h b/Userland/Libraries/LibJS/Heap/MarkedVector.h index 5546c6d7fc..5df39022ee 100644 --- a/Userland/Libraries/LibJS/Heap/MarkedVector.h +++ b/Userland/Libraries/LibJS/Heap/MarkedVector.h @@ -12,13 +12,13 @@ #include #include #include -#include +#include namespace JS { class MarkedVectorBase { public: - virtual void gather_roots(HashMap&) const = 0; + virtual void gather_roots(HashMap&) const = 0; protected: explicit MarkedVectorBase(Heap&); @@ -65,14 +65,14 @@ public: return *this; } - virtual void gather_roots(HashMap& roots) const override + virtual void gather_roots(HashMap& roots) const override { for (auto& value : *this) { if constexpr (IsSame) { if (value.is_cell()) - roots.set(&const_cast(value).as_cell(), HeapRootType::MarkedVector); + roots.set(&const_cast(value).as_cell(), HeapRoot { .type = HeapRoot::Type::MarkedVector }); } else { - roots.set(value, HeapRootType::MarkedVector); + roots.set(value, HeapRoot { .type = HeapRoot::Type::MarkedVector }); } } } diff --git a/Userland/Libraries/LibJS/Runtime/VM.cpp b/Userland/Libraries/LibJS/Runtime/VM.cpp index e8528ef8b4..c7abcd3f24 100644 --- a/Userland/Libraries/LibJS/Runtime/VM.cpp +++ b/Userland/Libraries/LibJS/Runtime/VM.cpp @@ -192,29 +192,29 @@ Bytecode::Interpreter& VM::bytecode_interpreter() return *m_bytecode_interpreter; } -void VM::gather_roots(HashMap& roots) +void VM::gather_roots(HashMap& roots) { - roots.set(m_empty_string, HeapRootType::VM); + roots.set(m_empty_string, HeapRoot { .type = HeapRoot::Type::VM }); for (auto string : m_single_ascii_character_strings) - roots.set(string, HeapRootType::VM); + roots.set(string, HeapRoot { .type = HeapRoot::Type::VM }); auto gather_roots_from_execution_context_stack = [&roots](Vector const& stack) { for (auto& execution_context : stack) { if (execution_context->this_value.is_cell()) - roots.set(&execution_context->this_value.as_cell(), HeapRootType::VM); + roots.set(&execution_context->this_value.as_cell(), { .type = HeapRoot::Type::VM }); for (auto& argument : execution_context->arguments) { if (argument.is_cell()) - roots.set(&argument.as_cell(), HeapRootType::VM); + roots.set(&argument.as_cell(), HeapRoot { .type = HeapRoot::Type::VM }); } - roots.set(execution_context->lexical_environment, HeapRootType::VM); - roots.set(execution_context->variable_environment, HeapRootType::VM); - roots.set(execution_context->private_environment, HeapRootType::VM); + roots.set(execution_context->lexical_environment, HeapRoot { .type = HeapRoot::Type::VM }); + roots.set(execution_context->variable_environment, HeapRoot { .type = HeapRoot::Type::VM }); + roots.set(execution_context->private_environment, HeapRoot { .type = HeapRoot::Type::VM }); if (auto context_owner = execution_context->context_owner) - roots.set(context_owner, HeapRootType::VM); + roots.set(context_owner, HeapRoot { .type = HeapRoot::Type::VM }); execution_context->script_or_module.visit( [](Empty) {}, [&](auto& script_or_module) { - roots.set(script_or_module.ptr(), HeapRootType::VM); + roots.set(script_or_module.ptr(), HeapRoot { .type = HeapRoot::Type::VM }); }); } }; @@ -224,15 +224,15 @@ void VM::gather_roots(HashMap& roots) gather_roots_from_execution_context_stack(saved_stack); #define __JS_ENUMERATE(SymbolName, snake_name) \ - roots.set(m_well_known_symbols.snake_name, HeapRootType::VM); + roots.set(m_well_known_symbols.snake_name, HeapRoot { .type = HeapRoot::Type::VM }); JS_ENUMERATE_WELL_KNOWN_SYMBOLS #undef __JS_ENUMERATE for (auto& symbol : m_global_symbol_registry) - roots.set(symbol.value, HeapRootType::VM); + roots.set(symbol.value, HeapRoot { .type = HeapRoot::Type::VM }); for (auto finalization_registry : m_finalization_registry_cleanup_jobs) - roots.set(finalization_registry, HeapRootType::VM); + roots.set(finalization_registry, HeapRoot { .type = HeapRoot::Type::VM }); } ThrowCompletionOr VM::named_evaluation_if_anonymous_function(ASTNode const& expression, DeprecatedFlyString const& name) diff --git a/Userland/Libraries/LibJS/Runtime/VM.h b/Userland/Libraries/LibJS/Runtime/VM.h index f515d7e7d1..36530a2704 100644 --- a/Userland/Libraries/LibJS/Runtime/VM.h +++ b/Userland/Libraries/LibJS/Runtime/VM.h @@ -47,7 +47,7 @@ public: void dump_backtrace() const; - void gather_roots(HashMap&); + void gather_roots(HashMap&); #define __JS_ENUMERATE(SymbolName, snake_name) \ NonnullGCPtr well_known_symbol_##snake_name() const \