From 223606fa569d4d9ced91461ec10e63ef414ad15e Mon Sep 17 00:00:00 2001 From: Michael Buch Date: Wed, 3 Jul 2024 11:43:47 +0200 Subject: [PATCH] [lldb][DataFormatter] Simplify std::map formatter Depends on: * https://github.com/llvm/llvm-project/pull/97544 * https://github.com/llvm/llvm-project/pull/97549 * https://github.com/llvm/llvm-project/pull/97551 This patch tries to simplify the way in which the `std::map` formatter goes from the root `__tree` pointer to a specific key/value pair. Previously we would synthesize a structure that mimicked what `__iter_pointer` looked like in memory, then called `GetChildCompilerTypeAtIndex` on it to find the byte offset into that structure at which the pair was located at, and finally use that offset through a call to `GetSyntheticChildAtOffset` to retrieve that pair. Not only was this logic hard to follow, and encoded the libc++ layout in non-obvious ways, it was also fragile to alignment miscalculations (https://github.com/llvm/llvm-project/pull/97443); this would break after the new layout of std::map landed as part of inhttps://github.com/llvm/llvm-project/issues/93069. Instead, this patch simply casts the `__iter_pointer` to the `__node_pointer` and uses a straightforward `GetChildMemberWithName("__value_")` to get to the key/value we care about. This allows us to get rid of some support infrastructure/class state. Ideally we would fix the underlying alignment issue, but this unblocks the libc++ refactor in the interim, while also benefitting the formatter in terms of readability (in my opinion). --- .../Plugins/Language/CPlusPlus/LibCxxMap.cpp | 172 +++++------------- 1 file changed, 48 insertions(+), 124 deletions(-) diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp index c2bb3555908be..093e182df1188 100644 --- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp +++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp @@ -18,11 +18,31 @@ #include "lldb/Utility/Status.h" #include "lldb/Utility/Stream.h" #include "lldb/lldb-forward.h" +#include +#include +#include using namespace lldb; using namespace lldb_private; using namespace lldb_private::formatters; +// The flattened layout of the std::__tree_iterator::__ptr_ looks +// as follows: +// +// The following shows the contiguous block of memory: +// +// +-----------------------------+ class __tree_end_node +// __ptr_ | pointer __left_; | +// +-----------------------------+ class __tree_node_base +// | pointer __right_; | +// | __parent_pointer __parent_; | +// | bool __is_black_; | +// +-----------------------------+ class __tree_node +// | __node_value_type __value_; | <<< our key/value pair +// +-----------------------------+ +// +// where __ptr_ has type __iter_pointer. + class MapEntry { public: MapEntry() = default; @@ -181,10 +201,6 @@ class LibcxxStdMapSyntheticFrontEnd : public SyntheticChildrenFrontEnd { size_t GetIndexOfChildWithName(ConstString name) override; private: - bool GetDataType(); - - void GetValueOffset(const lldb::ValueObjectSP &node); - /// Returns the ValueObject for the __tree_node type that /// holds the key/value pair of the node at index \ref idx. /// @@ -203,8 +219,7 @@ class LibcxxStdMapSyntheticFrontEnd : public SyntheticChildrenFrontEnd { ValueObject *m_tree = nullptr; ValueObject *m_root_node = nullptr; - CompilerType m_element_type; - uint32_t m_skip_size = UINT32_MAX; + CompilerType m_node_ptr_type; size_t m_count = UINT32_MAX; std::map m_iterators; }; @@ -234,7 +249,7 @@ class LibCxxMapIteratorSyntheticFrontEnd : public SyntheticChildrenFrontEnd { lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd:: LibcxxStdMapSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp) - : SyntheticChildrenFrontEnd(*valobj_sp), m_element_type(), m_iterators() { + : SyntheticChildrenFrontEnd(*valobj_sp) { if (valobj_sp) Update(); } @@ -260,146 +275,52 @@ llvm::Expected lldb_private::formatters:: return m_count; } -bool lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::GetDataType() { - if (m_element_type.IsValid()) - return true; - m_element_type.Clear(); - ValueObjectSP deref; - Status error; - deref = m_root_node->Dereference(error); - if (!deref || error.Fail()) - return false; - deref = deref->GetChildMemberWithName("__value_"); - if (deref) { - m_element_type = deref->GetCompilerType(); - return true; - } - deref = m_backend.GetChildAtNamePath({"__tree_", "__pair3_"}); - if (!deref) - return false; - m_element_type = deref->GetCompilerType() - .GetTypeTemplateArgument(1) - .GetTypeTemplateArgument(1); - if (m_element_type) { - std::string name; - uint64_t bit_offset_ptr; - uint32_t bitfield_bit_size_ptr; - bool is_bitfield_ptr; - m_element_type = m_element_type.GetFieldAtIndex( - 0, name, &bit_offset_ptr, &bitfield_bit_size_ptr, &is_bitfield_ptr); - m_element_type = m_element_type.GetTypedefedType(); - return m_element_type.IsValid(); - } else { - m_element_type = m_backend.GetCompilerType().GetTypeTemplateArgument(0); - return m_element_type.IsValid(); - } -} - -void lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::GetValueOffset( - const lldb::ValueObjectSP &node) { - if (m_skip_size != UINT32_MAX) - return; - if (!node) - return; - CompilerType node_type(node->GetCompilerType()); - uint64_t bit_offset; - if (node_type.GetIndexOfFieldWithName("__value_", nullptr, &bit_offset) != - UINT32_MAX) { - // Old layout (pre d05b10ab4fc65) - m_skip_size = bit_offset / 8u; - } else { - auto ast_ctx = node_type.GetTypeSystem().dyn_cast_or_null(); - if (!ast_ctx) - return; - CompilerType tree_node_type = ast_ctx->CreateStructForIdentifier( - llvm::StringRef(), - {{"ptr0", ast_ctx->GetBasicType(lldb::eBasicTypeVoid).GetPointerType()}, - {"ptr1", ast_ctx->GetBasicType(lldb::eBasicTypeVoid).GetPointerType()}, - {"ptr2", ast_ctx->GetBasicType(lldb::eBasicTypeVoid).GetPointerType()}, - {"cw", ast_ctx->GetBasicType(lldb::eBasicTypeBool)}, - {"payload", (m_element_type.GetCompleteType(), m_element_type)}}); - std::string child_name; - uint32_t child_byte_size; - int32_t child_byte_offset = 0; - uint32_t child_bitfield_bit_size; - uint32_t child_bitfield_bit_offset; - bool child_is_base_class; - bool child_is_deref_of_parent; - uint64_t language_flags; - auto child_type = - llvm::expectedToStdOptional(tree_node_type.GetChildCompilerTypeAtIndex( - nullptr, 4, true, true, true, child_name, child_byte_size, - child_byte_offset, child_bitfield_bit_size, - child_bitfield_bit_offset, child_is_base_class, - child_is_deref_of_parent, nullptr, language_flags)); - if (child_type && child_type->IsValid()) - m_skip_size = (uint32_t)child_byte_offset; - } -} - ValueObjectSP lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::GetKeyValuePair( size_t idx, size_t max_depth) { MapIterator iterator(m_root_node, max_depth); - const bool need_to_skip = (idx > 0); - size_t actual_advance = idx; - if (need_to_skip) { + size_t advance_by = idx; + if (idx > 0) { // If we have already created the iterator for the previous // index, we can start from there and advance by 1. auto cached_iterator = m_iterators.find(idx - 1); if (cached_iterator != m_iterators.end()) { iterator = cached_iterator->second; - actual_advance = 1; + advance_by = 1; } } - ValueObjectSP iterated_sp(iterator.advance(actual_advance)); + ValueObjectSP iterated_sp(iterator.advance(advance_by)); if (!iterated_sp) // this tree is garbage - stop return nullptr; - if (!GetDataType()) + if (!m_node_ptr_type.IsValid()) return nullptr; - if (!need_to_skip) { - Status error; - iterated_sp = iterated_sp->Dereference(error); - if (!iterated_sp || error.Fail()) - return nullptr; - - GetValueOffset(iterated_sp); - auto child_sp = iterated_sp->GetChildMemberWithName("__value_"); - if (child_sp) { - // Old layout (pre 089a7cc5dea) - iterated_sp = child_sp; - } else { - iterated_sp = iterated_sp->GetSyntheticChildAtOffset( - m_skip_size, m_element_type, true); - } + // iterated_sp is a __iter_pointer at this point. + // We can cast it to a __node_pointer (which is what libc++ does). + auto value_type_sp = iterated_sp->Cast(m_node_ptr_type); + if (!value_type_sp) + return nullptr; - if (!iterated_sp) - return nullptr; - } else { - // because of the way our debug info is made, we need to read item 0 - // first so that we can cache information used to generate other elements - if (m_skip_size == UINT32_MAX) - GetChildAtIndex(0); - - if (m_skip_size == UINT32_MAX) - return nullptr; - - iterated_sp = iterated_sp->GetSyntheticChildAtOffset(m_skip_size, - m_element_type, true); - if (!iterated_sp) - return nullptr; - } + // After dereferencing the __node_pointer, we will have a handle to + // a std::__1::__tree_node, which has the __value_ member + // we are looking for. + Status s; + value_type_sp = value_type_sp->Dereference(s); + if (!value_type_sp || s.Fail()) + return nullptr; + + // Finally, get the key/value pair. + value_type_sp = value_type_sp->GetChildMemberWithName("__value_"); + if (!value_type_sp) + return nullptr; m_iterators[idx] = iterator; - assert(iterated_sp != nullptr && - "Cached MapIterator for invalid ValueObject"); - return iterated_sp; + return value_type_sp; } lldb::ValueObjectSP @@ -459,6 +380,9 @@ lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::Update() { if (!m_tree) return lldb::ChildCacheState::eRefetch; m_root_node = m_tree->GetChildMemberWithName("__begin_node_").get(); + m_node_ptr_type = + m_tree->GetCompilerType().GetDirectNestedTypeWithName("__node_pointer"); + return lldb::ChildCacheState::eRefetch; }