Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb] Support new libc++ __compressed_pair layout #96538

Merged
merged 7 commits into from
Sep 16, 2024

Conversation

Michael137
Copy link
Member

@Michael137 Michael137 commented Jun 24, 2024

This patch is in preparation for the __compressed_pair refactor in #76756.

This is mostly reviewable now. With the new layout we no longer need to unwrap the __compressed_pair. Instead, we just need to look for child members. E.g., to get to the underlying pointer of std::unique_ptr we no longer do,

GetFirstValueOfCXXCompressedPair(GetChildMemberWithName("__ptr_"))

but instead do

GetChildMemberWithName("__ptr_")

We need to be slightly careful because previously the __compressed_pair had a member called __value_, whereas now __value_ might be a member of the class that used to hold the __compressed_pair. So before unwrapping the pair, we added checks for isOldCompressedLayout (not sure yet whether folding this check into GetFirstValueOfCXXCompressedPair is better).

Copy link

github-actions bot commented Jun 24, 2024

✅ With the latest revision this PR passed the Python code formatter.

@Michael137 Michael137 force-pushed the bugfix/lldb-compressed_pair-rework branch 3 times, most recently from dcc6e17 to 2b82d72 Compare July 10, 2024 11:19
@Michael137 Michael137 force-pushed the bugfix/lldb-compressed_pair-rework branch 4 times, most recently from 125e244 to 231e0dd Compare July 10, 2024 16:47
@Michael137 Michael137 marked this pull request as ready for review July 10, 2024 16:48
@llvmbot llvmbot added the lldb label Jul 10, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 10, 2024

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

This patch is in preparation for the __compressed_pair refactor in #76756.

This is mostly reviewable now. With the new layout we no longer need to unwrap the __compressed_pair. Instead, we just need to look for child members. E.g., to get to the underlying pointer of std::unique_ptr we no longer do,

GetFirstValueOfCXXCompressedPair(GetChildMemberWithName("__ptr_"))

but instead do

GetChildMemberWithName("__ptr_")

We need to be slightly careful because previously the __compressed_pair had a member called __value_, whereas now __value_ might be a member of the class that used to hold the __compressed_pair. So before unwrapping the pair, we added checks for isOldCompressedLayout (not sure yet whether folding this check into GetFirstValueOfCXXCompressedPair is better).


Patch is 26.60 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/96538.diff

9 Files Affected:

  • (modified) lldb/examples/synthetic/libcxx.py (+20-6)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp (+63-22)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxx.h (+2-1)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp (+43-29)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp (+31-10)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp (+103-63)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp (+19-19)
  • (modified) lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/list/TestDataFormatterGenericList.py (+1-1)
  • (modified) lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/simulator/main.cpp (+1)
diff --git a/lldb/examples/synthetic/libcxx.py b/lldb/examples/synthetic/libcxx.py
index 474aaa428fa23..060ff90100849 100644
--- a/lldb/examples/synthetic/libcxx.py
+++ b/lldb/examples/synthetic/libcxx.py
@@ -721,6 +721,12 @@ def _get_value_of_compressed_pair(self, pair):
     def update(self):
         logger = lldb.formatters.Logger.Logger()
         try:
+            has_compressed_pair_layout = True
+            alloc_valobj = self.valobj.GetChildMemberWithName("__alloc_")
+            size_valobj = self.valobj.GetChildMemberWithName("__size_")
+            if alloc_valobj.IsValid() and size_valobj.IsValid():
+                has_compressed_pair_layout = False
+
             # A deque is effectively a two-dim array, with fixed width.
             # 'map' contains pointers to the rows of this array. The
             # full memory area allocated by the deque is delimited
@@ -734,9 +740,13 @@ def update(self):
             # variable tells which element in this NxM array is the 0th
             # one, and the 'size' element gives the number of elements
             # in the deque.
-            count = self._get_value_of_compressed_pair(
-                self.valobj.GetChildMemberWithName("__size_")
-            )
+            if has_compressed_pair_layout:
+                count = self._get_value_of_compressed_pair(
+                    self.valobj.GetChildMemberWithName("__size_")
+                )
+            else:
+                count = size_valobj.GetValueAsUnsigned(0)
+
             # give up now if we cant access memory reliably
             if self.block_size < 0:
                 logger.write("block_size < 0")
@@ -748,9 +758,13 @@ def update(self):
             self.map_begin = map_.GetChildMemberWithName("__begin_")
             map_begin = self.map_begin.GetValueAsUnsigned(0)
             map_end = map_.GetChildMemberWithName("__end_").GetValueAsUnsigned(0)
-            map_endcap = self._get_value_of_compressed_pair(
-                map_.GetChildMemberWithName("__end_cap_")
-            )
+
+            if has_compressed_pair_layout:
+                map_endcap = self._get_value_of_compressed_pair(
+                    map_.GetChildMemberWithName("__end_cap_")
+                )
+            else:
+                map_endcap = map_.GetChildMemberWithName("__end_cap_").GetValueAsUnsigned(0)
 
             # check consistency
             if not map_first <= map_begin <= map_end <= map_endcap:
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
index feaa51a96843a..7d3b2410a7296 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -27,6 +27,7 @@
 #include "Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "lldb/lldb-enumerations.h"
+#include "lldb/lldb-forward.h"
 #include <optional>
 #include <tuple>
 
@@ -34,6 +35,32 @@ using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::formatters;
 
+static void consumeInlineNamespace(llvm::StringRef &name) {
+  // Delete past an inline namespace, if any: __[a-zA-Z0-9_]+::
+  auto scratch = name;
+  if (scratch.consume_front("__") && std::isalnum(scratch[0])) {
+    scratch = scratch.drop_while([](char c) { return std::isalnum(c); });
+    if (scratch.consume_front("::")) {
+      // Successfully consumed a namespace.
+      name = scratch;
+    }
+  }
+}
+
+bool lldb_private::formatters::isOldCompressedPairLayout(
+    ValueObject &pair_obj) {
+  return isStdTemplate(pair_obj.GetTypeName(), "__compressed_pair");
+}
+
+bool lldb_private::formatters::isStdTemplate(ConstString type_name,
+                                             llvm::StringRef type) {
+  llvm::StringRef name = type_name.GetStringRef();
+  // The type name may be prefixed with `std::__<inline-namespace>::`.
+  if (name.consume_front("std::"))
+    consumeInlineNamespace(name);
+  return name.consume_front(type) && name.starts_with("<");
+}
+
 lldb::ValueObjectSP lldb_private::formatters::GetChildMemberWithName(
     ValueObject &obj, llvm::ArrayRef<ConstString> alternative_names) {
   for (ConstString name : alternative_names) {
@@ -53,7 +80,7 @@ lldb_private::formatters::GetFirstValueOfLibCXXCompressedPair(
   if (first_child)
     value = first_child->GetChildMemberWithName("__value_");
   if (!value) {
-    // pre-r300140 member name
+    // pre-c88580c member name
     value = pair.GetChildMemberWithName("__first_");
   }
   return value;
@@ -70,7 +97,7 @@ lldb_private::formatters::GetSecondValueOfLibCXXCompressedPair(
     }
   }
   if (!value) {
-    // pre-r300140 member name
+    // pre-c88580c member name
     value = pair.GetChildMemberWithName("__second_");
   }
   return value;
@@ -176,7 +203,9 @@ bool lldb_private::formatters::LibcxxUniquePointerSummaryProvider(
   if (!ptr_sp)
     return false;
 
-  ptr_sp = GetFirstValueOfLibCXXCompressedPair(*ptr_sp);
+  if (isOldCompressedPairLayout(*ptr_sp))
+    ptr_sp = GetFirstValueOfLibCXXCompressedPair(*ptr_sp);
+
   if (!ptr_sp)
     return false;
 
@@ -363,13 +392,22 @@ lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEnd::Update() {
 
   // Retrieve the actual pointer and the deleter, and clone them to give them
   // user-friendly names.
-  ValueObjectSP value_pointer_sp = GetFirstValueOfLibCXXCompressedPair(*ptr_sp);
-  if (value_pointer_sp)
-    m_value_ptr_sp = value_pointer_sp->Clone(ConstString("pointer"));
+  if (isOldCompressedPairLayout(*ptr_sp)) {
+    if (ValueObjectSP value_pointer_sp =
+            GetFirstValueOfLibCXXCompressedPair(*ptr_sp))
+      m_value_ptr_sp = value_pointer_sp->Clone(ConstString("pointer"));
+
+    if (ValueObjectSP deleter_sp =
+            GetSecondValueOfLibCXXCompressedPair(*ptr_sp))
+      m_deleter_sp = deleter_sp->Clone(ConstString("deleter"));
+  } else {
+    m_value_ptr_sp = ptr_sp->Clone(ConstString("pointer"));
 
-  ValueObjectSP deleter_sp = GetSecondValueOfLibCXXCompressedPair(*ptr_sp);
-  if (deleter_sp)
-    m_deleter_sp = deleter_sp->Clone(ConstString("deleter"));
+    if (ValueObjectSP deleter_sp =
+            valobj_sp->GetChildMemberWithName("__deleter_"))
+      if (deleter_sp->GetNumChildrenIgnoringErrors() > 0)
+        m_deleter_sp = deleter_sp->Clone(ConstString("deleter"));
+  }
 
   return lldb::ChildCacheState::eRefetch;
 }
@@ -407,24 +445,27 @@ namespace {
 enum class StringLayout { CSD, DSC };
 }
 
+static ValueObjectSP ExtractLibCxxStringData(ValueObject &valobj) {
+  if (auto rep_sp = valobj.GetChildMemberWithName("__rep_"))
+    return rep_sp;
+
+  ValueObjectSP valobj_r_sp = valobj.GetChildMemberWithName("__r_");
+  if (!valobj_r_sp || !valobj_r_sp->GetError().Success())
+    return nullptr;
+
+  if (!isOldCompressedPairLayout(*valobj_r_sp))
+    return nullptr;
+
+  return GetFirstValueOfLibCXXCompressedPair(*valobj_r_sp);
+}
+
 /// Determine the size in bytes of \p valobj (a libc++ std::string object) and
 /// extract its data payload. Return the size + payload pair.
 // TODO: Support big-endian architectures.
 static std::optional<std::pair<uint64_t, ValueObjectSP>>
 ExtractLibcxxStringInfo(ValueObject &valobj) {
-  ValueObjectSP valobj_r_sp = valobj.GetChildMemberWithName("__r_");
-  if (!valobj_r_sp || !valobj_r_sp->GetError().Success())
-    return {};
-
-  // __r_ is a compressed_pair of the actual data and the allocator. The data we
-  // want is in the first base class.
-  ValueObjectSP valobj_r_base_sp = valobj_r_sp->GetChildAtIndex(0);
-  if (!valobj_r_base_sp)
-    return {};
-
-  ValueObjectSP valobj_rep_sp =
-      valobj_r_base_sp->GetChildMemberWithName("__value_");
-  if (!valobj_rep_sp)
+  ValueObjectSP valobj_rep_sp = ExtractLibCxxStringData(valobj);
+  if (!valobj_rep_sp || !valobj_rep_sp->GetError().Success())
     return {};
 
   ValueObjectSP l = valobj_rep_sp->GetChildMemberWithName("__l");
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
index 5307b5251ca84..dad033611b38d 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
@@ -25,7 +25,8 @@ GetChildMemberWithName(ValueObject &obj,
 
 lldb::ValueObjectSP GetFirstValueOfLibCXXCompressedPair(ValueObject &pair);
 lldb::ValueObjectSP GetSecondValueOfLibCXXCompressedPair(ValueObject &pair);
-
+bool isOldCompressedPairLayout(ValueObject &pair_obj);
+bool isStdTemplate(ConstString type_name, llvm::StringRef type);
 
 bool LibcxxStringSummaryProviderASCII(
     ValueObject &valobj, Stream &stream,
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp
index d7cfeb30557c3..4479f592fc2d2 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp
@@ -17,6 +17,7 @@
 #include "lldb/Utility/Endian.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/Stream.h"
+#include "lldb/lldb-enumerations.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -294,12 +295,17 @@ lldb::ChildCacheState ForwardListFrontEnd::Update() {
 
   ValueObjectSP impl_sp(m_backend.GetChildMemberWithName("__before_begin_"));
   if (!impl_sp)
-    return lldb::ChildCacheState::eRefetch;
-  impl_sp = GetFirstValueOfLibCXXCompressedPair(*impl_sp);
+    return ChildCacheState::eRefetch;
+
+  if (isOldCompressedPairLayout(*impl_sp))
+    impl_sp = GetFirstValueOfLibCXXCompressedPair(*impl_sp);
+
   if (!impl_sp)
-    return lldb::ChildCacheState::eRefetch;
+    return ChildCacheState::eRefetch;
+
   m_head = impl_sp->GetChildMemberWithName("__next_").get();
-  return lldb::ChildCacheState::eRefetch;
+
+  return ChildCacheState::eRefetch;
 }
 
 ListFrontEnd::ListFrontEnd(lldb::ValueObjectSP valobj_sp)
@@ -313,34 +319,42 @@ llvm::Expected<uint32_t> ListFrontEnd::CalculateNumChildren() {
     return m_count;
   if (!m_head || !m_tail || m_node_address == 0)
     return 0;
-  ValueObjectSP size_alloc(m_backend.GetChildMemberWithName("__size_alloc_"));
-  if (size_alloc) {
-    ValueObjectSP value = GetFirstValueOfLibCXXCompressedPair(*size_alloc);
-    if (value) {
-      m_count = value->GetValueAsUnsigned(UINT32_MAX);
-    }
+
+  ValueObjectSP size_node_sp(m_backend.GetChildMemberWithName("__size_"));
+  if (!size_node_sp) {
+    size_node_sp = m_backend.GetChildMemberWithName(
+        "__size_alloc_"); // pre-compressed_pair rework
+
+    if (!isOldCompressedPairLayout(*size_node_sp))
+      return llvm::createStringError("Unexpected std::list layout: expected "
+                                     "old __compressed_pair layout.");
+
+    size_node_sp = GetFirstValueOfLibCXXCompressedPair(*size_node_sp);
   }
-  if (m_count != UINT32_MAX) {
+
+  if (size_node_sp)
+    m_count = size_node_sp->GetValueAsUnsigned(UINT32_MAX);
+
+  if (m_count != UINT32_MAX)
     return m_count;
-  } else {
-    uint64_t next_val = m_head->GetValueAsUnsigned(0);
-    uint64_t prev_val = m_tail->GetValueAsUnsigned(0);
-    if (next_val == 0 || prev_val == 0)
-      return 0;
-    if (next_val == m_node_address)
-      return 0;
-    if (next_val == prev_val)
-      return 1;
-    uint64_t size = 2;
-    ListEntry current(m_head);
-    while (current.next() && current.next().value() != m_node_address) {
-      size++;
-      current = current.next();
-      if (size > m_list_capping_size)
-        break;
-    }
-    return m_count = (size - 1);
+
+  uint64_t next_val = m_head->GetValueAsUnsigned(0);
+  uint64_t prev_val = m_tail->GetValueAsUnsigned(0);
+  if (next_val == 0 || prev_val == 0)
+    return 0;
+  if (next_val == m_node_address)
+    return 0;
+  if (next_val == prev_val)
+    return 1;
+  uint64_t size = 2;
+  ListEntry current(m_head);
+  while (current.next() && current.next().value() != m_node_address) {
+    size++;
+    current = current.next();
+    if (size > m_list_capping_size)
+      break;
   }
+  return m_count = (size - 1);
 }
 
 lldb::ValueObjectSP ListFrontEnd::GetChildAtIndex(uint32_t idx) {
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
index 5106a63d531f8..b89afd6a388ab 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
@@ -202,6 +202,8 @@ class LibcxxStdMapSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
   size_t GetIndexOfChildWithName(ConstString name) override;
 
 private:
+  llvm::Expected<uint32_t> CalculateNumChildrenForOldCompressedPairLayout();
+
   /// Returns the ValueObject for the __tree_node type that
   /// holds the key/value pair of the node at index \ref idx.
   ///
@@ -254,6 +256,29 @@ lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::
     Update();
 }
 
+llvm::Expected<uint32_t>
+lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::
+    CalculateNumChildrenForOldCompressedPairLayout() {
+  ValueObjectSP node_sp(m_tree->GetChildMemberWithName("__pair3_"));
+  if (!node_sp)
+    return 0;
+
+  // TODO: or should this just be: assert
+  // (!isOldCompressedPairLayout(*node_sp));
+  if (!isOldCompressedPairLayout(*node_sp))
+    return llvm::createStringError("Unexpected std::map layout: expected "
+                                   "old __compressed_pair layout.");
+
+  node_sp = GetFirstValueOfLibCXXCompressedPair(*node_sp);
+
+  if (!node_sp)
+    return 0;
+
+  m_count = node_sp->GetValueAsUnsigned(0);
+
+  return m_count;
+}
+
 llvm::Expected<uint32_t> lldb_private::formatters::
     LibcxxStdMapSyntheticFrontEnd::CalculateNumChildren() {
   if (m_count != UINT32_MAX)
@@ -262,17 +287,12 @@ llvm::Expected<uint32_t> lldb_private::formatters::
   if (m_tree == nullptr)
     return 0;
 
-  ValueObjectSP size_node(m_tree->GetChildMemberWithName("__pair3_"));
-  if (!size_node)
-    return 0;
-
-  size_node = GetFirstValueOfLibCXXCompressedPair(*size_node);
-
-  if (!size_node)
-    return 0;
+  if (auto node_sp = m_tree->GetChildMemberWithName("__size_")) {
+    m_count = node_sp->GetValueAsUnsigned(0);
+    return m_count;
+  }
 
-  m_count = size_node->GetValueAsUnsigned(0);
-  return m_count;
+  return CalculateNumChildrenForOldCompressedPairLayout();
 }
 
 ValueObjectSP
@@ -371,6 +391,7 @@ lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::Update() {
   m_tree = m_backend.GetChildMemberWithName("__tree_").get();
   if (!m_tree)
     return lldb::ChildCacheState::eRefetch;
+
   m_root_node = m_tree->GetChildMemberWithName("__begin_node_").get();
   m_node_ptr_type =
       m_tree->GetCompilerType().GetDirectNestedTypeWithName("__node_pointer");
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
index 93e7f4f4fd86c..2f65c726aa51a 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
@@ -19,6 +19,7 @@
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/Stream.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -44,6 +45,10 @@ class LibcxxStdUnorderedMapSyntheticFrontEnd
   size_t GetIndexOfChildWithName(ConstString name) override;
 
 private:
+  CompilerType GetNodeType();
+  CompilerType GetElementType(CompilerType node_type);
+  llvm::Expected<size_t> CalculateNumChildrenImpl(ValueObject &table);
+
   CompilerType m_element_type;
   CompilerType m_node_type;
   ValueObject *m_tree = nullptr;
@@ -91,29 +96,53 @@ llvm::Expected<uint32_t> lldb_private::formatters::
   return m_num_elements;
 }
 
-static void consumeInlineNamespace(llvm::StringRef &name) {
-  // Delete past an inline namespace, if any: __[a-zA-Z0-9_]+::
-  auto scratch = name;
-  if (scratch.consume_front("__") && std::isalnum(scratch[0])) {
-    scratch = scratch.drop_while([](char c) { return std::isalnum(c); });
-    if (scratch.consume_front("::")) {
-      // Successfully consumed a namespace.
-      name = scratch;
-    }
-  }
+static bool isUnorderedMap(ConstString type_name) {
+  return isStdTemplate(type_name, "unordered_map") ||
+         isStdTemplate(type_name, "unordered_multimap");
 }
 
-static bool isStdTemplate(ConstString type_name, llvm::StringRef type) {
-  llvm::StringRef name = type_name.GetStringRef();
-  // The type name may be prefixed with `std::__<inline-namespace>::`.
-  if (name.consume_front("std::"))
-    consumeInlineNamespace(name);
-  return name.consume_front(type) && name.starts_with("<");
+CompilerType lldb_private::formatters::LibcxxStdUnorderedMapSyntheticFrontEnd::
+    GetElementType(CompilerType node_type) {
+  CompilerType element_type = node_type.GetTypeTemplateArgument(0);
+
+  // This synthetic provider is used for both unordered_(multi)map and
+  // unordered_(multi)set. For unordered_map, the element type has an
+  // additional type layer, an internal struct (`__hash_value_type`)
+  // that wraps a std::pair. Peel away the internal wrapper type - whose
+  // structure is of no value to users, to expose the std::pair. This
+  // matches the structure returned by the std::map synthetic provider.
+  if (isUnorderedMap(m_backend.GetTypeName())) {
+    std::string name;
+    CompilerType field_type =
+        element_type.GetFieldAtIndex(0, name, nullptr, nullptr, nullptr);
+    CompilerType actual_type = field_type.GetTypedefedType();
+    if (isStdTemplate(actual_type.GetTypeName(), "pair"))
+      element_type = actual_type;
+  }
+
+  return element_type;
 }
 
-static bool isUnorderedMap(ConstString type_name) {
-  return isStdTemplate(type_name, "unordered_map") ||
-         isStdTemplate(type_name, "unordered_multimap");
+CompilerType lldb_private::formatters::LibcxxStdUnorderedMapSyntheticFrontEnd::
+    GetNodeType() {
+  auto node_sp = m_backend.GetChildAtNamePath({"__table_", "__first_node_"});
+
+  if (!node_sp) {
+    auto p1_sp = m_backend.GetChildAtNamePath({"__table_", "__p1_"});
+    if (!p1_sp)
+      return {};
+
+    if (!isOldCompressedPairLayout(*p1_sp))
+      return {};
+
+    node_sp = GetFirstValueOfLibCXXCompressedPair(*p1_sp);
+    if (!node_sp)
+      return {};
+  }
+
+  assert(node_sp);
+
+  return node_sp->GetCompilerType().GetTypeTemplateArgument(0).GetPointeeType();
 }
 
 lldb::ValueObjectSP lldb_private::formatters::
@@ -136,36 +165,12 @@ lldb::ValueObjectSP lldb_private::formatters::
     ValueObjectSP hash_sp = node_sp->GetChildMemberWithName("__hash_");
     if (!hash_sp || !value_sp) {
       if (!m_element_type) {
-        auto p1_sp = m_backend.GetChildAtNamePath({"__table_", "__p1_"});
-        if (!p1_sp)
-          return nullptr;
-
-        ValueObjectSP first_sp = GetFirstValueOfLibCXXCompressedPair(*p1_sp);
-        if (!first_sp)
+        m_node_type = GetNodeType();
+        if (!m_node_type)
           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);
-        // This synthetic provider is used for both unordered_(multi)map and
-        // unordered_(multi)set. For unordered_map, the element type has an
-        // additional type layer, an internal struct (`__hash_value_type`)
-        // that wraps a std::pair. Peel away the internal wrapper type - whose
-        // structure is of no value to users, to expose the std::pair. This
-        // matches the structure returned by the std::map synthetic provider.
-        if (isUnorderedMap(m_backend.GetTypeName())) {
-          std::string name;
-          CompilerType field_type = m_element_type.GetFieldAtIndex(
-              0, name, nullptr, nullptr, nullptr);
-          CompilerType actual_type = field_type.GetTypedefedType();
-          if (isStdTemplate(actual_type.GetTypeName(), "pair"))
-            m_element_type = actual_type;
-        }
+        m_element_type = GetElementType(m_node_type);
       }
-      if (!m_node_type)
-        return nullptr;
       node_sp = m_next_element->Cast(m_node_type.GetPointerType())
               ->Dereference(error);
       if (!node_sp || error.Fail())
@@ -217,6 +222,49 @@ lldb::ValueObjectSP lldb_private::formatters::
                           ...
[truncated]

Comment on lines 266 to 267
// TODO: or should this just be: assert
// (!isOldCompressedPairLayout(*node_sp));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, as this could also be triggered by a some change in libc++, missing/corrupted debug info, etc.

Michael137 added a commit that referenced this pull request Jul 16, 2024
This is motivated by the upcoming refactor of libc++'s
`__compressed_pair` in #76756

As this will require changes to numerous LLDB libc++ data-formatters
(see early draft #96538), it
would be nice to have a test-suite that will actually exercise both the
old and new layout. We have a matrix bot that tests old versions of
Clang (but currently those only date back to Clang-15). Having them in
the test-suite will give us quicker signal on what broke.

We have an existing test that exercises various layouts of `std::string`
over time in `TestDataFormatterLibcxxStringSimulator.py`, but that's the
only STL type we have it for. This patch proposes a new
`libcxx-simulators` directory which will take the same approach for all
the STL types that we can feasibly support in this way (as @labath
points out, for some types this might just not be possible due to their
implementation complexity). Nonetheless, it'd be great to have a record
of how the layout of libc++ types changed over time.

Some related discussion:
*
#97568 (comment)
sayhaan pushed a commit to sayhaan/llvm-project that referenced this pull request Jul 16, 2024
)

Summary:
This is motivated by the upcoming refactor of libc++'s
`__compressed_pair` in llvm#76756

As this will require changes to numerous LLDB libc++ data-formatters
(see early draft llvm#96538), it
would be nice to have a test-suite that will actually exercise both the
old and new layout. We have a matrix bot that tests old versions of
Clang (but currently those only date back to Clang-15). Having them in
the test-suite will give us quicker signal on what broke.

We have an existing test that exercises various layouts of `std::string`
over time in `TestDataFormatterLibcxxStringSimulator.py`, but that's the
only STL type we have it for. This patch proposes a new
`libcxx-simulators` directory which will take the same approach for all
the STL types that we can feasibly support in this way (as @labath
points out, for some types this might just not be possible due to their
implementation complexity). Nonetheless, it'd be great to have a record
of how the layout of libc++ types changed over time.

Some related discussion:
*
llvm#97568 (comment)

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D59822424
@Michael137 Michael137 force-pushed the bugfix/lldb-compressed_pair-rework branch from 3c90ce0 to 7b83528 Compare July 18, 2024 14:17
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
This is motivated by the upcoming refactor of libc++'s
`__compressed_pair` in #76756

As this will require changes to numerous LLDB libc++ data-formatters
(see early draft #96538), it
would be nice to have a test-suite that will actually exercise both the
old and new layout. We have a matrix bot that tests old versions of
Clang (but currently those only date back to Clang-15). Having them in
the test-suite will give us quicker signal on what broke.

We have an existing test that exercises various layouts of `std::string`
over time in `TestDataFormatterLibcxxStringSimulator.py`, but that's the
only STL type we have it for. This patch proposes a new
`libcxx-simulators` directory which will take the same approach for all
the STL types that we can feasibly support in this way (as @labath
points out, for some types this might just not be possible due to their
implementation complexity). Nonetheless, it'd be great to have a record
of how the layout of libc++ types changed over time.

Some related discussion:
*
#97568 (comment)
@Michael137 Michael137 force-pushed the bugfix/lldb-compressed_pair-rework branch from 7b83528 to 28fd601 Compare July 28, 2024 19:58
@Michael137 Michael137 force-pushed the bugfix/lldb-compressed_pair-rework branch from 843673f to fe63917 Compare August 9, 2024 00:08
Comment on lines +743 to +748
if has_compressed_pair_layout:
count = self._get_value_of_compressed_pair(
self.valobj.GetChildMemberWithName("__size_")
)
else:
count = size_valobj.GetValueAsUnsigned(0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any verification we can do on this to ensure the data in this object is not just random data because the class hasn't been initialized? We had an issue with std::initializer_list<T> being super simple and if random data was in the object, its size could be huge. Can we verify the alignment of anything like __begin_ or __end_cap_ and if anything look really off or not aligned correctly return zero as the count?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I remember looking into an issue like that for some other formatter. Though this patch doesn't change behaviour in this case. If we had garbage in __size_, we previously would've also read its value and treated it as the count. The only difference here is that we don't unwrap the compressed_pair anymore.

I'll open a follow-up issue for fixing this in a separate PR. In fact, the other issue might still be open somewhere. I'll have a look

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah here's one example: #69614

@Michael137 Michael137 merged commit 9e9b117 into llvm:main Sep 16, 2024
4 of 6 checks passed
@Michael137 Michael137 deleted the bugfix/lldb-compressed_pair-rework branch September 16, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants