[lldb] Make the libcxx unique_ptr prettyprinter support custom deleters.

[jgorbe]

Summary

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.

[jgorbe]

Ping.

[labath]

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

52

We should check that pair.GetChildAtIndex(0, true) returns a valid value before dereferencing it, so we don't crash if the definition of the comppressed_pair type is missing or something like

65

Similarly, here (I'm don't think that GetNumChildren()>1 actually guarantees that the GetChildAtIndex(1) will return a valid object).

731–739

unrelated, but it might be nice to give a better name to the pointed-to value as well.

[jgorbe]

Addressed review comments:

• Check pair values before attempting to call GetChildMemberWithName on them
• Rename first child from __value_ to pointer. This is friendlier and also matches the name used by the libstdcxx unique_ptr prettyprinter.

[aprantl]

Looks like this broke on the Darwin incremental bot:

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/54508/

Can you please take a look and revert in the mean time?

[jgorbe]

Sure, I'll revert. Thanks for notifying me of the problem.

[jgorbe]

Landed again, with a fix for the test that was failing on Darwin, as https://github.com/llvm/llvm-project/commit/d69518b4e52d527b3f8fcc41e90ae21f1f234555. I'll keep an eye on the build bot.

[aprantl]

Thanks for the quick response!

lldb/include/lldb/DataFormatters/FormattersHelpers.h

void AddFilter(TypeCategoryImpl::SharedPointer category_sp,
                std::vector<std::string> children, const char *description,
                llvm::StringRef type_name,
                ScriptedSyntheticChildren::Flags flags, bool regex = false);
 
 size_t ExtractIndexFromString(const char *item_name);
 
 Address GetArrayAddressOrPointerValue(ValueObject &valobj);
 
-lldb::ValueObjectSP GetValueOfLibCXXCompressedPair(ValueObject &pair);
-
 time_t GetOSXEpoch();
 
 struct InferiorSizedWord {
 
   InferiorSizedWord(const InferiorSizedWord &word) : ptr_size(word.ptr_size) {
     if (ptr_size == 4)
       thirty_two = word.thirty_two;
     else

lldb/source/DataFormatters/FormattersHelpers.cpp

if (valobj.IsPointerType())
     data_addr = valobj.GetPointerValue(&type);
   else if (valobj.IsArrayType())
     data_addr = valobj.GetAddressOf(/*scalar_is_load_address=*/true, &type);
   if (data_addr != LLDB_INVALID_ADDRESS && type == eAddressTypeFile)
     return Address(data_addr, valobj.GetModule()->GetSectionList());
 
   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;
-}

lldb/source/Plugins/Language/CPlusPlus/LibCxx.h

 namespace lldb_private {
 namespace formatters {
 
 /// Find a child member of \c obj_sp, trying all alternative names in order.
 lldb::ValueObjectSP
 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
 
 bool LibcxxStringSummaryProviderUTF16(
     ValueObject &valobj, Stream &stream,
     const TypeSummaryOptions &summary_options); // libc++ std::u16string
 
public:
   bool MightHaveChildren() override;
 
   size_t GetIndexOfChildWithName(ConstString name) override;
 
   ~LibcxxUniquePtrSyntheticFrontEnd() override;
 
 private:
   lldb::ValueObjectSP m_value_ptr_sp;
+  lldb::ValueObjectSP m_deleter_sp;
 };
 
 SyntheticChildrenFrontEnd *
 LibcxxBitsetSyntheticFrontEndCreator(CXXSyntheticChildren *,
                                      lldb::ValueObjectSP);
 
 SyntheticChildrenFrontEnd *
 LibcxxSharedPtrSyntheticFrontEndCreator(CXXSyntheticChildren *,

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

for (ConstString name : alternative_names) {
     lldb::ValueObjectSP child_sp = obj.GetChildMemberWithName(name, true);
 
     if (child_sp)
       return child_sp;
   }
   return {};
 }
 
+lldb::ValueObjectSP
+lldb_private::formatters::GetFirstValueOfLibCXXCompressedPair(
+    ValueObject &pair) {
+  ValueObjectSP value;
+  ValueObjectSP first_child = pair.GetChildAtIndex(0, true);
+  if (first_child)
+    value = first_child->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) {
+    ValueObjectSP second_child = pair.GetChildAtIndex(1, true);
+    if (second_child) {
+      value =
+          second_child->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());
   if (!valobj_sp)
     return false;
 
   // An optional either contains a value or not, the member __engaged_ is
   // a bool flag, it is true if the optional has a value and false otherwise.
bool lldb_private::formatters::LibcxxUniquePointerSummaryProvider(
   if (!valobj_sp)
     return false;
 
   ValueObjectSP ptr_sp(
       valobj_sp->GetChildMemberWithName(ConstString("__ptr_"), true));
   if (!ptr_sp)
     return false;
 
-  ptr_sp = GetValueOfLibCXXCompressedPair(*ptr_sp);
+  ptr_sp = GetFirstValueOfLibCXXCompressedPair(*ptr_sp);
   if (!ptr_sp)
     return false;
 
   if (ptr_sp->GetValueAsUnsigned(0) == 0) {
     stream.Printf("nullptr");
     return true;
   } else {
     bool print_pointee = false;

 lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEndCreator(
     CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
   return (valobj_sp ? new LibcxxUniquePtrSyntheticFrontEnd(valobj_sp)
                     : nullptr);
 }
 
 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
 lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEnd::GetChildAtIndex(
     size_t idx) {
   if (!m_value_ptr_sp)
     return lldb::ValueObjectSP();
 
   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()) {
       return value_sp;
     }
   }
 
   return lldb::ValueObjectSP();
 }
 
 bool lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEnd::Update() {
   ValueObjectSP valobj_sp = m_backend.GetSP();
   if (!valobj_sp)
     return false;
 
   ValueObjectSP ptr_sp(
       valobj_sp->GetChildMemberWithName(ConstString("__ptr_"), true));
   if (!ptr_sp)
     return false;
 
-  m_value_ptr_sp = GetValueOfLibCXXCompressedPair(*ptr_sp);
+  // 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"));
+
+  ValueObjectSP deleter_sp = GetSecondValueOfLibCXXCompressedPair(*ptr_sp);
+  if (deleter_sp)
+    m_deleter_sp = deleter_sp->Clone(ConstString("deleter"));
 
   return false;
 }
 
 bool lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEnd::
     MightHaveChildren() {
   return true;
 }
 
 size_t lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEnd::
     GetIndexOfChildWithName(ConstString name) {
-  if (name == "__value_")
+  if (name == "pointer")
     return 0;
-  if (name == "$$dereference$$")
+  if (name == "deleter")
     return 1;
+  if (name == "$$dereference$$")
+    return 2;
   return UINT32_MAX;
 }
 
 bool lldb_private::formatters::LibcxxContainerSummaryProvider(
     ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
   if (valobj.IsPointerType()) {
     uint64_t value = valobj.GetValueAsUnsigned(0);
     if (!value)

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

bool ForwardListFrontEnd::Update() {
   ValueObjectSP backend_addr(m_backend.AddressOf(err));
   if (err.Fail() || !backend_addr)
     return false;
 
   ValueObjectSP impl_sp(
       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();
   return false;
 }
 
 ListFrontEnd::ListFrontEnd(lldb::ValueObjectSP valobj_sp)
     : AbstractListFrontEnd(*valobj_sp) {
   if (valobj_sp)
     Update();
 }
 
 size_t ListFrontEnd::CalculateNumChildren() {
   if (m_count != UINT32_MAX)
     return m_count;
   if (!m_head || !m_tail || m_node_address == 0)
     return 0;
   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);
     }
   }
   if (m_count != UINT32_MAX) {
     return m_count;
   } else {
     uint64_t next_val = m_head->GetValueAsUnsigned(0);

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/unique_ptr/TestDataFormatterLibcxxUniquePtr.py

def test_unique_ptr_variables(self):
         lldbutil.run_to_source_breakpoint(
             self, "// break here", lldb.SBFileSpec("main.cpp")
         )
 
         valobj = self.expect_var_path(
             "up_empty",
             type=self.make_expected_type("int"),
             summary="nullptr",
-            children=[ValueCheck(name="__value_")],
+            children=[ValueCheck(name="pointer")],
         )
         self.assertEqual(
             valobj.child[0].GetValueAsUnsigned(lldb.LLDB_INVALID_ADDRESS), 0
         )
 
         self.expect(
             "frame variable *up_empty", substrs=["(int) *up_empty = <parent is NULL>"]
         )
 
         valobj = self.expect_var_path(
             "up_int",
             type=self.make_expected_type("int"),
             summary="10",
-            children=[ValueCheck(name="__value_")],
+            children=[ValueCheck(name="pointer")],
         )
         self.assertNotEqual(valobj.child[0].unsigned, 0)
 
         valobj = self.expect_var_path(
             "up_int_ref",
             type=self.make_expected_type("int", qualifiers="&"),
             summary="10",
-            children=[ValueCheck(name="__value_")],
+            children=[ValueCheck(name="pointer")],
         )
         self.assertNotEqual(valobj.child[0].unsigned, 0)
 
         valobj = self.expect_var_path(
             "up_int_ref_ref",
             type=self.make_expected_type("int", qualifiers="&&"),
             summary="10",
-            children=[ValueCheck(name="__value_")],
+            children=[ValueCheck(name="pointer")],
         )
         self.assertNotEqual(valobj.child[0].unsigned, 0)
 
         valobj = self.expect_var_path(
             "up_str",
             type=self.make_expected_basic_string_ptr(),
             summary='"hello"',
-            children=[ValueCheck(name="__value_", summary='"hello"')],
+            children=[ValueCheck(name="pointer", summary='"hello"')],
         )
 
         valobj = self.expect_var_path(
             "up_user", type=self.make_expected_type("User")
         )
         self.assertRegex(valobj.summary, "^User @ 0x0*[1-9a-f][0-9a-f]+$")
         self.assertNotEqual(valobj.child[0].unsigned, 0)
 
         valobj = self.expect_var_path(
             "*up_user",
             type="User",
             children=[
                 ValueCheck(name="id", value="30"),
                 ValueCheck(name="name", summary='"steph"'),
             ],
         )
-        self.assertEqual(str(valobj), '(User) *__value_ = (id = 30, name = "steph")')
+        self.assertEqual(str(valobj), '(User) *pointer = (id = 30, name = "steph")')
+
+        valobj = self.expect_var_path(
+            "up_non_empty_deleter",
+            type="std::unique_ptr<int, NonEmptyIntDeleter>",
+            summary="1234",
+            children=[
+                ValueCheck(name="pointer"),
+                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"')

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/unique_ptr/main.cpp

 #include <memory>
 #include <string>
 
 struct User {
   int id = 30;
   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);
   std::unique_ptr<std::string> up_str = std::make_unique<std::string>("hello");
   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
 }