[lldb][DataFormatter][NFC] Move std::map iterator formatter into LibCxxMap.cpp #97687
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
…xxMap.cpp The two formatters follow very similar techniques to retrieve data out of the map. We're changing this for `std::map` in llvm#97579 and plan to change it in the same way for the iterator formatter. Having them in the same place will allow us to re-use some of the logic (and we won't have to repeat some of the clarification comments).
Michael137
requested review from
kastiglione,
DavidSpickett,
adrian-prantl and
labath
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThe two formatters follow very similar techniques to retrieve data out of the map. We're changing this for Full diff: https://github.com/llvm/llvm-project/pull/97687.diff 3 Files Affected:
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
index ad467c3966e60..05cfa0568c25d 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -202,194 +202,6 @@ bool lldb_private::formatters::LibcxxUniquePointerSummaryProvider(
return true;
}
-/*
- (lldb) fr var ibeg --raw --ptr-depth 1
- (std::__1::__map_iterator<std::__1::__tree_iterator<std::__1::pair<int,
- std::__1::basic_string<char, std::__1::char_traits<char>,
- std::__1::allocator<char> > >, std::__1::__tree_node<std::__1::pair<int,
- std::__1::basic_string<char, std::__1::char_traits<char>,
- std::__1::allocator<char> > >, void *> *, long> >) ibeg = {
- __i_ = {
- __ptr_ = 0x0000000100103870 {
- std::__1::__tree_node_base<void *> = {
- std::__1::__tree_end_node<std::__1::__tree_node_base<void *> *> = {
- __left_ = 0x0000000000000000
- }
- __right_ = 0x0000000000000000
- __parent_ = 0x00000001001038b0
- __is_black_ = true
- }
- __value_ = {
- first = 0
- second = { std::string }
- */
-
-lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::
- LibCxxMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
- : SyntheticChildrenFrontEnd(*valobj_sp), m_pair_ptr(), m_pair_sp() {
- if (valobj_sp)
- Update();
-}
-
-lldb::ChildCacheState
-lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::Update() {
- m_pair_sp.reset();
- m_pair_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;
-
- // 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_pair_ptr = valobj_sp
- ->GetValueForExpressionPath(
- ".__i_.__ptr_->__value_", nullptr, nullptr,
- ValueObject::GetValueForExpressionPathOptions()
- .DontCheckDotVsArrowSyntax()
- .SetSyntheticChildrenTraversal(
- ValueObject::GetValueForExpressionPathOptions::
- SyntheticChildrenTraversal::None),
- nullptr)
- .get();
-
- if (!m_pair_ptr) {
- m_pair_ptr = valobj_sp
- ->GetValueForExpressionPath(
- ".__i_.__ptr_", nullptr, nullptr,
- ValueObject::GetValueForExpressionPathOptions()
- .DontCheckDotVsArrowSyntax()
- .SetSyntheticChildrenTraversal(
- ValueObject::GetValueForExpressionPathOptions::
- SyntheticChildrenTraversal::None),
- nullptr)
- .get();
- if (m_pair_ptr) {
- auto __i_(valobj_sp->GetChildMemberWithName("__i_"));
- if (!__i_) {
- m_pair_ptr = nullptr;
- return lldb::ChildCacheState::eRefetch;
- }
- CompilerType pair_type(
- __i_->GetCompilerType().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_pair_ptr = nullptr;
- return lldb::ChildCacheState::eRefetch;
- }
-
- auto addr(m_pair_ptr->GetValueAsUnsigned(LLDB_INVALID_ADDRESS));
- m_pair_ptr = nullptr;
- if (addr && addr != LLDB_INVALID_ADDRESS) {
- 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::__tree_iterator::__ptr_ and read it in
- // from process memory.
- //
- // 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
- // +-----------------------------+
- //
- 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", 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(4);
- }
- }
- }
-
- return lldb::ChildCacheState::eRefetch;
-}
-
-llvm::Expected<uint32_t> lldb_private::formatters::
- LibCxxMapIteratorSyntheticFrontEnd::CalculateNumChildren() {
- return 2;
-}
-
-lldb::ValueObjectSP
-lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::GetChildAtIndex(
- uint32_t idx) {
- if (m_pair_ptr)
- return m_pair_ptr->GetChildAtIndex(idx);
- if (m_pair_sp)
- return m_pair_sp->GetChildAtIndex(idx);
- return lldb::ValueObjectSP();
-}
-
-bool lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::
- MightHaveChildren() {
- return true;
-}
-
-size_t lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::
- GetIndexOfChildWithName(ConstString name) {
- if (name == "first")
- return 0;
- if (name == "second")
- return 1;
- return UINT32_MAX;
-}
-
-lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::
- ~LibCxxMapIteratorSyntheticFrontEnd() {
- // this will be deleted when its parent dies (since it's a child object)
- // delete m_pair_ptr;
-}
-
-SyntheticChildrenFrontEnd *
-lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEndCreator(
- CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
- return (valobj_sp ? new LibCxxMapIteratorSyntheticFrontEnd(valobj_sp)
- : nullptr);
-}
-
lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
LibCxxUnorderedMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
: SyntheticChildrenFrontEnd(*valobj_sp) {
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
index 7fe15d1bf3f7a..21dba015d1ba1 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
@@ -87,31 +87,6 @@ bool LibcxxContainerSummaryProvider(ValueObject &valobj, Stream &stream,
bool LibcxxSpanSummaryProvider(ValueObject &valobj, Stream &stream,
const TypeSummaryOptions &options);
-class LibCxxMapIteratorSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
-public:
- LibCxxMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp);
-
- 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;
-
- ~LibCxxMapIteratorSyntheticFrontEnd() override;
-
-private:
- ValueObject *m_pair_ptr;
- lldb::ValueObjectSP m_pair_sp;
-};
-
-SyntheticChildrenFrontEnd *
-LibCxxMapIteratorSyntheticFrontEndCreator(CXXSyntheticChildren *,
- lldb::ValueObjectSP);
-
/// Formats libcxx's std::unordered_map iterators
///
/// In raw form a std::unordered_map::iterator is represented as follows:
@@ -247,6 +222,10 @@ SyntheticChildrenFrontEnd *
LibcxxStdMapSyntheticFrontEndCreator(CXXSyntheticChildren *,
lldb::ValueObjectSP);
+SyntheticChildrenFrontEnd *
+LibCxxMapIteratorSyntheticFrontEndCreator(CXXSyntheticChildren *,
+ lldb::ValueObjectSP);
+
SyntheticChildrenFrontEnd *
LibcxxStdUnorderedMapSyntheticFrontEndCreator(CXXSyntheticChildren *,
lldb::ValueObjectSP);
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
index 6c2bc1a34137a..c2bb3555908be 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
@@ -208,6 +208,27 @@ class LibcxxStdMapSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
size_t m_count = UINT32_MAX;
std::map<size_t, MapIterator> m_iterators;
};
+
+class LibCxxMapIteratorSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
+public:
+ LibCxxMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp);
+
+ 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;
+
+ ~LibCxxMapIteratorSyntheticFrontEnd() override;
+
+private:
+ ValueObject *m_pair_ptr;
+ lldb::ValueObjectSP m_pair_sp;
+};
} // namespace formatters
} // namespace lldb_private
@@ -456,3 +477,169 @@ lldb_private::formatters::LibcxxStdMapSyntheticFrontEndCreator(
CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
return (valobj_sp ? new LibcxxStdMapSyntheticFrontEnd(valobj_sp) : nullptr);
}
+
+lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::
+ LibCxxMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
+ : SyntheticChildrenFrontEnd(*valobj_sp), m_pair_ptr(), m_pair_sp() {
+ if (valobj_sp)
+ Update();
+}
+
+lldb::ChildCacheState
+lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::Update() {
+ m_pair_sp.reset();
+ m_pair_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;
+
+ // 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_pair_ptr = valobj_sp
+ ->GetValueForExpressionPath(
+ ".__i_.__ptr_->__value_", nullptr, nullptr,
+ ValueObject::GetValueForExpressionPathOptions()
+ .DontCheckDotVsArrowSyntax()
+ .SetSyntheticChildrenTraversal(
+ ValueObject::GetValueForExpressionPathOptions::
+ SyntheticChildrenTraversal::None),
+ nullptr)
+ .get();
+
+ if (!m_pair_ptr) {
+ m_pair_ptr = valobj_sp
+ ->GetValueForExpressionPath(
+ ".__i_.__ptr_", nullptr, nullptr,
+ ValueObject::GetValueForExpressionPathOptions()
+ .DontCheckDotVsArrowSyntax()
+ .SetSyntheticChildrenTraversal(
+ ValueObject::GetValueForExpressionPathOptions::
+ SyntheticChildrenTraversal::None),
+ nullptr)
+ .get();
+ if (m_pair_ptr) {
+ auto __i_(valobj_sp->GetChildMemberWithName("__i_"));
+ if (!__i_) {
+ m_pair_ptr = nullptr;
+ return lldb::ChildCacheState::eRefetch;
+ }
+ CompilerType pair_type(
+ __i_->GetCompilerType().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_pair_ptr = nullptr;
+ return lldb::ChildCacheState::eRefetch;
+ }
+
+ auto addr(m_pair_ptr->GetValueAsUnsigned(LLDB_INVALID_ADDRESS));
+ m_pair_ptr = nullptr;
+ if (addr && addr != LLDB_INVALID_ADDRESS) {
+ 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::__tree_iterator::__ptr_ and read it in
+ // from process memory.
+ //
+ // 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
+ // +-----------------------------+
+ //
+ 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", 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(4);
+ }
+ }
+ }
+
+ return lldb::ChildCacheState::eRefetch;
+}
+
+llvm::Expected<uint32_t> lldb_private::formatters::
+ LibCxxMapIteratorSyntheticFrontEnd::CalculateNumChildren() {
+ return 2;
+}
+
+lldb::ValueObjectSP
+lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::GetChildAtIndex(
+ uint32_t idx) {
+ if (m_pair_ptr)
+ return m_pair_ptr->GetChildAtIndex(idx);
+ if (m_pair_sp)
+ return m_pair_sp->GetChildAtIndex(idx);
+ return lldb::ValueObjectSP();
+}
+
+bool lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::
+ MightHaveChildren() {
+ return true;
+}
+
+size_t lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::
+ GetIndexOfChildWithName(ConstString name) {
+ if (name == "first")
+ return 0;
+ if (name == "second")
+ return 1;
+ return UINT32_MAX;
+}
+
+lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::
+ ~LibCxxMapIteratorSyntheticFrontEnd() {
+ // this will be deleted when its parent dies (since it's a child object)
+ // delete m_pair_ptr;
+}
+
+SyntheticChildrenFrontEnd *
+lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEndCreator(
+ CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
+ return (valobj_sp ? new LibCxxMapIteratorSyntheticFrontEnd(valobj_sp)
+ : nullptr);
+}
|
kbluck
pushed a commit
to kbluck/llvm-project
that referenced
this pull request
Jul 6, 2024
…xxMap.cpp (llvm#97687) The two formatters follow very similar techniques to retrieve data out of the map. We're changing this for `std::map` in llvm#97579 and plan to change it in the same way for the iterator formatter. Having them in the same place will allow us to re-use some of the logic (and we won't have to repeat some of the clarification comments).
Michael137
added a commit
that referenced
this pull request
Jul 8, 2024
…o LibCxxUnorderedMap.cpp (#97752) Similar to how we moved the `std::map::iterator` formatter in #97687, do the same for `std::unordered_map::iterator`. Again the `unordered_map` and `unordered_map::iterator` formatters try to do very similar things: retrieve data out of the map. The iterator formatter does this in a fragile way (similar to how `std::map` does it, see #97579). Thus we will be refactoring the `std::unordered_map::iterator` in upcoming patches. Having it in `LibCxxUnorderedMap` will allow us to re-use some of the logic (and we won't have to repeat some of the clarification comments).
Michael137
added a commit
that referenced
this pull request
Jul 8, 2024
…97713) Depends on #97687 Similar to #97579, this patch simplifies the way in which we retrieve the key/value pair of a `std::map` (in this case of the `std::map::iterator`). We do this for the same reason: not only was the old logic hard to follow, and encoded the libc++ layout in non-obvious ways, it was also fragile to alignment miscalculations (#97443); this would break once the new layout of std::map landed as part of #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. We can eventually re-use the core-part of the `std::map` and `std::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.
The two formatters follow very similar techniques to retrieve data out of the map. We're changing this for
std::mapin #97579 and plan to change it in the same way for the iterator formatter. Having them in the same place will allow us to re-use some of the logic (and we won't have to repeat some of the clarification comments).