jgorbe created this revision.
jgorbe added reviewers: shafik, kastiglione.
jgorbe added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
jgorbe requested review of this revision.

The unique_ptr prettyprinter calls `GetValueOfLibCXXCompressedPair`,
which looks for a `__value_` child. However, when the second value in
the compressed pair is not an empty class, there are two `__value_`
children because `__compressed_pair` derives twice from
`__compressed_pair_elem`, one for each member of the pair. And then the
lookup fails because it's ambiguous.

This patch makes the following changes:

- Rename `GetValueOfLibCXXCompressedPair` to 
`GetFirstValueOfLibCXXCompressedPair`, and add a similar function to get the 
second value. Put both functions in Plugin/Language/CPlusPlus/LibCxx.cpp 
because it seems inappropriate to have libcxx-specific helpers separate from 
all the libcxx-dependent code.

- Read the second value of the `__ptr_` pair and display a "deleter" child in 
the unique_ptr synthetic child provider, when available.

- Add a test case for the non-empty deleter case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148662

Files:
  lldb/include/lldb/DataFormatters/FormattersHelpers.h
  lldb/source/DataFormatters/FormattersHelpers.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
  lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/unique_ptr/TestDataFormatterLibcxxUniquePtr.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/unique_ptr/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/unique_ptr/main.cpp
===================================================================
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/unique_ptr/main.cpp
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/unique_ptr/main.cpp
@@ -6,6 +6,15 @@
   std::string name = "steph";
 };
 
+// libc++ stores unique_ptr data in a compressed pair, which has a specialized
+// representation when the type of the second element is an empty class. So
+// we need a deleter class with a dummy data member to trigger the other path.
+struct NonEmptyIntDeleter {
+  void operator()(int* ptr) { delete ptr; }
+
+  int dummy_ = 9999;
+};
+
 int main() {
   std::unique_ptr<int> up_empty;
   std::unique_ptr<int> up_int = std::make_unique<int>(10);
@@ -13,6 +22,8 @@
   std::unique_ptr<int> &up_int_ref = up_int;
   std::unique_ptr<int> &&up_int_ref_ref = std::make_unique<int>(10);
   std::unique_ptr<User> up_user = std::make_unique<User>();
+  auto up_non_empty_deleter =
+      std::unique_ptr<int, NonEmptyIntDeleter>(new int(1234));
 
   return 0; // break here
 }
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/unique_ptr/TestDataFormatterLibcxxUniquePtr.py
===================================================================
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/unique_ptr/TestDataFormatterLibcxxUniquePtr.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/unique_ptr/TestDataFormatterLibcxxUniquePtr.py
@@ -97,5 +97,18 @@
         )
         self.assertEqual(str(valobj), '(User) *__value_ = (id = 30, name = "steph")')
 
+        valobj = self.expect_var_path(
+            "up_non_empty_deleter",
+            type="std::unique_ptr<int, NonEmptyIntDeleter>",
+            summary="1234",
+            children=[
+                ValueCheck(name="__value_"),
+                ValueCheck(name="deleter", children=[
+                    ValueCheck(name="dummy_", value="9999")
+                ]),
+            ],
+        )
+        self.assertNotEqual(valobj.child[0].unsigned, 0)
+
         self.expect_var_path("up_user->id", type="int", value="30")
         self.expect_var_path("up_user->name", type="std::string", summary='"steph"')
Index: lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp
===================================================================
--- lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp
@@ -300,7 +300,7 @@
       m_backend.GetChildMemberWithName(ConstString("__before_begin_"), true));
   if (!impl_sp)
     return false;
-  impl_sp = GetValueOfLibCXXCompressedPair(*impl_sp);
+  impl_sp = GetFirstValueOfLibCXXCompressedPair(*impl_sp);
   if (!impl_sp)
     return false;
   m_head = impl_sp->GetChildMemberWithName(ConstString("__next_"), true).get();
@@ -321,7 +321,7 @@
   ValueObjectSP size_alloc(
       m_backend.GetChildMemberWithName(ConstString("__size_alloc_"), true));
   if (size_alloc) {
-    ValueObjectSP value = GetValueOfLibCXXCompressedPair(*size_alloc);
+    ValueObjectSP value = GetFirstValueOfLibCXXCompressedPair(*size_alloc);
     if (value) {
       m_count = value->GetValueAsUnsigned(UINT32_MAX);
     }
Index: lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
===================================================================
--- lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
@@ -23,6 +23,10 @@
 GetChildMemberWithName(ValueObject &obj,
                        llvm::ArrayRef<ConstString> alternative_names);
 
+lldb::ValueObjectSP GetFirstValueOfLibCXXCompressedPair(ValueObject &pair);
+lldb::ValueObjectSP GetSecondValueOfLibCXXCompressedPair(ValueObject &pair);
+
+
 bool LibcxxStringSummaryProviderASCII(
     ValueObject &valobj, Stream &stream,
     const TypeSummaryOptions &summary_options); // libc++ std::string
@@ -200,6 +204,7 @@
 
 private:
   lldb::ValueObjectSP m_value_ptr_sp;
+  lldb::ValueObjectSP m_deleter_sp;
 };
 
 SyntheticChildrenFrontEnd *
Index: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
===================================================================
--- lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -46,6 +46,32 @@
   return {};
 }
 
+lldb::ValueObjectSP
+lldb_private::formatters::GetFirstValueOfLibCXXCompressedPair(
+    ValueObject &pair) {
+  ValueObjectSP value = pair.GetChildAtIndex(0, true)->GetChildMemberWithName(
+      ConstString("__value_"), true);
+  if (!value) {
+    // pre-r300140 member name
+    value = pair.GetChildMemberWithName(ConstString("__first_"), true);
+  }
+  return value;
+}
+
+lldb::ValueObjectSP
+lldb_private::formatters::GetSecondValueOfLibCXXCompressedPair(
+    ValueObject &pair) {
+  ValueObjectSP value;
+  if (pair.GetNumChildren() > 1)
+    value = pair.GetChildAtIndex(1, true)->GetChildMemberWithName(
+        ConstString("__value_"), true);
+  if (!value) {
+    // pre-r300140 member name
+    value = pair.GetChildMemberWithName(ConstString("__second_"), true);
+  }
+  return value;
+}
+
 bool lldb_private::formatters::LibcxxOptionalSummaryProvider(
     ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
   ValueObjectSP valobj_sp(valobj.GetNonSyntheticValue());
@@ -170,7 +196,7 @@
   if (!ptr_sp)
     return false;
 
-  ptr_sp = GetValueOfLibCXXCompressedPair(*ptr_sp);
+  ptr_sp = GetFirstValueOfLibCXXCompressedPair(*ptr_sp);
   if (!ptr_sp)
     return false;
 
@@ -658,7 +684,9 @@
 
 size_t lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEnd::
     CalculateNumChildren() {
-  return (m_value_ptr_sp ? 1 : 0);
+  if (m_value_ptr_sp)
+    return m_deleter_sp ? 2 : 1;
+  return 0;
 }
 
 lldb::ValueObjectSP
@@ -670,7 +698,10 @@
   if (idx == 0)
     return m_value_ptr_sp;
 
-  if (idx == 1) {
+  if (idx == 1)
+    return m_deleter_sp;
+
+  if (idx == 2) {
     Status status;
     auto value_sp = m_value_ptr_sp->Dereference(status);
     if (status.Success()) {
@@ -691,7 +722,12 @@
   if (!ptr_sp)
     return false;
 
-  m_value_ptr_sp = GetValueOfLibCXXCompressedPair(*ptr_sp);
+  m_value_ptr_sp = GetFirstValueOfLibCXXCompressedPair(*ptr_sp);
+  ValueObjectSP deleter_sp = GetSecondValueOfLibCXXCompressedPair(*ptr_sp);
+  // If there's a deleter, clone its value as "deleter" so we don't have two
+  // children named "__value_".
+  if (deleter_sp)
+    m_deleter_sp = deleter_sp->Clone(ConstString("deleter"));
 
   return false;
 }
@@ -705,8 +741,10 @@
     GetIndexOfChildWithName(ConstString name) {
   if (name == "__value_")
     return 0;
-  if (name == "$$dereference$$")
+  if (name == "deleter")
     return 1;
+  if (name == "$$dereference$$")
+    return 2;
   return UINT32_MAX;
 }
 
Index: lldb/source/DataFormatters/FormattersHelpers.cpp
===================================================================
--- lldb/source/DataFormatters/FormattersHelpers.cpp
+++ lldb/source/DataFormatters/FormattersHelpers.cpp
@@ -126,14 +126,3 @@
 
   return data_addr;
 }
-
-lldb::ValueObjectSP
-lldb_private::formatters::GetValueOfLibCXXCompressedPair(ValueObject &pair) {
-  ValueObjectSP value =
-      pair.GetChildMemberWithName(ConstString("__value_"), true);
-  if (!value) {
-    // pre-r300140 member name
-    value = pair.GetChildMemberWithName(ConstString("__first_"), true);
-  }
-  return value;
-}
Index: lldb/include/lldb/DataFormatters/FormattersHelpers.h
===================================================================
--- lldb/include/lldb/DataFormatters/FormattersHelpers.h
+++ lldb/include/lldb/DataFormatters/FormattersHelpers.h
@@ -58,8 +58,6 @@
 
 Address GetArrayAddressOrPointerValue(ValueObject &valobj);
 
-lldb::ValueObjectSP GetValueOfLibCXXCompressedPair(ValueObject &pair);
-
 time_t GetOSXEpoch();
 
 struct InferiorSizedWord {
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to