[lldb] Add deref support and tests to shared_ptr synthetic

[kastiglione]

Summary

Add frame variable dereference suppport to libc++ std::shared_ptr.

This change allows for commands like v *thing_sp and v thing_sp->m_id. These commands now work the same way they would with raw pointers, and as they would with expression. This is done by adding an unaccounted for child member named $$dereference$$.

Without this change, the command would have to be written as v *thing_sp.__ptr_ or v thing_sp.__ptr_->m_id which exposes internal structure and is more clumsy to type.

Also, introduces API tests for std::shared_ptr, previously there were none.

[kastiglione]

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

410–433

Note that these two indexes were not accessed because CalculateNumChildren returns either 0 or 1. So this is actually NFC despite its looks.

472–475

Note that count and weak_count were not accessed because CalculateNumChildren returns either 0 or 1. So this is actually NFC despite its looks.

[JDevlieghere]

Looks like a great improvement.

[teemperor]

You can use expect_var_path to test frame var (LLDB calls the frame var input "expressions paths", hence the _path suffix). That way you can test all of these things for you without having to rely on just finding substrs in the output or having to depend on the way LLDB prints the result:

self.expect_var_path("sp_str", summary="\"hello\" strong=1 weak=1",
                     type="std::shared_ptr<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >",
    children = [
        ValueCheck(name="__ptr_", summary="\"hello\"")
    ]
)

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py

12

Libcx typo. FWIW, you don't have to give these classes a unique name anymore.

18

Comment + function name don't really fit to the test

22

You only need lldbutil.run_to_source_breakpoint(self, '// break here', lldb.SBFileSpec("main.cpp")) (the variable assignments and so on aren't necessary)

26

FWIW, this isn't really testing anything. This just tests that the pointer starts with '0x0', but even non-nullptrs start usually with 0x0 as we pad them with zeroes.

In theory you could test that this is equal to 0x0000000000000000 but then we have the test suite depend on the pointer size.

I think the best solution is to have the test suite figure out what a hex nullptr is on the target and then just provide that as utility variable in lldbtest. But that's out of the scope for this test, so I would say you can just leave it out until someone has time to implement that.

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

2

Unused include?

[kastiglione]

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py

12

also copy & paste. What name should I give it, if not unique?

18

ha, this is copypasta from the test for unique_ptr, which also contains this.

22

more copy, thanks

26

I could use a regex for now.

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

2

pasta, thanks

[teemperor]

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py

12

You can just call it TestCase, but you can also keep a unique name. It doesn't really matter. I personally use TestCase because repeating the file name inside a file is redundant, but we used to give all tests unique class names, so feel free to carry on that tradition.

26

That would also work in the meantime.

[kastiglione]

Use expect_var_path and fix other issues with the tests

[kastiglione]

minor fix

[jingham]

This looks great. It's a little weird to use "SBValue.value" to get a string version of something that's a pointer and then have to do regex compares. It's more direct to use "unsigned" and compare against 0x0.

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py

32

If you used "unsigned" instead of "value" you'd get a number back and you could check that directly against 0x0 rather than having to do a regex match. If you want to be careful, use GetValueAsUnsigned(fail_value=lldb.LLDB_INVALID_ADDRESS) since the default fail value is 0...

[kastiglione]

test for null using SBValue.unsigned

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py

32

thanks, this is much nicer, I've fixed up the pointer asserts to check against the unsigned value.

[jingham]

LGTM at this point.

[teemperor]

This LGTM modulo a missing nullptr check. Thanks for fixing this!

Also we probably should investigate the strong= summary differences. I would have blamed the fake constructors we are creating in LLDB, but as this isn't running any expressions this might us just reading the wrong values?

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

406

ptr_sp should have a nullptr check as __ptr_ might be missing because we screw up some debug info parsing, or someone renamed the member or idk. In any case, this shouldn't crash.

[kastiglione]

check ptr_sp before use

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

public:
   bool MightHaveChildren() override;
 
   size_t GetIndexOfChildWithName(ConstString name) override;
 
   ~LibcxxSharedPtrSyntheticFrontEnd() override;
 
 private:
   ValueObject *m_cntrl;
-  lldb::ValueObjectSP m_count_sp;
-  lldb::ValueObjectSP m_weak_count_sp;
-  uint8_t m_ptr_size;
-  lldb::ByteOrder m_byte_order;
 };
 
 class LibcxxUniquePtrSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
 public:
   LibcxxUniquePtrSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp);
 
   size_t CalculateNumChildren() override;
 

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

if (!g_item_name)
     g_item_name.SetCString("__i");
   return (valobj_sp
               ? new VectorIteratorSyntheticFrontEnd(valobj_sp, g_item_name)
               : nullptr);
 }
 
 lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::
     LibcxxSharedPtrSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_cntrl(nullptr), m_count_sp(),
-      m_weak_count_sp(), m_ptr_size(0), m_byte_order(lldb::eByteOrderInvalid) {
+    : SyntheticChildrenFrontEnd(*valobj_sp), m_cntrl(nullptr) {
   if (valobj_sp)
     Update();
 }
 
 size_t lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::
     CalculateNumChildren() {
   return (m_cntrl ? 1 : 0);
 }
 
 lldb::ValueObjectSP
 lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::GetChildAtIndex(
     size_t idx) {
   if (!m_cntrl)
     return lldb::ValueObjectSP();
 
   ValueObjectSP valobj_sp = m_backend.GetSP();
   if (!valobj_sp)
     return lldb::ValueObjectSP();
 
   if (idx == 0)
     return valobj_sp->GetChildMemberWithName(ConstString("__ptr_"), true);
 
-  if (idx > 2)
-    return lldb::ValueObjectSP();
-
   if (idx == 1) {
-    if (!m_count_sp) {
-      ValueObjectSP shared_owners_sp(m_cntrl->GetChildMemberWithName(
-          ConstString("__shared_owners_"), true));
-      if (!shared_owners_sp)
-        return lldb::ValueObjectSP();
-      uint64_t count = 1 + shared_owners_sp->GetValueAsUnsigned(0);
-      DataExtractor data(&count, 8, m_byte_order, m_ptr_size);
-      m_count_sp = CreateValueObjectFromData(
-          "count", data, valobj_sp->GetExecutionContextRef(),
-          shared_owners_sp->GetCompilerType());
-    }
-    return m_count_sp;
-  } else /* if (idx == 2) */
-  {
-    if (!m_weak_count_sp) {
-      ValueObjectSP shared_weak_owners_sp(m_cntrl->GetChildMemberWithName(
-          ConstString("__shared_weak_owners_"), true));
-      if (!shared_weak_owners_sp)
-        return lldb::ValueObjectSP();
-      uint64_t count = 1 + shared_weak_owners_sp->GetValueAsUnsigned(0);
-      DataExtractor data(&count, 8, m_byte_order, m_ptr_size);
-      m_weak_count_sp = CreateValueObjectFromData(
-          "count", data, valobj_sp->GetExecutionContextRef(),
-          shared_weak_owners_sp->GetCompilerType());
+    if (auto ptr_sp =
+            valobj_sp->GetChildMemberWithName(ConstString("__ptr_"), true)) {
+      Status status;
+      auto value_sp = ptr_sp->Dereference(status);
+      if (status.Success()) {
+        auto value_type_sp =
+            valobj_sp->GetCompilerType().GetTypeTemplateArgument(0);
+        return value_sp->Cast(value_type_sp);
       }
-    return m_weak_count_sp;
     }
   }
 
+  return lldb::ValueObjectSP();
+}
+
 bool lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::Update() {
-  m_count_sp.reset();
-  m_weak_count_sp.reset();
   m_cntrl = nullptr;
 
   ValueObjectSP valobj_sp = m_backend.GetSP();
   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();
-
   lldb::ValueObjectSP cntrl_sp(
       valobj_sp->GetChildMemberWithName(ConstString("__cntrl_"), true));
 
   m_cntrl = cntrl_sp.get(); // need to store the raw pointer to avoid a circular
                             // dependency
   return false;
 }
 
 bool lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::
     MightHaveChildren() {
   return true;
 }
 
 size_t lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::
     GetIndexOfChildWithName(ConstString name) {
   if (name == "__ptr_")
     return 0;
-  if (name == "count")
+  if (name == "$$dereference$$")
     return 1;
-  if (name == "weak_count")
-    return 2;
   return UINT32_MAX;
 }
 
 lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::
     ~LibcxxSharedPtrSyntheticFrontEnd() = default;
 
 SyntheticChildrenFrontEnd *
 lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEndCreator(

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/Makefile

+CXX_SOURCES := main.cpp
+
+USE_LIBCPP := 1
+
+CXXFLAGS_EXTRAS := -std=c++14
+include Makefile.rules

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py

+"""
+Test lldb data formatter for libc++ std::shared_ptr.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @add_test_categories(["libc++"])
+    def test_shared_ptr_variables(self):
+        """Test `frame variable` output for `std::shared_ptr` types."""
+        self.build()
+
+        lldbutil.run_to_source_breakpoint(
+            self, "// break here", lldb.SBFileSpec("main.cpp")
+        )
+
+        valobj = self.expect_var_path(
+            "sp_empty",
+            type="std::shared_ptr<int>",
+            summary="nullptr",
+            children=[ValueCheck(name="__ptr_")],
+        )
+        self.assertEqual(
+            valobj.child[0].GetValueAsUnsigned(lldb.LLDB_INVALID_ADDRESS), 0
+        )
+
+        self.expect(
+            "frame variable *sp_empty", substrs=["(int) *sp_empty = <parent is NULL>"]
+        )
+
+        valobj = self.expect_var_path(
+            "sp_int",
+            type="std::shared_ptr<int>",
+            children=[ValueCheck(name="__ptr_")],
+        )
+        self.assertRegex(valobj.summary, r"^10( strong=1)? weak=1$")
+        self.assertNotEqual(valobj.child[0].unsigned, 0)
+
+        valobj = self.expect_var_path(
+            "sp_int_ref",
+            type="std::shared_ptr<int> &",
+            children=[ValueCheck(name="__ptr_")],
+        )
+        self.assertRegex(valobj.summary, r"^10( strong=1)? weak=1$")
+        self.assertNotEqual(valobj.child[0].unsigned, 0)
+
+        valobj = self.expect_var_path(
+            "sp_int_ref_ref",
+            type="std::shared_ptr<int> &&",
+            children=[ValueCheck(name="__ptr_")],
+        )
+        self.assertRegex(valobj.summary, r"^10( strong=1)? weak=1$")
+        self.assertNotEqual(valobj.child[0].unsigned, 0)
+
+        valobj = self.expect_var_path(
+            "sp_str",
+            type="std::shared_ptr<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >",
+            children=[ValueCheck(name="__ptr_", summary='"hello"')],
+        )
+        self.assertRegex(valobj.summary, r'^"hello"( strong=1)? weak=1$')
+
+        valobj = self.expect_var_path("sp_user", type="std::shared_ptr<User>")
+        self.assertRegex(
+            valobj.summary,
+            "^std(::__1)?::shared_ptr<User>::element_type @ 0x0*[1-9a-f][0-9a-f]+( strong=1)? weak=1",
+        )
+        self.assertNotEqual(valobj.child[0].unsigned, 0)
+
+        valobj = self.expect_var_path(
+            "*sp_user",
+            type="User",
+            children=[
+                ValueCheck(name="id", value="30"),
+                ValueCheck(name="name", summary='"steph"'),
+            ],
+        )
+        self.assertEqual(str(valobj), '(User) *__ptr_ = (id = 30, name = "steph")')
+
+        self.expect_var_path("sp_user->id", type="int", value="30")
+        self.expect_var_path("sp_user->name", type="std::string", summary='"steph"')

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

+#include <memory>
+#include <string>
+
+struct User {
+  int id = 30;
+  std::string name = "steph";
+};
+
+int main() {
+  std::shared_ptr<int> sp_empty;
+  std::shared_ptr<int> sp_int = std::make_shared<int>(10);
+  std::shared_ptr<std::string> sp_str = std::make_shared<std::string>("hello");
+  std::shared_ptr<int> &sp_int_ref = sp_int;
+  std::shared_ptr<int> &&sp_int_ref_ref = std::make_shared<int>(10);
+  std::shared_ptr<User> sp_user = std::make_shared<User>();
+
+  return 0; // break here
+}