[lldb][DataFormatter] Move std::unordered_map::iterator formatter into LibCxxUnorderedMap.cpp #97752
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…o LibCxxUnorderedMap.cpp
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesSimilar to how we moved the Again the Full diff: https://github.com/llvm/llvm-project/pull/97752.diff 3 Files Affected:
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
index 05cfa0568c25d..feaa51a96843a 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -202,155 +202,6 @@ bool lldb_private::formatters::LibcxxUniquePointerSummaryProvider(
return true;
}
-lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
- LibCxxUnorderedMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
- : SyntheticChildrenFrontEnd(*valobj_sp) {
- if (valobj_sp)
- Update();
-}
-
-lldb::ChildCacheState lldb_private::formatters::
- LibCxxUnorderedMapIteratorSyntheticFrontEnd::Update() {
- m_pair_sp.reset();
- m_iter_ptr = nullptr;
-
- ValueObjectSP valobj_sp = m_backend.GetSP();
- if (!valobj_sp)
- return lldb::ChildCacheState::eRefetch;
-
- TargetSP target_sp(valobj_sp->GetTargetSP());
-
- if (!target_sp)
- return lldb::ChildCacheState::eRefetch;
-
- if (!valobj_sp)
- return lldb::ChildCacheState::eRefetch;
-
- auto exprPathOptions = ValueObject::GetValueForExpressionPathOptions()
- .DontCheckDotVsArrowSyntax()
- .SetSyntheticChildrenTraversal(
- ValueObject::GetValueForExpressionPathOptions::
- SyntheticChildrenTraversal::None);
-
- // This must be a ValueObject* because it is a child of the ValueObject we
- // are producing children for it if were a ValueObjectSP, we would end up
- // with a loop (iterator -> synthetic -> child -> parent == iterator) and
- // that would in turn leak memory by never allowing the ValueObjects to die
- // and free their memory.
- m_iter_ptr =
- valobj_sp
- ->GetValueForExpressionPath(".__i_.__node_", nullptr, nullptr,
- exprPathOptions, nullptr)
- .get();
-
- if (m_iter_ptr) {
- auto iter_child(valobj_sp->GetChildMemberWithName("__i_"));
- if (!iter_child) {
- m_iter_ptr = nullptr;
- return lldb::ChildCacheState::eRefetch;
- }
-
- CompilerType node_type(iter_child->GetCompilerType()
- .GetTypeTemplateArgument(0)
- .GetPointeeType());
-
- CompilerType pair_type(node_type.GetTypeTemplateArgument(0));
-
- std::string name;
- uint64_t bit_offset_ptr;
- uint32_t bitfield_bit_size_ptr;
- bool is_bitfield_ptr;
-
- pair_type = pair_type.GetFieldAtIndex(
- 0, name, &bit_offset_ptr, &bitfield_bit_size_ptr, &is_bitfield_ptr);
- if (!pair_type) {
- m_iter_ptr = nullptr;
- return lldb::ChildCacheState::eRefetch;
- }
-
- uint64_t addr = m_iter_ptr->GetValueAsUnsigned(LLDB_INVALID_ADDRESS);
- m_iter_ptr = nullptr;
-
- if (addr == 0 || addr == LLDB_INVALID_ADDRESS)
- return lldb::ChildCacheState::eRefetch;
-
- auto ts = pair_type.GetTypeSystem();
- auto ast_ctx = ts.dyn_cast_or_null<TypeSystemClang>();
- if (!ast_ctx)
- return lldb::ChildCacheState::eRefetch;
-
- // Mimick layout of std::__hash_iterator::__node_ and read it in
- // from process memory.
- //
- // The following shows the contiguous block of memory:
- //
- // +-----------------------------+ class __hash_node_base
- // __node_ | __next_pointer __next_; |
- // +-----------------------------+ class __hash_node
- // | size_t __hash_; |
- // | __node_value_type __value_; | <<< our key/value pair
- // +-----------------------------+
- //
- CompilerType tree_node_type = ast_ctx->CreateStructForIdentifier(
- llvm::StringRef(),
- {{"__next_",
- ast_ctx->GetBasicType(lldb::eBasicTypeVoid).GetPointerType()},
- {"__hash_", ast_ctx->GetBasicType(lldb::eBasicTypeUnsignedLongLong)},
- {"__value_", pair_type}});
- std::optional<uint64_t> size = tree_node_type.GetByteSize(nullptr);
- if (!size)
- return lldb::ChildCacheState::eRefetch;
- WritableDataBufferSP buffer_sp(new DataBufferHeap(*size, 0));
- ProcessSP process_sp(target_sp->GetProcessSP());
- Status error;
- process_sp->ReadMemory(addr, buffer_sp->GetBytes(),
- buffer_sp->GetByteSize(), error);
- if (error.Fail())
- return lldb::ChildCacheState::eRefetch;
- DataExtractor extractor(buffer_sp, process_sp->GetByteOrder(),
- process_sp->GetAddressByteSize());
- auto pair_sp = CreateValueObjectFromData(
- "pair", extractor, valobj_sp->GetExecutionContextRef(), tree_node_type);
- if (pair_sp)
- m_pair_sp = pair_sp->GetChildAtIndex(2);
- }
-
- return lldb::ChildCacheState::eRefetch;
-}
-
-llvm::Expected<uint32_t> lldb_private::formatters::
- LibCxxUnorderedMapIteratorSyntheticFrontEnd::CalculateNumChildren() {
- return 2;
-}
-
-lldb::ValueObjectSP lldb_private::formatters::
- LibCxxUnorderedMapIteratorSyntheticFrontEnd::GetChildAtIndex(uint32_t idx) {
- if (m_pair_sp)
- return m_pair_sp->GetChildAtIndex(idx);
- return lldb::ValueObjectSP();
-}
-
-bool lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
- MightHaveChildren() {
- return true;
-}
-
-size_t lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
- GetIndexOfChildWithName(ConstString name) {
- if (name == "first")
- return 0;
- if (name == "second")
- return 1;
- return UINT32_MAX;
-}
-
-SyntheticChildrenFrontEnd *
-lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEndCreator(
- CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
- return (valobj_sp ? new LibCxxUnorderedMapIteratorSyntheticFrontEnd(valobj_sp)
- : nullptr);
-}
-
/*
(lldb) fr var ibeg --raw --ptr-depth 1 -T
(std::__1::__wrap_iter<int *>) ibeg = {
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
index 21dba015d1ba1..5307b5251ca84 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
@@ -87,56 +87,6 @@ bool LibcxxContainerSummaryProvider(ValueObject &valobj, Stream &stream,
bool LibcxxSpanSummaryProvider(ValueObject &valobj, Stream &stream,
const TypeSummaryOptions &options);
-/// Formats libcxx's std::unordered_map iterators
-///
-/// In raw form a std::unordered_map::iterator is represented as follows:
-///
-/// (lldb) var it --raw --ptr-depth 1
-/// (std::__1::__hash_map_iterator<
-/// std::__1::__hash_iterator<
-/// std::__1::__hash_node<
-/// std::__1::__hash_value_type<
-/// std::__1::basic_string<char, std::__1::char_traits<char>,
-/// std::__1::allocator<char> >, std::__1::basic_string<char,
-/// std::__1::char_traits<char>, std::__1::allocator<char> > >,
-/// void *> *> >)
-/// it = {
-/// __i_ = {
-/// __node_ = 0x0000600001700040 {
-/// __next_ = 0x0000600001704000
-/// }
-/// }
-/// }
-class LibCxxUnorderedMapIteratorSyntheticFrontEnd
- : public SyntheticChildrenFrontEnd {
-public:
- LibCxxUnorderedMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp);
-
- ~LibCxxUnorderedMapIteratorSyntheticFrontEnd() override = default;
-
- llvm::Expected<uint32_t> CalculateNumChildren() override;
-
- lldb::ValueObjectSP GetChildAtIndex(uint32_t idx) override;
-
- lldb::ChildCacheState Update() override;
-
- bool MightHaveChildren() override;
-
- size_t GetIndexOfChildWithName(ConstString name) override;
-
-private:
- ValueObject *m_iter_ptr = nullptr; ///< Held, not owned. Child of iterator
- ///< ValueObject supplied at construction.
-
- lldb::ValueObjectSP m_pair_sp; ///< ValueObject for the key/value pair
- ///< that the iterator currently points
- ///< to.
-};
-
-SyntheticChildrenFrontEnd *
-LibCxxUnorderedMapIteratorSyntheticFrontEndCreator(CXXSyntheticChildren *,
- lldb::ValueObjectSP);
-
SyntheticChildrenFrontEnd *
LibCxxVectorIteratorSyntheticFrontEndCreator(CXXSyntheticChildren *,
lldb::ValueObjectSP);
@@ -230,6 +180,10 @@ SyntheticChildrenFrontEnd *
LibcxxStdUnorderedMapSyntheticFrontEndCreator(CXXSyntheticChildren *,
lldb::ValueObjectSP);
+SyntheticChildrenFrontEnd *
+LibCxxUnorderedMapIteratorSyntheticFrontEndCreator(CXXSyntheticChildren *,
+ lldb::ValueObjectSP);
+
SyntheticChildrenFrontEnd *
LibcxxInitializerListSyntheticFrontEndCreator(CXXSyntheticChildren *,
lldb::ValueObjectSP);
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
index af29fdb6d0010..f5be2f5a5c317 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
@@ -51,6 +51,53 @@ class LibcxxStdUnorderedMapSyntheticFrontEnd
ValueObject *m_next_element = nullptr;
std::vector<std::pair<ValueObject *, uint64_t>> m_elements_cache;
};
+
+/// Formats libcxx's std::unordered_map iterators
+///
+/// In raw form a std::unordered_map::iterator is represented as follows:
+///
+/// (lldb) var it --raw --ptr-depth 1
+/// (std::__1::__hash_map_iterator<
+/// std::__1::__hash_iterator<
+/// std::__1::__hash_node<
+/// std::__1::__hash_value_type<
+/// std::__1::basic_string<char, std::__1::char_traits<char>,
+/// std::__1::allocator<char> >, std::__1::basic_string<char,
+/// std::__1::char_traits<char>, std::__1::allocator<char> > >,
+/// void *> *> >)
+/// it = {
+/// __i_ = {
+/// __node_ = 0x0000600001700040 {
+/// __next_ = 0x0000600001704000
+/// }
+/// }
+/// }
+class LibCxxUnorderedMapIteratorSyntheticFrontEnd
+ : public SyntheticChildrenFrontEnd {
+public:
+ LibCxxUnorderedMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp);
+
+ ~LibCxxUnorderedMapIteratorSyntheticFrontEnd() override = default;
+
+ llvm::Expected<uint32_t> CalculateNumChildren() override;
+
+ lldb::ValueObjectSP GetChildAtIndex(uint32_t idx) override;
+
+ lldb::ChildCacheState Update() override;
+
+ bool MightHaveChildren() override;
+
+ size_t GetIndexOfChildWithName(ConstString name) override;
+
+private:
+ ValueObject *m_iter_ptr = nullptr; ///< Held, not owned. Child of iterator
+ ///< ValueObject supplied at construction.
+
+ lldb::ValueObjectSP m_pair_sp; ///< ValueObject for the key/value pair
+ ///< that the iterator currently points
+ ///< to.
+};
+
} // namespace formatters
} // namespace lldb_private
@@ -246,3 +293,152 @@ lldb_private::formatters::LibcxxStdUnorderedMapSyntheticFrontEndCreator(
return (valobj_sp ? new LibcxxStdUnorderedMapSyntheticFrontEnd(valobj_sp)
: nullptr);
}
+
+lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
+ LibCxxUnorderedMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
+ : SyntheticChildrenFrontEnd(*valobj_sp) {
+ if (valobj_sp)
+ Update();
+}
+
+lldb::ChildCacheState lldb_private::formatters::
+ LibCxxUnorderedMapIteratorSyntheticFrontEnd::Update() {
+ m_pair_sp.reset();
+ m_iter_ptr = nullptr;
+
+ ValueObjectSP valobj_sp = m_backend.GetSP();
+ if (!valobj_sp)
+ return lldb::ChildCacheState::eRefetch;
+
+ TargetSP target_sp(valobj_sp->GetTargetSP());
+
+ if (!target_sp)
+ return lldb::ChildCacheState::eRefetch;
+
+ if (!valobj_sp)
+ return lldb::ChildCacheState::eRefetch;
+
+ auto exprPathOptions = ValueObject::GetValueForExpressionPathOptions()
+ .DontCheckDotVsArrowSyntax()
+ .SetSyntheticChildrenTraversal(
+ ValueObject::GetValueForExpressionPathOptions::
+ SyntheticChildrenTraversal::None);
+
+ // This must be a ValueObject* because it is a child of the ValueObject we
+ // are producing children for it if were a ValueObjectSP, we would end up
+ // with a loop (iterator -> synthetic -> child -> parent == iterator) and
+ // that would in turn leak memory by never allowing the ValueObjects to die
+ // and free their memory.
+ m_iter_ptr =
+ valobj_sp
+ ->GetValueForExpressionPath(".__i_.__node_", nullptr, nullptr,
+ exprPathOptions, nullptr)
+ .get();
+
+ if (m_iter_ptr) {
+ auto iter_child(valobj_sp->GetChildMemberWithName("__i_"));
+ if (!iter_child) {
+ m_iter_ptr = nullptr;
+ return lldb::ChildCacheState::eRefetch;
+ }
+
+ CompilerType node_type(iter_child->GetCompilerType()
+ .GetTypeTemplateArgument(0)
+ .GetPointeeType());
+
+ CompilerType pair_type(node_type.GetTypeTemplateArgument(0));
+
+ std::string name;
+ uint64_t bit_offset_ptr;
+ uint32_t bitfield_bit_size_ptr;
+ bool is_bitfield_ptr;
+
+ pair_type = pair_type.GetFieldAtIndex(
+ 0, name, &bit_offset_ptr, &bitfield_bit_size_ptr, &is_bitfield_ptr);
+ if (!pair_type) {
+ m_iter_ptr = nullptr;
+ return lldb::ChildCacheState::eRefetch;
+ }
+
+ uint64_t addr = m_iter_ptr->GetValueAsUnsigned(LLDB_INVALID_ADDRESS);
+ m_iter_ptr = nullptr;
+
+ if (addr == 0 || addr == LLDB_INVALID_ADDRESS)
+ return lldb::ChildCacheState::eRefetch;
+
+ auto ts = pair_type.GetTypeSystem();
+ auto ast_ctx = ts.dyn_cast_or_null<TypeSystemClang>();
+ if (!ast_ctx)
+ return lldb::ChildCacheState::eRefetch;
+
+ // Mimick layout of std::__hash_iterator::__node_ and read it in
+ // from process memory.
+ //
+ // The following shows the contiguous block of memory:
+ //
+ // +-----------------------------+ class __hash_node_base
+ // __node_ | __next_pointer __next_; |
+ // +-----------------------------+ class __hash_node
+ // | size_t __hash_; |
+ // | __node_value_type __value_; | <<< our key/value pair
+ // +-----------------------------+
+ //
+ CompilerType tree_node_type = ast_ctx->CreateStructForIdentifier(
+ llvm::StringRef(),
+ {{"__next_",
+ ast_ctx->GetBasicType(lldb::eBasicTypeVoid).GetPointerType()},
+ {"__hash_", ast_ctx->GetBasicType(lldb::eBasicTypeUnsignedLongLong)},
+ {"__value_", pair_type}});
+ std::optional<uint64_t> size = tree_node_type.GetByteSize(nullptr);
+ if (!size)
+ return lldb::ChildCacheState::eRefetch;
+ WritableDataBufferSP buffer_sp(new DataBufferHeap(*size, 0));
+ ProcessSP process_sp(target_sp->GetProcessSP());
+ Status error;
+ process_sp->ReadMemory(addr, buffer_sp->GetBytes(),
+ buffer_sp->GetByteSize(), error);
+ if (error.Fail())
+ return lldb::ChildCacheState::eRefetch;
+ DataExtractor extractor(buffer_sp, process_sp->GetByteOrder(),
+ process_sp->GetAddressByteSize());
+ auto pair_sp = CreateValueObjectFromData(
+ "pair", extractor, valobj_sp->GetExecutionContextRef(), tree_node_type);
+ if (pair_sp)
+ m_pair_sp = pair_sp->GetChildAtIndex(2);
+ }
+
+ return lldb::ChildCacheState::eRefetch;
+}
+
+llvm::Expected<uint32_t> lldb_private::formatters::
+ LibCxxUnorderedMapIteratorSyntheticFrontEnd::CalculateNumChildren() {
+ return 2;
+}
+
+lldb::ValueObjectSP lldb_private::formatters::
+ LibCxxUnorderedMapIteratorSyntheticFrontEnd::GetChildAtIndex(uint32_t idx) {
+ if (m_pair_sp)
+ return m_pair_sp->GetChildAtIndex(idx);
+ return lldb::ValueObjectSP();
+}
+
+bool lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
+ MightHaveChildren() {
+ return true;
+}
+
+size_t lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
+ GetIndexOfChildWithName(ConstString name) {
+ if (name == "first")
+ return 0;
+ if (name == "second")
+ return 1;
+ return UINT32_MAX;
+}
+
+SyntheticChildrenFrontEnd *
+lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEndCreator(
+ CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
+ return (valobj_sp ? new LibCxxUnorderedMapIteratorSyntheticFrontEnd(valobj_sp)
+ : nullptr);
+}
|
labath
approved these changes
Jul 8, 2024
Michael137
added a commit
that referenced
this pull request
Jul 8, 2024
…#97754) Depends on #97752 This patch changes the way we retrieve the key/value pair in the `std::unordered_map::iterator` formatter (similar to how we are changing it for `std::map::iterator` in #97713, the motivations being the same). The old logic was not very easy to follow, and encoded the libc++ layout in non-obvious ways. But mainly it was also fragile to alignment miscalculations (#97443); this would break once the new layout of `std::unordered_map` landed as part of #93069. Instead, this patch simply casts the `__hash_iterator` to a `__node_pointer` (which is what libc++ does too) and uses a straightforward `GetChildMemberWithName("__value_")` to get to the key/value we care about. The `std::unordered_map` already does it this way, so we align the iterator counterpart to do the same. We can eventually re-use the core-part of the `std::unordered_map` and `std::unordered_map::iterator` formatters. But it will be an easier to change to review once both simplifications landed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Similar to how we moved the
std::map::iteratorformatter in #97687, do the same forstd::unordered_map::iterator.Again the
unordered_mapandunordered_map::iteratorformatters try to do very similar things: retrieve data out of the map. The iterator formatter does this in a fragile way (similar to howstd::mapdoes it, see #97579). Thus we will be refactoring thestd::unordered_map::iteratorin upcoming patches. Having it inLibCxxUnorderedMapwill allow us to re-use some of the logic (and we won't have to repeat some of the clarification comments).