[Lldb-commits] [lldb] [lldb][DataFormatter] Clean up LibcxxStdMapSyntheticFrontEnd::GetKeyValuePair (PR #97551)
Michael Buch via lldb-commits
lldb-commits at lists.llvm.org
Wed Jul 3 03:14:56 PDT 2024
https://github.com/Michael137 created https://github.com/llvm/llvm-project/pull/97551
This patch cleans up the core of the `std::map` libc++ formatter.
Depends on https://github.com/llvm/llvm-project/pull/97544 and
https://github.com/llvm/llvm-project/pull/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.
>From 9dabd3a399f37789b6a9bc7578b76e738c344f1d Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Wed, 3 Jul 2024 10:55:40 +0200
Subject: [PATCH 1/2] [lldb][DataFormatter][NFC] Factor out MapIterator logic
into separate 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
---
.../Plugins/Language/CPlusPlus/LibCxxMap.cpp | 115 +++++++++++-------
1 file changed, 72 insertions(+), 43 deletions(-)
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;
}
>From 78fa7f2b0382a187311954bdc40bb371841855e0 Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Wed, 3 Jul 2024 11:43:47 +0200
Subject: [PATCH 2/2] [lldb][DataFormatter] Clean up
LibcxxStdMapSyntheticFrontEnd::GetKeyValuePair
This patch cleans up the core of the `std::map` libc++ formatter.
Depends on https://github.com/llvm/llvm-project/pull/97544 and
https://github.com/llvm/llvm-project/pull/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.
---
.../Plugins/Language/CPlusPlus/LibCxxMap.cpp | 133 ++++++++----------
1 file changed, 55 insertions(+), 78 deletions(-)
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
index 370dfa35e7703..e12704ce28443 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
@@ -18,6 +18,8 @@
#include "lldb/Utility/Status.h"
#include "lldb/Utility/Stream.h"
#include "lldb/lldb-forward.h"
+#include <cstdint>
+#include <optional>
using namespace lldb;
using namespace lldb_private;
@@ -183,7 +185,7 @@ class LibcxxStdMapSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
private:
bool GetDataType();
- void GetValueOffset(const lldb::ValueObjectSP &node);
+ std::optional<uint32_t> GetValueOffset();
/// Returns the ValueObject for the __tree_node type that
/// holds the key/value pair of the node at index \ref idx.
@@ -204,7 +206,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;
+ uint32_t m_value_type_offset = UINT32_MAX;
size_t m_count = UINT32_MAX;
std::map<size_t, MapIterator> m_iterators;
};
@@ -274,46 +276,43 @@ bool lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::GetDataType() {
}
}
-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 089a7cc5dea)
- m_skip_size = bit_offset / 8u;
- } else {
- auto ast_ctx = node_type.GetTypeSystem().dyn_cast_or_null<TypeSystemClang>();
- 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;
- }
+std::optional<uint32_t>
+lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::GetValueOffset() {
+ if (!m_tree)
+ return std::nullopt;
+
+ auto ast_ctx = m_tree->GetCompilerType()
+ .GetTypeSystem()
+ .dyn_cast_or_null<TypeSystemClang>();
+ if (!ast_ctx)
+ return std::nullopt;
+
+ 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())
+ return std::nullopt;
+
+ return child_byte_offset;
}
ValueObjectSP
@@ -321,19 +320,18 @@ 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;
@@ -341,42 +339,21 @@ lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::GetKeyValuePair(
if (!GetDataType())
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);
- }
-
- if (!iterated_sp)
+ if (m_value_type_offset == UINT32_MAX) {
+ if (auto offset = GetValueOffset())
+ m_value_type_offset = *offset;
+ else
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;
+ assert(m_value_type_offset != UINT32_MAX);
- iterated_sp = iterated_sp->GetSyntheticChildAtOffset(m_skip_size,
- m_element_type, true);
- if (!iterated_sp)
- return nullptr;
- }
+ iterated_sp = iterated_sp->GetSyntheticChildAtOffset(m_value_type_offset,
+ m_element_type, true);
+ if (!iterated_sp)
+ return nullptr;
m_iterators[idx] = iterator;
- assert(iterated_sp != nullptr &&
- "Cached MapIterator for invalid ValueObject");
return iterated_sp;
}
More information about the lldb-commits
mailing list