From a54e283901736012c6d8b0b91892ac5b334b9a16 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sat, 30 Sep 2023 09:48:01 +0200 Subject: [PATCH] LibJS: Fix two bugs in the GC fast rejection of possible pointers - Convert to FlatPtr instead of doing pointer arithmetic on a too-large pointer type in find_min_and_max_block_addresses(). This makes the range more accurate. - Untag possible cell pointers in add_possible_value() before doing the rejection. Otherwise we end up rejecting most pointers since the tags sit in the highest bits! This fixes a crash when running the Speedometer benchmark. --- Userland/Libraries/LibJS/Heap/Heap.cpp | 31 ++++++++++---------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/Userland/Libraries/LibJS/Heap/Heap.cpp b/Userland/Libraries/LibJS/Heap/Heap.cpp index 677a02b8d9..353f6766d5 100644 --- a/Userland/Libraries/LibJS/Heap/Heap.cpp +++ b/Userland/Libraries/LibJS/Heap/Heap.cpp @@ -97,19 +97,22 @@ Cell* Heap::allocate_cell(size_t size) static void add_possible_value(HashMap& possible_pointers, FlatPtr data, HeapRoot origin, FlatPtr min_block_address, FlatPtr max_block_address) { - if (data < min_block_address || data > max_block_address) - return; - if constexpr (sizeof(FlatPtr*) == sizeof(Value)) { // Because Value stores pointers in non-canonical form we have to check if the top bytes // match any pointer-backed tag, in that case we have to extract the pointer to its // canonical form and add that as a possible pointer. + FlatPtr possible_pointer; if ((data & SHIFTED_IS_CELL_PATTERN) == SHIFTED_IS_CELL_PATTERN) - possible_pointers.set(Value::extract_pointer_bits(data), move(origin)); + possible_pointer = Value::extract_pointer_bits(data); else - possible_pointers.set(data, move(origin)); + possible_pointer = data; + if (possible_pointer < min_block_address || possible_pointer > max_block_address) + return; + possible_pointers.set(possible_pointer, move(origin)); } else { static_assert((sizeof(Value) % sizeof(FlatPtr*)) == 0); + if (data < min_block_address || data > max_block_address) + return; // In the 32-bit case we will look at the top and bottom part of Value separately we just // add both the upper and lower bytes as possible pointers. possible_pointers.set(data, move(origin)); @@ -118,23 +121,13 @@ static void add_possible_value(HashMap& possible_pointers, Fl void Heap::find_min_and_max_block_addresses(FlatPtr& min_address, FlatPtr& max_address) { - Optional min_block_address, max_block_address; + min_address = explode_byte(0xff); + max_address = 0; for_each_block([&](auto& block) { - if (!min_block_address.has_value()) - min_block_address = reinterpret_cast(&block); - else if (reinterpret_cast(&block) < min_block_address.value()) - min_block_address = reinterpret_cast(&block); - if (!max_block_address.has_value()) - max_block_address = reinterpret_cast(&block + HeapBlockBase::block_size); - else if (reinterpret_cast(&block + HeapBlockBase::block_size) > max_block_address.value()) - max_block_address = reinterpret_cast(&block + HeapBlockBase::block_size); + min_address = min(min_address, reinterpret_cast(&block)); + max_address = max(max_address, reinterpret_cast(&block) + HeapBlockBase::block_size); return IterationDecision::Continue; }); - - VERIFY(min_block_address.has_value()); - VERIFY(max_block_address.has_value()); - min_address = min_block_address.value(); - max_address = max_block_address.value(); } template