[lldb][DataFormatter][NFC] Factor out MapIterator logic into separate helper #97544
Conversation
This file contains 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
… helper This patch factors all the logic for advancing the `MapIterator` out of `GetChildAtIndex`. This, in my opinion, helps readability, and will be useful for upcoming cleanups in this area. While here, some drive-by changes: * added a couple of clarification comments * fixed a variable name typo * turned the `return lldb::ValueObjectSP()` into `return nullptr` * added an assertion to make sure we keep the iterator cache in a valid state
Michael137
requested review from
kastiglione,
adrian-prantl,
labath and
dwblaikie
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThis patch factors all the logic for advancing the While here, some drive-by changes:
Full diff: https://github.com/llvm/llvm-project/pull/97544.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
index 44fe294ced722..370dfa35e7703 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
@@ -17,6 +17,7 @@
#include "lldb/Utility/Endian.h"
#include "lldb/Utility/Status.h"
#include "lldb/Utility/Stream.h"
+#include "lldb/lldb-forward.h"
using namespace lldb;
using namespace lldb_private;
@@ -184,6 +185,22 @@ class LibcxxStdMapSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
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.
+ ///
+ /// \param[in] idx The child index that we're looking to get
+ /// the key/value pair for.
+ ///
+ /// \param[in] max_depth The maximum search depth after which
+ /// we stop trying to find the key/value
+ /// pair for.
+ ///
+ /// \returns On success, returns the ValueObjectSP corresponding
+ /// to the __tree_node's __value_ member (which holds
+ /// the key/value pair the formatter wants to display).
+ /// On failure, will return nullptr.
+ ValueObjectSP GetKeyValuePair(size_t idx, size_t max_depth);
+
ValueObject *m_tree = nullptr;
ValueObject *m_root_node = nullptr;
CompilerType m_element_type;
@@ -299,75 +316,88 @@ void lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::GetValueOffset(
}
}
-lldb::ValueObjectSP
-lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::GetChildAtIndex(
- uint32_t idx) {
- static ConstString g_cc_("__cc_"), g_cc("__cc");
- static ConstString g_nc("__nc");
- uint32_t num_children = CalculateNumChildrenIgnoringErrors();
- if (idx >= num_children)
- return lldb::ValueObjectSP();
- if (m_tree == nullptr || m_root_node == nullptr)
- return lldb::ValueObjectSP();
-
- MapIterator iterator(m_root_node, num_children);
+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_advancde = idx;
+ size_t actual_advance = idx;
if (need_to_skip) {
+ // 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_advancde = 1;
+ actual_advance = 1;
}
}
- ValueObjectSP iterated_sp(iterator.advance(actual_advancde));
- if (!iterated_sp) {
+ ValueObjectSP iterated_sp(iterator.advance(actual_advance));
+ if (!iterated_sp)
// this tree is garbage - stop
- m_tree =
- nullptr; // this will stop all future searches until an Update() happens
- return iterated_sp;
- }
+ return nullptr;
- if (!GetDataType()) {
- m_tree = nullptr;
- return lldb::ValueObjectSP();
- }
+ if (!GetDataType())
+ return nullptr;
if (!need_to_skip) {
Status error;
iterated_sp = iterated_sp->Dereference(error);
- if (!iterated_sp || error.Fail()) {
- m_tree = nullptr;
- return lldb::ValueObjectSP();
- }
+ if (!iterated_sp || error.Fail())
+ return nullptr;
+
GetValueOffset(iterated_sp);
auto child_sp = iterated_sp->GetChildMemberWithName("__value_");
- if (child_sp)
+ if (child_sp) {
+ // Old layout (pre 089a7cc5dea)
iterated_sp = child_sp;
- else
+ } else {
iterated_sp = iterated_sp->GetSyntheticChildAtOffset(
m_skip_size, m_element_type, true);
- if (!iterated_sp) {
- m_tree = nullptr;
- return lldb::ValueObjectSP();
}
+
+ 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) {
- m_tree = nullptr;
- return lldb::ValueObjectSP();
- }
+
+ if (m_skip_size == UINT32_MAX)
+ return nullptr;
+
iterated_sp = iterated_sp->GetSyntheticChildAtOffset(m_skip_size,
m_element_type, true);
- if (!iterated_sp) {
- m_tree = nullptr;
- return lldb::ValueObjectSP();
- }
+ if (!iterated_sp)
+ return nullptr;
+ }
+
+ m_iterators[idx] = iterator;
+ assert(iterated_sp != nullptr &&
+ "Cached MapIterator for invalid ValueObject");
+
+ return iterated_sp;
+}
+
+lldb::ValueObjectSP
+lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::GetChildAtIndex(
+ uint32_t idx) {
+ static ConstString g_cc_("__cc_"), g_cc("__cc");
+ static ConstString g_nc("__nc");
+ uint32_t num_children = CalculateNumChildrenIgnoringErrors();
+ if (idx >= num_children)
+ return nullptr;
+
+ if (m_tree == nullptr || m_root_node == nullptr)
+ return nullptr;
+
+ ValueObjectSP key_val_sp = GetKeyValuePair(idx, /*max_depth=*/num_children);
+ if (!key_val_sp) {
+ // this will stop all future searches until an Update() happens
+ m_tree = nullptr;
+ return nullptr;
}
// at this point we have a valid
@@ -375,7 +405,7 @@ lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::GetChildAtIndex(
// all items named __value_
StreamString name;
name.Printf("[%" PRIu64 "]", (uint64_t)idx);
- auto potential_child_sp = iterated_sp->Clone(ConstString(name.GetString()));
+ auto potential_child_sp = key_val_sp->Clone(ConstString(name.GetString()));
if (potential_child_sp) {
switch (potential_child_sp->GetNumChildrenIgnoringErrors()) {
case 1: {
@@ -396,7 +426,6 @@ lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::GetChildAtIndex(
}
}
}
- m_iterators[idx] = iterator;
return potential_child_sp;
}
|
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Jul 3, 2024
…aluePair This patch cleans up the core of the `std::map` libc++ formatter. Depends on llvm#97544 and llvm#97549. Changes: * Renamed `m_skip_size` to `m_value_type_offset` to better describe what it's actually for. * Made updating `m_skip_size` (now `m_value_type_offset`) isolated to `GetKeyValuePair` (instead of doing so in the `GetValueOffset` helper). * We don't need special logic for the 0th index, so I merged the two `need_to_skip` branches.
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Jul 3, 2024
Depends on: * llvm#97544 * llvm#97549 * llvm#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 (llvm#97443); this would break after the new layout of std::map landed as part of inhttps://github.com/llvm/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).
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Jul 3, 2024
…aluePair This patch cleans up the core of the `std::map` libc++ formatter. Depends on llvm#97544 and llvm#97549. Changes: * Renamed `m_skip_size` to `m_value_type_offset` to better describe what it's actually for. * Made updating `m_skip_size` (now `m_value_type_offset`) isolated to `GetKeyValuePair` (instead of doing so in the `GetValueOffset` helper). * We don't need special logic for the 0th index, so I merged the two `need_to_skip` branches.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/1001 Here is the relevant piece of the build log for the reference: |
kbluck
pushed a commit
to kbluck/llvm-project
that referenced
this pull request
Jul 6, 2024
… helper (llvm#97544) This patch factors all the logic for advancing the `MapIterator` out of `GetChildAtIndex`. This, in my opinion, helps readability, and will be useful for upcoming cleanups in this area. While here, some drive-by changes: * added a couple of clarification comments * fixed a variable name typo * turned the `return lldb::ValueObjectSP()` into `return nullptr` * added an assertion to make sure we keep the iterator cache in a valid state
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Jul 8, 2024
Depends on: * llvm#97544 * llvm#97549 * llvm#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 (llvm#97443); this would break after the new layout of std::map landed as part of inhttps://github.com/llvm/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).
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Jul 8, 2024
Depends on: * llvm#97544 * llvm#97549 * llvm#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 (llvm#97443); this would break after the new layout of std::map landed as part of inhttps://github.com/llvm/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).
Michael137
added a commit
that referenced
this pull request
Jul 8, 2024
Depends on: * #97544 * #97549 * #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: 1. synthesize a structure that mimicked what `__iter_pointer` looked like in memory 2. call `GetChildCompilerTypeAtIndex` on it to find the byte offset at which the pair was located in the synthesized structure 3. finally, use that offset through a call to `GetSyntheticChildAtOffset` to retrieve the key/value 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 (#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. 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).
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.
This patch factors all the logic for advancing the
MapIteratorout ofGetChildAtIndex. This, in my opinion, helps readability, and will be useful for upcoming cleanups in this area.While here, some drive-by changes:
return lldb::ValueObjectSP()intoreturn nullptr