Skip to content

Commit da827d0

Browse files
authored
[lldb][DataFormatter] Simplify std::unordered_map::iterator formatter (#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.
1 parent 83b01aa commit da827d0

File tree

3 files changed

+96
-124
lines changed

3 files changed

+96
-124
lines changed

lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp

Lines changed: 50 additions & 106 deletions
Original file line numberDiff line numberDiff line change
@@ -52,26 +52,6 @@ class LibcxxStdUnorderedMapSyntheticFrontEnd
5252
std::vector<std::pair<ValueObject *, uint64_t>> m_elements_cache;
5353
};
5454

55-
/// Formats libcxx's std::unordered_map iterators
56-
///
57-
/// In raw form a std::unordered_map::iterator is represented as follows:
58-
///
59-
/// (lldb) var it --raw --ptr-depth 1
60-
/// (std::__1::__hash_map_iterator<
61-
/// std::__1::__hash_iterator<
62-
/// std::__1::__hash_node<
63-
/// std::__1::__hash_value_type<
64-
/// std::__1::basic_string<char, std::__1::char_traits<char>,
65-
/// std::__1::allocator<char> >, std::__1::basic_string<char,
66-
/// std::__1::char_traits<char>, std::__1::allocator<char> > >,
67-
/// void *> *> >)
68-
/// it = {
69-
/// __i_ = {
70-
/// __node_ = 0x0000600001700040 {
71-
/// __next_ = 0x0000600001704000
72-
/// }
73-
/// }
74-
/// }
7555
class LibCxxUnorderedMapIteratorSyntheticFrontEnd
7656
: public SyntheticChildrenFrontEnd {
7757
public:
@@ -90,9 +70,6 @@ class LibCxxUnorderedMapIteratorSyntheticFrontEnd
9070
size_t GetIndexOfChildWithName(ConstString name) override;
9171

9272
private:
93-
ValueObject *m_iter_ptr = nullptr; ///< Held, not owned. Child of iterator
94-
///< ValueObject supplied at construction.
95-
9673
lldb::ValueObjectSP m_pair_sp; ///< ValueObject for the key/value pair
9774
///< that the iterator currently points
9875
///< to.
@@ -304,7 +281,6 @@ lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
304281
lldb::ChildCacheState lldb_private::formatters::
305282
LibCxxUnorderedMapIteratorSyntheticFrontEnd::Update() {
306283
m_pair_sp.reset();
307-
m_iter_ptr = nullptr;
308284

309285
ValueObjectSP valobj_sp = m_backend.GetSP();
310286
if (!valobj_sp)
@@ -315,98 +291,66 @@ lldb::ChildCacheState lldb_private::formatters::
315291
if (!target_sp)
316292
return lldb::ChildCacheState::eRefetch;
317293

318-
if (!valobj_sp)
294+
// Get the unordered_map::iterator
295+
// m_backend is an 'unordered_map::iterator', aka a
296+
// '__hash_map_iterator<__hash_table::iterator>'
297+
//
298+
// __hash_map_iterator::__i_ is a __hash_table::iterator (aka
299+
// __hash_iterator<__node_pointer>)
300+
auto hash_iter_sp = valobj_sp->GetChildMemberWithName("__i_");
301+
if (!hash_iter_sp)
319302
return lldb::ChildCacheState::eRefetch;
320303

321-
auto exprPathOptions = ValueObject::GetValueForExpressionPathOptions()
322-
.DontCheckDotVsArrowSyntax()
323-
.SetSyntheticChildrenTraversal(
324-
ValueObject::GetValueForExpressionPathOptions::
325-
SyntheticChildrenTraversal::None);
326-
327-
// This must be a ValueObject* because it is a child of the ValueObject we
328-
// are producing children for it if were a ValueObjectSP, we would end up
329-
// with a loop (iterator -> synthetic -> child -> parent == iterator) and
330-
// that would in turn leak memory by never allowing the ValueObjects to die
331-
// and free their memory.
332-
m_iter_ptr =
333-
valobj_sp
334-
->GetValueForExpressionPath(".__i_.__node_", nullptr, nullptr,
335-
exprPathOptions, nullptr)
336-
.get();
337-
338-
if (m_iter_ptr) {
339-
auto iter_child(valobj_sp->GetChildMemberWithName("__i_"));
340-
if (!iter_child) {
341-
m_iter_ptr = nullptr;
342-
return lldb::ChildCacheState::eRefetch;
343-
}
344-
345-
CompilerType node_type(iter_child->GetCompilerType()
346-
.GetTypeTemplateArgument(0)
347-
.GetPointeeType());
348-
349-
CompilerType pair_type(node_type.GetTypeTemplateArgument(0));
350-
351-
std::string name;
352-
uint64_t bit_offset_ptr;
353-
uint32_t bitfield_bit_size_ptr;
354-
bool is_bitfield_ptr;
355-
356-
pair_type = pair_type.GetFieldAtIndex(
357-
0, name, &bit_offset_ptr, &bitfield_bit_size_ptr, &is_bitfield_ptr);
358-
if (!pair_type) {
359-
m_iter_ptr = nullptr;
360-
return lldb::ChildCacheState::eRefetch;
361-
}
304+
// Type is '__hash_iterator<__node_pointer>'
305+
auto hash_iter_type = hash_iter_sp->GetCompilerType();
306+
if (!hash_iter_type.IsValid())
307+
return lldb::ChildCacheState::eRefetch;
362308

363-
uint64_t addr = m_iter_ptr->GetValueAsUnsigned(LLDB_INVALID_ADDRESS);
364-
m_iter_ptr = nullptr;
309+
// Type is '__node_pointer'
310+
auto node_pointer_type = hash_iter_type.GetTypeTemplateArgument(0);
311+
if (!node_pointer_type.IsValid())
312+
return lldb::ChildCacheState::eRefetch;
365313

366-
if (addr == 0 || addr == LLDB_INVALID_ADDRESS)
367-
return lldb::ChildCacheState::eRefetch;
314+
// Cast the __hash_iterator to a __node_pointer (which stores our key/value
315+
// pair)
316+
auto hash_node_sp = hash_iter_sp->Cast(node_pointer_type);
317+
if (!hash_node_sp)
318+
return lldb::ChildCacheState::eRefetch;
368319

369-
auto ts = pair_type.GetTypeSystem();
370-
auto ast_ctx = ts.dyn_cast_or_null<TypeSystemClang>();
371-
if (!ast_ctx)
320+
auto key_value_sp = hash_node_sp->GetChildMemberWithName("__value_");
321+
if (!key_value_sp) {
322+
// clang-format off
323+
// Since D101206 (ba79fb2e1f), libc++ wraps the `__value_` in an
324+
// anonymous union.
325+
// Child 0: __hash_node_base base class
326+
// Child 1: __hash_
327+
// Child 2: anonymous union
328+
// clang-format on
329+
auto anon_union_sp = hash_node_sp->GetChildAtIndex(2);
330+
if (!anon_union_sp)
372331
return lldb::ChildCacheState::eRefetch;
373332

374-
// Mimick layout of std::__hash_iterator::__node_ and read it in
375-
// from process memory.
376-
//
377-
// The following shows the contiguous block of memory:
378-
//
379-
// +-----------------------------+ class __hash_node_base
380-
// __node_ | __next_pointer __next_; |
381-
// +-----------------------------+ class __hash_node
382-
// | size_t __hash_; |
383-
// | __node_value_type __value_; | <<< our key/value pair
384-
// +-----------------------------+
385-
//
386-
CompilerType tree_node_type = ast_ctx->CreateStructForIdentifier(
387-
llvm::StringRef(),
388-
{{"__next_",
389-
ast_ctx->GetBasicType(lldb::eBasicTypeVoid).GetPointerType()},
390-
{"__hash_", ast_ctx->GetBasicType(lldb::eBasicTypeUnsignedLongLong)},
391-
{"__value_", pair_type}});
392-
std::optional<uint64_t> size = tree_node_type.GetByteSize(nullptr);
393-
if (!size)
394-
return lldb::ChildCacheState::eRefetch;
395-
WritableDataBufferSP buffer_sp(new DataBufferHeap(*size, 0));
396-
ProcessSP process_sp(target_sp->GetProcessSP());
397-
Status error;
398-
process_sp->ReadMemory(addr, buffer_sp->GetBytes(),
399-
buffer_sp->GetByteSize(), error);
400-
if (error.Fail())
333+
key_value_sp = anon_union_sp->GetChildMemberWithName("__value_");
334+
if (!key_value_sp)
401335
return lldb::ChildCacheState::eRefetch;
402-
DataExtractor extractor(buffer_sp, process_sp->GetByteOrder(),
403-
process_sp->GetAddressByteSize());
404-
auto pair_sp = CreateValueObjectFromData(
405-
"pair", extractor, valobj_sp->GetExecutionContextRef(), tree_node_type);
406-
if (pair_sp)
407-
m_pair_sp = pair_sp->GetChildAtIndex(2);
408336
}
409337

338+
// Create the synthetic child, which is a pair where the key and value can be
339+
// retrieved by querying the synthetic frontend for
340+
// GetIndexOfChildWithName("first") and GetIndexOfChildWithName("second")
341+
// respectively.
342+
//
343+
// std::unordered_map stores the actual key/value pair in
344+
// __hash_value_type::__cc_ (or previously __cc).
345+
auto potential_child_sp = key_value_sp->Clone(ConstString("pair"));
346+
if (potential_child_sp)
347+
if (potential_child_sp->GetNumChildrenIgnoringErrors() == 1)
348+
if (auto child0_sp = potential_child_sp->GetChildAtIndex(0);
349+
child0_sp->GetName() == "__cc_" || child0_sp->GetName() == "__cc")
350+
potential_child_sp = child0_sp->Clone(ConstString("pair"));
351+
352+
m_pair_sp = potential_child_sp;
353+
410354
return lldb::ChildCacheState::eRefetch;
411355
}
412356

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/iterator/TestDataFormatterLibccIterator.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,3 +59,19 @@ def cleanup():
5959

6060
self.expect("frame variable svI", substrs=['item = "hello"'])
6161
self.expect("expr svI", substrs=['item = "hello"'])
62+
63+
self.expect("frame variable iiumI", substrs=["first = 61453", "second = 51966"])
64+
self.expect("expr iiumI", substrs=["first = 61453", "second = 51966"])
65+
66+
self.expect("frame variable siumI", substrs=['first = "hello"', "second = 137"])
67+
self.expect("expr siumI", substrs=['first = "hello"', "second = 137"])
68+
69+
self.expect("frame variable iiumI.first", substrs=["first = 61453"])
70+
self.expect("frame variable iiumI.first", substrs=["second"], matching=False)
71+
self.expect("frame variable iiumI.second", substrs=["second = 51966"])
72+
self.expect("frame variable iiumI.second", substrs=["first"], matching=False)
73+
74+
self.expect("frame variable siumI.first", substrs=['first = "hello"'])
75+
self.expect("frame variable siumI.first", substrs=["second"], matching=False)
76+
self.expect("frame variable siumI.second", substrs=["second = 137"])
77+
self.expect("frame variable siumI.second", substrs=["first"], matching=False)
Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,38 +1,50 @@
1-
#include <string>
21
#include <map>
2+
#include <string>
33
#include <vector>
44

55
typedef std::map<int, int> intint_map;
66
typedef std::map<std::string, int> strint_map;
77

8+
typedef std::unordered_map<int, int> intint_umap;
9+
typedef std::unordered_map<std::string, int> strint_umap;
10+
811
typedef std::vector<int> int_vector;
912
typedef std::vector<std::string> string_vector;
1013

11-
typedef intint_map::iterator iimter;
12-
typedef strint_map::iterator simter;
14+
typedef intint_map::iterator ii_map_iter;
15+
typedef strint_map::iterator si_map_iter;
16+
typedef intint_umap::iterator ii_umap_iter;
17+
typedef strint_umap::iterator si_umap_iter;
1318

1419
typedef int_vector::iterator ivter;
1520
typedef string_vector::iterator svter;
1621

17-
int main()
18-
{
19-
intint_map iim;
20-
iim[0xABCD] = 0xF0F1;
22+
int main() {
23+
intint_map iim;
24+
iim[0xABCD] = 0xF0F1;
25+
26+
strint_map sim;
27+
sim["world"] = 42;
28+
29+
intint_umap iium;
30+
iium[0xF00D] = 0xCAFE;
2131

22-
strint_map sim;
23-
sim["world"] = 42;
32+
strint_umap sium;
33+
sium["hello"] = 137;
2434

25-
int_vector iv;
26-
iv.push_back(3);
35+
int_vector iv;
36+
iv.push_back(3);
2737

28-
string_vector sv;
29-
sv.push_back("hello");
38+
string_vector sv;
39+
sv.push_back("hello");
3040

31-
iimter iimI = iim.begin();
32-
simter simI = sim.begin();
41+
ii_map_iter iimI = iim.begin();
42+
si_map_iter simI = sim.begin();
43+
ii_umap_iter iiumI = iium.begin();
44+
si_umap_iter siumI = sium.begin();
3345

34-
ivter ivI = iv.begin();
35-
svter svI = sv.begin();
46+
ivter ivI = iv.begin();
47+
svter svI = sv.begin();
3648

37-
return 0; // Set break point at this line.
49+
return 0; // Set break point at this line.
3850
}

0 commit comments

Comments
 (0)