Improve the libstdc++ smart pointer formatters

[tberghammer]

Summary

Improve the libstdc++ smart pointer formatters

• Display the strong/weak count in the summary
• Display the pointed object as a synthetic member
• Create synthetic children for weak/strong count

[tberghammer]

Move the code to a new cpp file

[Eugene.Zelenko]

Please run Clang-format over new code.

source/Plugins/Language/CPlusPlus/LibStdcppSmartPointer.cpp

13

STL headers should be after application's headers.

29

No space before access specifiers. Same below.

[tberghammer]

In D25726#573127, @Eugene.Zelenko wrote:

Please run Clang-format over new code.

I run it before upload but it mush have picked up some strange config. Will run it again before submit. Thanks for noticing it.

[labath]

Looks good from my side. Enrico, do you want to have a look at this?

A couple tiny comments below. Also, be sure to properly reformat and reorder the headers.

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libstdcpp/smart_ptr/TestDataFormatterStdSmartPtr.py

40

Could you add a test that accesses a non-existing subobject of ssp, to make sure it does something not-completely-broken.

source/Plugins/Language/CPlusPlus/LibStdcppSmartPointer.cpp

28

What do you think about renaming this class to something shorter (SharedPtrFrontEnd, SharedPtrSynthetic, simply FrontEnd ?). Since it's local to a cpp, I don't think we need to be so explicit, and it seems wasteful to use half of the line length for the class name.

90

It would be useful to add a short comment explaining the -1

139

I know you just copied it, but this seems wrong (size_t can be 64-bit). Will this work if you just return ~0.

[granata.enrico]

I see you already got a bunch of feedback on specific items. The overall idea looks good to me. I'll try to delve a little deeper in the code ASAP (I was out for a couple days and have some backlog...), but should be good to go assuming you address the feedback you already got + make sure tests work!

[tberghammer]

source/Plugins/Language/CPlusPlus/LibStdcppSmartPointer.cpp

139

Actually as far as I can tell all use case of this API assumes it is a uint32_t and compares the return value against UINT32_MAX :(
I will just leave it as it is and then we should have a separate CL what cleans up the mess around it

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libstdcpp/smart_ptr/TestDataFormatterStdSmartPtr.py

def test_with_run_command(self):
             self, "Set break point at this line.")
         self.runCmd("run", RUN_SUCCEEDED)
 
         # The stop reason of the thread should be breakpoint.
         self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
                     substrs=['stopped', 'stop reason = breakpoint'])
 
         self.expect("frame variable nsp", substrs=['nsp = nullptr'])
-        self.expect("frame variable isp", substrs=['isp = 123'])
-        self.expect("frame variable ssp", substrs=['ssp = "foobar"'])
-
+        self.expect("frame variable isp", substrs=['isp = 123', 'strong=1', 'weak=1'])
+        self.expect("frame variable ssp", substrs=['ssp = "foobar"', 'strong=1', 'weak=1'])
         self.expect("frame variable nwp", substrs=['nwp = nullptr'])
-        self.expect("frame variable iwp", substrs=['iwp = 123'])
-        self.expect("frame variable swp", substrs=['swp = "foobar"'])
+        self.expect("frame variable iwp", substrs=['iwp = 123', 'strong=1', 'weak=1'])
+        self.expect("frame variable swp", substrs=['swp = "foobar"', 'strong=1', 'weak=1'])
+        
+        frame = self.frame()
+        self.assertTrue(frame.IsValid())
+
+        self.assertEqual(0, frame.GetValueForVariablePath("nsp.pointer").GetValueAsUnsigned())
+        self.assertEqual(0, frame.GetValueForVariablePath("nwp.pointer").GetValueAsUnsigned())
+
+        self.assertNotEqual(0, frame.GetValueForVariablePath("isp.pointer").GetValueAsUnsigned())
+        self.assertEqual(123, frame.GetValueForVariablePath("isp.object").GetValueAsUnsigned())
+        self.assertEqual(1, frame.GetValueForVariablePath("isp.count").GetValueAsUnsigned())
+        self.assertEqual(1, frame.GetValueForVariablePath("isp.weak_count").GetValueAsUnsigned())
+        self.assertFalse(frame.GetValueForVariablePath("isp.foobar").IsValid())
+
+        self.assertNotEqual(0, frame.GetValueForVariablePath("ssp.pointer").GetValueAsUnsigned())
+        self.assertEqual('"foobar"', frame.GetValueForVariablePath("ssp.object").GetSummary())
+        self.assertEqual(1, frame.GetValueForVariablePath("ssp.count").GetValueAsUnsigned())
+        self.assertEqual(1, frame.GetValueForVariablePath("ssp.weak_count").GetValueAsUnsigned())
+        self.assertFalse(frame.GetValueForVariablePath("ssp.foobar").IsValid())
+        
+        self.assertNotEqual(0, frame.GetValueForVariablePath("iwp.pointer").GetValueAsUnsigned())
+        self.assertEqual(123, frame.GetValueForVariablePath("iwp.object").GetValueAsUnsigned())
+        self.assertEqual(1, frame.GetValueForVariablePath("iwp.count").GetValueAsUnsigned())
+        self.assertEqual(1, frame.GetValueForVariablePath("iwp.weak_count").GetValueAsUnsigned())
+        self.assertFalse(frame.GetValueForVariablePath("iwp.foobar").IsValid())
+
+        self.assertNotEqual(0, frame.GetValueForVariablePath("swp.pointer").GetValueAsUnsigned())
+        self.assertEqual('"foobar"', frame.GetValueForVariablePath("swp.object").GetSummary())
+        self.assertEqual(1, frame.GetValueForVariablePath("swp.count").GetValueAsUnsigned())
+        self.assertEqual(1, frame.GetValueForVariablePath("swp.weak_count").GetValueAsUnsigned())
+        self.assertFalse(frame.GetValueForVariablePath("swp.foobar").IsValid())
 
         self.runCmd("continue")
 
+        frame = self.frame()
+        self.assertTrue(frame.IsValid())
+
         self.expect("frame variable nsp", substrs=['nsp = nullptr'])
         self.expect("frame variable isp", substrs=['isp = nullptr'])
         self.expect("frame variable ssp", substrs=['ssp = nullptr'])
-
         self.expect("frame variable nwp", substrs=['nwp = nullptr'])
-        self.expect("frame variable iwp", substrs=['iwp = nullptr'])
-        self.expect("frame variable swp", substrs=['swp = nullptr'])
+        self.expect("frame variable iwp", substrs=['iwp = nullptr', 'strong=0', 'weak=1'])
+        self.expect("frame variable swp", substrs=['swp = nullptr', 'strong=0', 'weak=1'])
+
+        self.assertFalse(frame.GetValueForVariablePath("iwp.object").IsValid())
+        self.assertEqual(0, frame.GetValueForVariablePath("iwp.count").GetValueAsUnsigned())
+        self.assertEqual(1, frame.GetValueForVariablePath("iwp.weak_count").GetValueAsUnsigned())
+
+        self.assertFalse(frame.GetValueForVariablePath("swp.object").IsValid())
+        self.assertEqual(0, frame.GetValueForVariablePath("swp.count").GetValueAsUnsigned())
+        self.assertEqual(1, frame.GetValueForVariablePath("swp.weak_count").GetValueAsUnsigned())

source/Plugins/Language/CPlusPlus/CMakeLists.txt

 add_lldb_library(lldbPluginCPlusPlusLanguage
   BlockPointer.cpp
   CPlusPlusLanguage.cpp
   CxxStringTypes.cpp
   LibCxx.cpp
   LibCxxAtomic.cpp
   LibCxxInitializerList.cpp
   LibCxxList.cpp
   LibCxxMap.cpp
   LibCxxUnorderedMap.cpp
   LibCxxVector.cpp
   LibStdcpp.cpp
+  LibStdcppSmartPointer.cpp
 )

source/Plugins/Language/CPlusPlus/LibStdcpp.cpp

 
 private:
   ExecutionContextRef m_exe_ctx_ref;
   lldb::addr_t m_pair_address;
   CompilerType m_pair_type;
   lldb::ValueObjectSP m_pair_sp;
 };
 
-class LibStdcppSharedPtrSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
-public:
-  explicit LibStdcppSharedPtrSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp);
-
-  size_t CalculateNumChildren() override;
-
-  lldb::ValueObjectSP GetChildAtIndex(size_t idx) override;
-
-  bool Update() override;
-
-  bool MightHaveChildren() override;
-
-  size_t GetIndexOfChildWithName(const ConstString &name) override;
-};
-
 } // end of anonymous namespace
 
 LibstdcppMapIteratorSyntheticFrontEnd::LibstdcppMapIteratorSyntheticFrontEnd(
     lldb::ValueObjectSP valobj_sp)
     : SyntheticChildrenFrontEnd(*valobj_sp), m_exe_ctx_ref(), m_pair_address(0),
       m_pair_type(), m_pair_sp() {
   if (valobj_sp)
     Update();
case eAddressTypeHost:
       break;
     case eAddressTypeInvalid:
     case eAddressTypeFile:
       break;
     }
   }
   return false;
 }
-
-LibStdcppSharedPtrSyntheticFrontEnd::LibStdcppSharedPtrSyntheticFrontEnd(
-    lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp) {
-  if (valobj_sp)
-    Update();
-}
-
-size_t LibStdcppSharedPtrSyntheticFrontEnd::CalculateNumChildren() { return 1; }
-
-lldb::ValueObjectSP
-LibStdcppSharedPtrSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
-  ValueObjectSP valobj_sp = m_backend.GetSP();
-  if (!valobj_sp)
-    return lldb::ValueObjectSP();
-
-  if (idx == 0)
-    return valobj_sp->GetChildMemberWithName(ConstString("_M_ptr"), true);
-  else
-    return lldb::ValueObjectSP();
-}
-
-bool LibStdcppSharedPtrSyntheticFrontEnd::Update() { return false; }
-
-bool LibStdcppSharedPtrSyntheticFrontEnd::MightHaveChildren() { return true; }
-
-size_t LibStdcppSharedPtrSyntheticFrontEnd::GetIndexOfChildWithName(
-    const ConstString &name) {
-  if (name == ConstString("_M_ptr"))
-    return 0;
-  return UINT32_MAX;
-}
-
-SyntheticChildrenFrontEnd *
-lldb_private::formatters::LibStdcppSharedPtrSyntheticFrontEndCreator(
-    CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
-  return (valobj_sp ? new LibStdcppSharedPtrSyntheticFrontEnd(valobj_sp)
-                    : nullptr);
-}
-
-bool lldb_private::formatters::LibStdcppSmartPointerSummaryProvider(
-    ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
-  ValueObjectSP valobj_sp(valobj.GetNonSyntheticValue());
-  if (!valobj_sp)
-    return false;
-
-  ValueObjectSP ptr_sp(
-      valobj_sp->GetChildMemberWithName(ConstString("_M_ptr"), true));
-  if (!ptr_sp)
-    return false;
-
-  ValueObjectSP usecount_sp(valobj_sp->GetChildAtNamePath(
-      {ConstString("_M_refcount"), ConstString("_M_pi"),
-       ConstString("_M_use_count")}));
-  if (!usecount_sp)
-    return false;
-
-  if (ptr_sp->GetValueAsUnsigned(0) == 0 ||
-      usecount_sp->GetValueAsUnsigned(0) == 0) {
-    stream.Printf("nullptr");
-    return true;
-  }
-
-  Error error;
-  ValueObjectSP pointee_sp = ptr_sp->Dereference(error);
-  if (pointee_sp && error.Success()) {
-    if (pointee_sp->DumpPrintableRepresentation(
-            stream, ValueObject::eValueObjectRepresentationStyleSummary,
-            lldb::eFormatInvalid,
-            ValueObject::ePrintableRepresentationSpecialCasesDisable, false)) {
-      return true;
-    }
-  }
-
-  stream.Printf("ptr = 0x%" PRIx64, ptr_sp->GetValueAsUnsigned(0));
-  return true;
-}

source/Plugins/Language/CPlusPlus/LibStdcppSmartPointer.cpp

+//===-- LibStdcppSmartPointer.cpp -------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "LibStdcpp.h"
+
+#include "lldb/Core/ConstString.h"
+#include "lldb/Core/ValueObject.h"
+#include "lldb/DataFormatters/FormattersHelpers.h"
+#include "lldb/DataFormatters/TypeSynthetic.h"
+#include "lldb/Target/Target.h"
+
+#include <memory>
+#include <vector>
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace lldb_private::formatters;
+
+namespace {
+
+class SharedPtrFrontEnd : public SyntheticChildrenFrontEnd {
+public:
+  explicit SharedPtrFrontEnd(lldb::ValueObjectSP valobj_sp);
+
+  size_t CalculateNumChildren() override;
+
+  lldb::ValueObjectSP GetChildAtIndex(size_t idx) override;
+
+  bool Update() override;
+
+  bool MightHaveChildren() override;
+
+  size_t GetIndexOfChildWithName(const ConstString &name) override;
+
+  bool GetSummary(Stream &stream, const TypeSummaryOptions &options);
+
+private:
+  ValueObjectSP m_ptr_obj;
+  ValueObjectSP m_obj_obj;
+  ValueObjectSP m_use_obj;
+  ValueObjectSP m_weak_obj;
+
+  uint8_t m_ptr_size = 0;
+  lldb::ByteOrder m_byte_order = lldb::eByteOrderInvalid;
+
+  bool IsEmpty();
+  bool IsValid();
+};
+
+} // end of anonymous namespace
+
+SharedPtrFrontEnd::SharedPtrFrontEnd(lldb::ValueObjectSP valobj_sp)
+    : SyntheticChildrenFrontEnd(*valobj_sp) {
+  Update();
+}
+
+bool SharedPtrFrontEnd::Update() {
+  ValueObjectSP valobj_backend_sp = m_backend.GetSP();
+  if (!valobj_backend_sp)
+    return false;
+
+  ValueObjectSP valobj_sp = valobj_backend_sp->GetNonSyntheticValue();
+  if (!valobj_sp)
+    return false;
+
+  TargetSP target_sp(valobj_sp->GetTargetSP());
+  if (!target_sp)
+    return false;
+
+  m_byte_order = target_sp->GetArchitecture().GetByteOrder();
+  m_ptr_size = target_sp->GetArchitecture().GetAddressByteSize();
+
+  m_ptr_obj = valobj_sp->GetChildMemberWithName(ConstString("_M_ptr"), true);
+
+  m_use_obj = valobj_sp->GetChildAtNamePath({ConstString("_M_refcount"),
+                                             ConstString("_M_pi"),
+                                             ConstString("_M_use_count")});
+
+  m_weak_obj = valobj_sp->GetChildAtNamePath({ConstString("_M_refcount"),
+                                              ConstString("_M_pi"),
+                                              ConstString("_M_weak_count")});
+
+  // libstdc++ implements the weak usage count in a way that it is offset by 1
+  // if the strong count is not 0 (as part of a preformance optimization). We
+  // want to undo this before showing the weak count to the user as an offseted
+  // weak count would be very confusing.
+  if (m_use_obj && m_weak_obj && m_use_obj->GetValueAsUnsigned(0) > 0) {
+    bool success = false;
+    uint64_t count = m_weak_obj->GetValueAsUnsigned(0, &success) - 1;
+    if (success) {
+      auto data = std::make_shared<DataBufferHeap>(&count, sizeof(count));
+      m_weak_obj = CreateValueObjectFromData(
+          "weak_count", DataExtractor(data, m_byte_order, m_ptr_size),
+          m_weak_obj->GetExecutionContextRef(), m_weak_obj->GetCompilerType());
+    }
+  }
+
+  if (m_ptr_obj && !IsEmpty()) {
+    Error error;
+    m_obj_obj = m_ptr_obj->Dereference(error);
+    if (error.Success()) {
+      m_obj_obj->SetName(ConstString("object"));
+    }
+  }
+
+  return false;
+}
+
+bool SharedPtrFrontEnd::MightHaveChildren() { return true; }
+
+lldb::ValueObjectSP SharedPtrFrontEnd::GetChildAtIndex(size_t idx) {
+  if (idx == 0)
+    return m_obj_obj;
+  if (idx == 1)
+    return m_ptr_obj;
+  if (idx == 2)
+    return m_use_obj;
+  if (idx == 3)
+    return m_weak_obj;
+  return lldb::ValueObjectSP();
+}
+
+size_t SharedPtrFrontEnd::CalculateNumChildren() {
+  if (IsEmpty())
+    return 0;
+  return 1;
+}
+
+size_t SharedPtrFrontEnd::GetIndexOfChildWithName(const ConstString &name) {
+  if (name == ConstString("obj") || name == ConstString("object"))
+    return 0;
+  if (name == ConstString("ptr") || name == ConstString("pointer") ||
+      name == ConstString("_M_ptr"))
+    return 1;
+  if (name == ConstString("cnt") || name == ConstString("count") ||
+      name == ConstString("use_count") || name == ConstString("strong") ||
+      name == ConstString("_M_use_count"))
+    return 2;
+  if (name == ConstString("weak") || name == ConstString("weak_count") ||
+      name == ConstString("_M_weak_count"))
+    return 3;
+  return UINT32_MAX;
+}
+
+bool SharedPtrFrontEnd::GetSummary(Stream &stream,
+                                   const TypeSummaryOptions &options) {
+  if (!IsValid())
+    return false;
+
+  if (IsEmpty()) {
+    stream.Printf("nullptr");
+  } else {
+    Error error;
+    bool print_pointee = false;
+    if (m_obj_obj) {
+      if (m_obj_obj->DumpPrintableRepresentation(
+              stream, ValueObject::eValueObjectRepresentationStyleSummary,
+              lldb::eFormatInvalid,
+              ValueObject::ePrintableRepresentationSpecialCasesDisable,
+              false)) {
+        print_pointee = true;
+      }
+    }
+    if (!print_pointee)
+      stream.Printf("ptr = 0x%" PRIx64, m_ptr_obj->GetValueAsUnsigned(0));
+  }
+
+  if (m_use_obj && m_use_obj->GetError().Success())
+    stream.Printf(" strong=%" PRIu64, m_use_obj->GetValueAsUnsigned(0));
+
+  if (m_weak_obj && m_weak_obj->GetError().Success())
+    stream.Printf(" weak=%" PRIu64, m_weak_obj->GetValueAsUnsigned(0));
+
+  return true;
+}
+
+bool SharedPtrFrontEnd::IsValid() { return m_ptr_obj != nullptr; }
+
+bool SharedPtrFrontEnd::IsEmpty() {
+  return !IsValid() || m_ptr_obj->GetValueAsUnsigned(0) == 0 ||
+         (m_use_obj && m_use_obj->GetValueAsUnsigned(0) == 0);
+}
+
+SyntheticChildrenFrontEnd *
+lldb_private::formatters::LibStdcppSharedPtrSyntheticFrontEndCreator(
+    CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
+  return valobj_sp ? new SharedPtrFrontEnd(valobj_sp) : nullptr;
+}
+
+bool lldb_private::formatters::LibStdcppSmartPointerSummaryProvider(
+    ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
+  SharedPtrFrontEnd formatter(valobj.GetSP());
+  return formatter.GetSummary(stream, options);
+}