[lldb] Expose std::pair children for unordered_map

[kastiglione]

Summary

Change the behavior of the libc++ std::unordered_map synthetic provider to
present children as std::pair values, just like std::map does. This change
also applies equally to std::unordered_multimap.

Before this change, the synthetic provider for libc++ std::unordered_map has
returned children whose type is an trivial wrapper struct
(__hash_value_type). For example, given an unordered map initialized with
{{1,2}, {3, 4}}, the output is:

(std::unordered_map<int, int, std::hash<int>, std::equal_to<int>, std::allocator<std::pair<const int, int> > >) map = size=2 {
  [0] = {
    __cc = (first = 3, second = 4)
  }
  [1] = {
    __cc = (first = 1, second = 2)
  }
}

Compare this output to std::map:

(std::map<int, int, std::less<int>, std::allocator<std::pair<const int, int> > >) map = size=2 {
  [0] = (first = 1, second = 2)
  [1] = (first = 3, second = 4)

The children of std::map have the type std::pair<const Key, T>.

For std::unordered_map, it's not useful to expose the internal wrapper type.
This type is structurally a trivial wrapper, having a single field (__cc)
whose type is std::pair. This change exposes the pair instead of the internal
wrapper type.

It appears the synthetic provider implementation for unordered_map was meant
to provide this behavior (see rGd22a94377f7554a7e9df050f6dfc3ee42384e3fe). The
code has both an m_node_type and an m_element_type, but has only used the
former. The latter, m_element_type, is exactly the type needed for the
children pairs. With this existing code, it's not much of a change to make this
work.

[kastiglione]

some tabs snuck in

[kastiglione]

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

128

This block of changes is needed to support unordered_set, because this synthetic provider is shared by unordered_set and unordered_map.

For unordered_map, the element type is an internal hash node. For unoredered_set, it's the type T from unordered_set<T>. This part of the code needs to know whether there's a hash node here, and to step down through the __cc field.

I am not sure what the best way to do this is. I could do a string prefix check against std::__hash_value_type. I could do a smarter full string equality check, but could that possibly have false negatives if the wrong string type name is constructed. As it currently is, I assume if there's a one or more fields, and if the first one is named __cc, then it's assumed to be an internal hash node.

Suggestions welcome.

162

This change is the fix needed to get unordered_map working.

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py

51

This regex checks that the string __cc = is not in the output.

[jingham]

Sorry for the delay here.

This output is definitely nicer than the previous version, so this change is desirable.

If you really are switching on unordered_map vrs. unordered_set behavior, can you check the backend ValueObject's type to determine whether you are dealing with a map or set? It might be nice to make that a little more explicit. I can't see anything in this file that indicates that this synthetic child provider is used for both classes, so having something conditional on it being a map is surprising.

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

128

IIUC, this depends on whether the backend is of type unordered_map or unordered_set? Can you just check the type of the backend?

[kastiglione]

Use backend's type name to differentiate between unordered_{map,set}

[kastiglione]

Ignore the internal field name and use type names for conditions

[kastiglione]

Handle GetTypeName() returning strings both with and without std:: namespace

[kastiglione]

fix comment grammar

[Michael137]

LGTM

[kastiglione]

@jingham what do you think of this update?

[JDevlieghere]

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

73–74

Maybe a little helper could avoid some duplication here and below. Something like:

static bool isStdType(llvm::StringRef name, llvm::StringRef type) {
	return name.startswith(type) || name.startswith(llvm::Twine("std::__1::") + type);
}

[kastiglione]

Add @JDevlieghere helper

[kastiglione]

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

73–74

good idea. Since startswith doesn't accept a Twine I went with a slightly different approach, to the same effect.

[JDevlieghere]

LGTM

[rsmith]

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

72

This is not right -- libc++ allows customizing its inline namespace name for versioning purpose; it's not __1 across all deployments. For example, with the libc++ unstable ABI, it will likely be something else. To handle this properly I think you'll need to look for either std::name< or something like std::[a-zA-Z0-9_]*::name<.

[kastiglione]

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

72

thanks. Looks like CPPLanguageRuntime.cpp has makes some similar assumptions.

[kastiglione]

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

72

D133259

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

 //===-- LibCxxUnorderedMap.cpp --------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
size_t lldb_private::formatters::LibcxxStdUnorderedMapSyntheticFrontEnd::
     CalculateNumChildren() {
   return m_num_elements;
 }
 
 lldb::ValueObjectSP lldb_private::formatters::
     LibcxxStdUnorderedMapSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
   if (idx >= CalculateNumChildren())
     return lldb::ValueObjectSP();
   if (m_tree == nullptr)
     return lldb::ValueObjectSP();
 
   while (idx >= m_elements_cache.size()) {
     if (m_next_element == nullptr)
       return lldb::ValueObjectSP();
 
     Status error;
     ValueObjectSP node_sp = m_next_element->Dereference(error);
     if (!node_sp || error.Fail())
       return lldb::ValueObjectSP();
if (!hash_sp || !value_sp) {
 
         if (!first_sp)
           return nullptr;
         m_element_type = first_sp->GetCompilerType();
         m_element_type = m_element_type.GetTypeTemplateArgument(0);
         m_element_type = m_element_type.GetPointeeType();
         m_node_type = m_element_type;
         m_element_type = m_element_type.GetTypeTemplateArgument(0);
+        if (m_element_type.GetNumFields() > 0) {
+    // Check for - and use - the underlying key/value container type used
+    // within std::unordered_map.
           std::string name;
-        m_element_type =
+          auto key_value_type =
               m_element_type.GetFieldAtIndex(0, name, nullptr, nullptr, nullptr);
-        m_element_type = m_element_type.GetTypedefedType();
+          // __cc is a field of __hash_value_type, which is a typedef.
+          if (name == "__cc")
+            m_element_type = key_value_type.GetTypedefedType();
+        }
       }
       if (!m_node_type)
         return nullptr;
       node_sp = node_sp->Cast(m_node_type);
       value_sp = node_sp->GetChildMemberWithName(ConstString("__value_"), true);
       hash_sp = node_sp->GetChildMemberWithName(ConstString("__hash_"), true);
       if (!value_sp || !hash_sp)
         return nullptr;
lldb::ValueObjectSP lldb_private::formatters::
   Status error;
   val_hash.first->GetData(data, error);
   if (error.Fail())
     return lldb::ValueObjectSP();
   const bool thread_and_frame_only_if_stopped = true;
   ExecutionContext exe_ctx = val_hash.first->GetExecutionContextRef().Lock(
       thread_and_frame_only_if_stopped);
   return CreateValueObjectFromData(stream.GetString(), data, exe_ctx,
-                                   val_hash.first->GetCompilerType());
+                                   m_element_type);
 }
 
 bool lldb_private::formatters::LibcxxStdUnorderedMapSyntheticFrontEnd::
     Update() {
   m_num_elements = 0;
   m_next_element = nullptr;
   m_elements_cache.clear();
   ValueObjectSP table_sp =

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py

def do_test_with_run_command(self, stdlib_type):
 
         ns = self.namespace
 
         # We check here that the map shows 0 children even with corrupt data.
         self.look_for_content_and_continue(
             "corrupt_map", ['%s::unordered_map' %
                     ns, 'size=0 {}'])
 
+        must_not_contain__cc = r'(?s)^(?!.*\b__cc = )'
+
         self.look_for_content_and_continue(
-            "map", ['%s::unordered_map' %
-                    ns, 'size=5 {', 'hello', 'world', 'this', 'is', 'me'])
+            "map", ['%s::unordered_map' % ns,
+                    must_not_contain__cc,
+                    'size=5 {', 'hello', 'world', 'this', 'is', 'me'])
 
         self.look_for_content_and_continue(
-            "mmap", ['%s::unordered_multimap' % ns, 'size=6 {', 'first = 3', 'second = "this"',
+            "mmap", ['%s::unordered_multimap' % ns,
+                     must_not_contain__cc,
+                     'size=6 {', 'first = 3', 'second = "this"',
                      'first = 2', 'second = "hello"'])
 
         self.look_for_content_and_continue(
             "iset", ['%s::unordered_set' %
                      ns, 'size=5 {', '\[\d\] = 5', '\[\d\] = 3', '\[\d\] = 2'])
 
         self.look_for_content_and_continue(
             "sset", ['%s::unordered_set' % ns, 'size=5 {', '\[\d\] = "is"', '\[\d\] = "world"',