Add data formatter for libc++'s forward_list

[labath]

Summary

This adds a data formatter for the implementation of forward_list in
libc++. I've refactored the existing std::list data formatter a bit to
enable more sharing of code (mainly the loop detection stuff).

[labath]

Hey Jim,

I wrote a couple of new libc++ data formatters in july. You seem to be the data formatter owner nowadays. Would you mind taking a look?

[jingham]

Will do. I was sick all last week so I have a bit of a backlog, I'll get to this as soon as I can.

[jingham]

It looks like the behavior of the two derived list classes here are only partially factored out. See the individual comments but it looks like there is a lot more that could be shared.

I also don't think we should change the naming conventions used for these classes piecemeal. If we want to simplify the names and hide the FrontEnds in the source files we should do that wholesale, and not one by one. Otherwise next time I read this file I'm going to waste time wondering why the ForwardListFrontEnd isn't specific to LibCxx whereas the LibcxxStdListSyntheticFrontEnd does seem to be...

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/forward_list/main.cpp

5 ↗

(On Diff #107084)

Some compilers (including clang at other times) will omit debug info for variables that it doesn't see getting used. I usually try to use the variables I'm going to print (call size on them, pass an element to printf, whatever...) to keep this from happening.

OTOH, it's nice if compilers don't do that, so maybe relying on their not doing that in our tests is a useful forcing function???

source/Plugins/Language/CPlusPlus/LibCxxList.cpp

141 ↗

(On Diff #107084)

I wonder about the decision to move these classes out of lldb_private::formatters. They don't need to be in a globally visible namespace, but all the others are. They also all have a consistent naming convention, which this one doesn't have. This doesn't seem like the sort of thing we should change piecemeal, that will just lead to confusion.

Was there some other reason for doing this that I'm missing?

153 ↗

(On Diff #107084)

Why are these three ivars not in the AbstractListFrontEnd? They appear in both derived classes and seem to be used in the same way.

182 ↗

(On Diff #107084)

The AbstractFrontEndList has the m_fast_runner and m_slow_runner but they are only reset in the Update for the LibcxxStdListSyntheticFrontEnd, and not here. Is that on purpose?

251–260 ↗

(On Diff #107084)

This use of the iterators also seems like it should be shared.

299–301 ↗

(On Diff #107084)

?

303–310 ↗

(On Diff #107084)

This bit is done in exactly the same way in the two sub-classes. Could it be moved into the base class Update?

[labath]

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/forward_list/main.cpp

5 ↗

(On Diff #107084)

Hmm... I'd say we can leave it like this. I'd be surprised if the compiler can figure out that these declarations are a no-op without very aggressive inlining (which it just won't do at -O0).

source/Plugins/Language/CPlusPlus/LibCxxList.cpp

141 ↗

(On Diff #107084)

There's no hard reason, but I was trying to make these names more concise

I've put them in the anonymous namespace, as llvm coding style encourages that for file-local entities https://llvm.org/docs/CodingStandards.html#anonymous-namespaces. I'd be happy to move the other formatters into the anonymous namespace as well, as I have a couple of other formatters in the pipeline, which already are in the anon namespace.

As for the names, if I followed the existing style, this would read something like

class LibCxxStdForwardListSyntheticFrontEnd: public LibCxxAbstractListSyntheticFrontEnd

which would be formatted horribly, as it does not fit a single line. As these are file-local entities, I think we can be a bit less verbose.

I had not renamed the std::list class because of the way I wrote this change (make a copy of the std::list formatter, and then factor out the common code), but you are right that they should follow the same convention. I propose to shorten the name of the other class as well.

153 ↗

(On Diff #107084)

Good question. I started with two completely distinct implementations and then worked towards merging them. I guess did not finish the job.

182 ↗

(On Diff #107084)

It doesn't matter since they are re-initialized in HasLoop(), but it does look weird. Fixed it.

251–260 ↗

(On Diff #107084)

Indeed it can.

299–301 ↗

(On Diff #107084)

Oops.

[labath]

Address review comments.

[jingham]

Great, this is much nicer.

source/Plugins/Language/CPlusPlus/LibCxxList.cpp

141 ↗

(On Diff #107084)

The effect on choosing names is one of the many reasons why I don't favor the llvm 80 character restriction. But we're stuck with it so...

[labath]

Thanks.

By the way, I have a couple of other patches lined up that you probably did not notice:
https://reviews.llvm.org/D35666
https://reviews.llvm.org/D35615
https://reviews.llvm.org/D35305

lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/forward_list/Makefile

+LEVEL = ../../../../../make
+
+CXX_SOURCES := main.cpp
+
+USE_LIBCPP := 1
+include $(LEVEL)/Makefile.rules

lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/forward_list/TestDataFormatterLibcxxForwardList.py

+"""
+Test lldb data formatter subsystem.
+"""
+
+from __future__ import print_function
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestDataFormatterLibcxxForwardList(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        TestBase.setUp(self)
+        self.line = line_number('main.cpp', '// break here')
+        ns = 'ndk' if lldbplatformutil.target_is_android() else ''
+        self.namespace = 'std::__' + ns + '1'
+
+    @add_test_categories(["libc++"])
+    def test(self):
+        """Test that std::forward_list is displayed correctly"""
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, '// break here',
+                lldb.SBFileSpec("main.cpp", False))
+
+        forward_list = self.namespace + '::forward_list'
+        self.expect("frame variable empty",
+                    substrs=[forward_list,
+                             'size=0',
+                             '{}'])
+
+        self.expect("frame variable one_elt",
+                    substrs=[forward_list,
+                             'size=1',
+                             '{',
+                             '[0] = 47',
+                             '}'])
+
+        self.expect("frame variable five_elts",
+                    substrs=[forward_list,
+                             'size=5',
+                             '{',
+                             '[0] = 1',
+                             '[1] = 22',
+                             '[2] = 333',
+                             '[3] = 4444',
+                             '[4] = 55555',
+                             '}'])

lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/forward_list/main.cpp

+#include <forward_list>
+
+int main()
+{
+  std::forward_list<int> empty{}, one_elt{47}, five_elts{1,22,333,4444,55555};
+  return 0; // break here
+}

lldb/trunk/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

#ifndef LLDB_DISABLE_PYTHON
   AddCXXSynthetic(
       cpp_category_sp,
       lldb_private::formatters::LibcxxStdVectorSyntheticFrontEndCreator,
       "libc++ std::vector synthetic children",
       ConstString("^std::__(ndk)?1::vector<.+>(( )?&)?$"), stl_synth_flags,
       true);
   AddCXXSynthetic(
       cpp_category_sp,
+      lldb_private::formatters::LibcxxStdForwardListSyntheticFrontEndCreator,
+      "libc++ std::forward_list synthetic children",
+      ConstString("^std::__(ndk)?1::forward_list<.+>(( )?&)?$"),
+      stl_synth_flags, true);
+  AddCXXSynthetic(
+      cpp_category_sp,
       lldb_private::formatters::LibcxxStdListSyntheticFrontEndCreator,
       "libc++ std::list synthetic children",
       ConstString("^std::__(ndk)?1::list<.+>(( )?&)?$"), stl_synth_flags, true);
   AddCXXSynthetic(
       cpp_category_sp,
       lldb_private::formatters::LibcxxStdMapSyntheticFrontEndCreator,
       "libc++ std::map synthetic children",
       ConstString("^std::__(ndk)?1::map<.+> >(( )?&)?$"), stl_synth_flags,
#ifndef LLDB_DISABLE_PYTHON
   AddCXXSummary(cpp_category_sp,
                 lldb_private::formatters::LibcxxContainerSummaryProvider,
                 "libc++ std::vector summary provider",
                 ConstString("^std::__(ndk)?1::vector<.+>(( )?&)?$"),
                 stl_summary_flags, true);
   AddCXXSummary(cpp_category_sp,
                 lldb_private::formatters::LibcxxContainerSummaryProvider,
                 "libc++ std::list summary provider",
+                ConstString("^std::__(ndk)?1::forward_list<.+>(( )?&)?$"),
+                stl_summary_flags, true);
+  AddCXXSummary(cpp_category_sp,
+                lldb_private::formatters::LibcxxContainerSummaryProvider,
+                "libc++ std::list summary provider",
                 ConstString("^std::__(ndk)?1::list<.+>(( )?&)?$"),
                 stl_summary_flags, true);
   AddCXXSummary(cpp_category_sp,
                 lldb_private::formatters::LibcxxContainerSummaryProvider,
                 "libc++ std::map summary provider",
                 ConstString("^std::__(ndk)?1::map<.+>(( )?&)?$"),
                 stl_summary_flags, true);
   AddCXXSummary(cpp_category_sp,

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

 LibcxxStdVectorSyntheticFrontEndCreator(CXXSyntheticChildren *,
                                         lldb::ValueObjectSP);
 
 SyntheticChildrenFrontEnd *
 LibcxxStdListSyntheticFrontEndCreator(CXXSyntheticChildren *,
                                       lldb::ValueObjectSP);
 
 SyntheticChildrenFrontEnd *
+LibcxxStdForwardListSyntheticFrontEndCreator(CXXSyntheticChildren *,
+                                             lldb::ValueObjectSP);
+
+SyntheticChildrenFrontEnd *
 LibcxxStdMapSyntheticFrontEndCreator(CXXSyntheticChildren *,
                                      lldb::ValueObjectSP);
 
 SyntheticChildrenFrontEnd *
 LibcxxStdUnorderedMapSyntheticFrontEndCreator(CXXSyntheticChildren *,
                                               lldb::ValueObjectSP);
 
 SyntheticChildrenFrontEnd *

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

protected:
   void next() { m_entry = m_entry.next(); }
 
   void prev() { m_entry = m_entry.prev(); }
 
 private:
   ListEntry m_entry;
 };
 
-} // end anonymous namespace
-
-namespace lldb_private {
-namespace formatters {
-class LibcxxStdListSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
+class AbstractListFrontEnd : public SyntheticChildrenFrontEnd {
 public:
-  LibcxxStdListSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp);
+  size_t GetIndexOfChildWithName(const ConstString &name) override {
+    return ExtractIndexFromString(name.GetCString());
+  }
+  bool MightHaveChildren() override { return true; }
+  bool Update() override;
 
-  ~LibcxxStdListSyntheticFrontEnd() override = default;
+protected:
+  AbstractListFrontEnd(ValueObject &valobj)
+      : SyntheticChildrenFrontEnd(valobj) {}
 
-  size_t CalculateNumChildren() override;
+  size_t m_count;
+  ValueObject *m_head;
 
-  lldb::ValueObjectSP GetChildAtIndex(size_t idx) override;
+  static constexpr bool g_use_loop_detect = true;
+  size_t m_loop_detected; // The number of elements that have had loop detection
+                          // run over them.
+  ListEntry m_slow_runner; // Used for loop detection
+  ListEntry m_fast_runner; // Used for loop detection
+
+  size_t m_list_capping_size;
+  CompilerType m_element_type;
+  std::map<size_t, ListIterator> m_iterators;
+
+  bool HasLoop(size_t count);
+  ValueObjectSP GetItem(size_t idx);
+};
 
+class ForwardListFrontEnd : public AbstractListFrontEnd {
+public:
+  ForwardListFrontEnd(ValueObject &valobj);
+
+  size_t CalculateNumChildren() override;
+  ValueObjectSP GetChildAtIndex(size_t idx) override;
   bool Update() override;
+};
 
-  bool MightHaveChildren() override;
+class ListFrontEnd : public AbstractListFrontEnd {
+public:
+  ListFrontEnd(lldb::ValueObjectSP valobj_sp);
 
-  size_t GetIndexOfChildWithName(const ConstString &name) override;
+  ~ListFrontEnd() override = default;
 
-private:
-  bool HasLoop(size_t count);
+  size_t CalculateNumChildren() override;
 
-  size_t m_list_capping_size;
-  static const bool g_use_loop_detect = true;
+  lldb::ValueObjectSP GetChildAtIndex(size_t idx) override;
 
-  size_t m_loop_detected; // The number of elements that have had loop detection
-                          // run over them.
-  ListEntry m_slow_runner; // Used for loop detection
-  ListEntry m_fast_runner; // Used for loop detection
+  bool Update() override;
 
+private:
   lldb::addr_t m_node_address;
-  ValueObject *m_head;
   ValueObject *m_tail;
-  CompilerType m_element_type;
-  size_t m_count;
-  std::map<size_t, ListIterator> m_iterators;
 };
-} // namespace formatters
-} // namespace lldb_private
 
-lldb_private::formatters::LibcxxStdListSyntheticFrontEnd::
-    LibcxxStdListSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_list_capping_size(0),
-      m_loop_detected(0), m_node_address(), m_head(nullptr), m_tail(nullptr),
-      m_element_type(), m_count(UINT32_MAX), m_iterators() {
-  if (valobj_sp)
-    Update();
+} // end anonymous namespace
+
+bool AbstractListFrontEnd::Update() {
+  m_loop_detected = 0;
+  m_count = UINT32_MAX;
+  m_head = nullptr;
+  m_list_capping_size = 0;
+  m_slow_runner.SetEntry(nullptr);
+  m_fast_runner.SetEntry(nullptr);
+  m_iterators.clear();
+
+  if (m_backend.GetTargetSP())
+    m_list_capping_size =
+        m_backend.GetTargetSP()->GetMaximumNumberOfChildrenToDisplay();
+  if (m_list_capping_size == 0)
+    m_list_capping_size = 255;
+
+  CompilerType list_type = m_backend.GetCompilerType();
+  if (list_type.IsReferenceType())
+    list_type = list_type.GetNonReferenceType();
+
+  if (list_type.GetNumTemplateArguments() == 0)
+    return false;
+  TemplateArgumentKind kind;
+  m_element_type = list_type.GetTemplateArgument(0, kind);
+
+  return false;
 }
 
-bool lldb_private::formatters::LibcxxStdListSyntheticFrontEnd::HasLoop(
-    size_t count) {
+bool AbstractListFrontEnd::HasLoop(size_t count) {
   if (!g_use_loop_detect)
     return false;
   // don't bother checking for a loop if we won't actually need to jump nodes
   if (m_count < 2)
     return false;
 
   if (m_loop_detected == 0) {
     // This is the first time we are being run (after the last update). Set up
bool AbstractListFrontEnd::HasLoop(size_t count) {
   }
   if (count <= m_loop_detected)
     return false; // No loop in the first m_loop_detected elements.
   if (!m_slow_runner || !m_fast_runner)
     return false; // Reached the end of the list. Definitely no loops.
   return m_slow_runner == m_fast_runner;
 }
 
-size_t lldb_private::formatters::LibcxxStdListSyntheticFrontEnd::
-    CalculateNumChildren() {
+ValueObjectSP AbstractListFrontEnd::GetItem(size_t idx) {
+  size_t advance = idx;
+  ListIterator current(m_head);
+  if (idx > 0) {
+    auto cached_iterator = m_iterators.find(idx - 1);
+    if (cached_iterator != m_iterators.end()) {
+      current = cached_iterator->second;
+      advance = 1;
+    }
+  }
+  ValueObjectSP value_sp = current.advance(advance);
+  m_iterators[idx] = current;
+  return value_sp;
+}
+
+ForwardListFrontEnd::ForwardListFrontEnd(ValueObject &valobj)
+    : AbstractListFrontEnd(valobj) {
+  Update();
+}
+
+size_t ForwardListFrontEnd::CalculateNumChildren() {
+  if (m_count != UINT32_MAX)
+    return m_count;
+
+  ListEntry current(m_head);
+  m_count = 0;
+  while (current && m_count < m_list_capping_size) {
+    ++m_count;
+    current = current.next();
+  }
+  return m_count;
+}
+
+ValueObjectSP ForwardListFrontEnd::GetChildAtIndex(size_t idx) {
+  if (idx >= CalculateNumChildren())
+    return nullptr;
+
+  if (!m_head)
+    return nullptr;
+
+  if (HasLoop(idx + 1))
+    return nullptr;
+
+  ValueObjectSP current_sp = GetItem(idx);
+  if (!current_sp)
+    return nullptr;
+
+  current_sp = current_sp->GetChildAtIndex(1, true); // get the __value_ child
+  if (!current_sp)
+    return nullptr;
+
+  // we need to copy current_sp into a new object otherwise we will end up with
+  // all items named __value_
+  DataExtractor data;
+  Status error;
+  current_sp->GetData(data, error);
+  if (error.Fail())
+    return nullptr;
+
+  return CreateValueObjectFromData(llvm::formatv("[{0}]", idx).str(), data,
+                                   m_backend.GetExecutionContextRef(),
+                                   m_element_type);
+}
+
+bool ForwardListFrontEnd::Update() {
+  AbstractListFrontEnd::Update();
+
+  Status err;
+  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 = impl_sp->GetChildMemberWithName(ConstString("__first_"), true);
+  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), m_node_address(), m_tail(nullptr) {
+  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 first(
while (current.next() && current.next().value() != m_node_address) {
       current = current.next();
       if (size > m_list_capping_size)
         break;
     }
     return m_count = (size - 1);
   }
 }
 
-lldb::ValueObjectSP
-lldb_private::formatters::LibcxxStdListSyntheticFrontEnd::GetChildAtIndex(
-    size_t idx) {
+lldb::ValueObjectSP ListFrontEnd::GetChildAtIndex(size_t idx) {
   static ConstString g_value("__value_");
   static ConstString g_next("__next_");
 
   if (idx >= CalculateNumChildren())
     return lldb::ValueObjectSP();
 
   if (!m_head || !m_tail || m_node_address == 0)
     return lldb::ValueObjectSP();
 
   if (HasLoop(idx + 1))
     return lldb::ValueObjectSP();
 
-  size_t actual_advance = idx;
-
-  ListIterator current(m_head);
-  if (idx > 0) {
-    auto cached_iterator = m_iterators.find(idx - 1);
-    if (cached_iterator != m_iterators.end()) {
-      current = cached_iterator->second;
-      actual_advance = 1;
-    }
-  }
-
-  ValueObjectSP current_sp(current.advance(actual_advance));
+  ValueObjectSP current_sp = GetItem(idx);
   if (!current_sp)
     return lldb::ValueObjectSP();
 
-  m_iterators[idx] = current;
-
   current_sp = current_sp->GetChildAtIndex(1, true); // get the __value_ child
   if (!current_sp)
     return lldb::ValueObjectSP();
 
   if (current_sp->GetName() == g_next) {
     ProcessSP process_sp(current_sp->GetProcessSP());
     if (!process_sp)
       return nullptr;
lldb::ValueObjectSP ListFrontEnd::GetChildAtIndex(size_t idx) {
 
   StreamString name;
   name.Printf("[%" PRIu64 "]", (uint64_t)idx);
   return CreateValueObjectFromData(name.GetString(), data,
                                    m_backend.GetExecutionContextRef(),
                                    m_element_type);
 }
 
-bool lldb_private::formatters::LibcxxStdListSyntheticFrontEnd::Update() {
-  m_iterators.clear();
-  m_head = m_tail = nullptr;
+bool ListFrontEnd::Update() {
+  AbstractListFrontEnd::Update();
+  m_tail = nullptr;
   m_node_address = 0;
-  m_count = UINT32_MAX;
-  m_loop_detected = 0;
-  m_slow_runner.SetEntry(nullptr);
-  m_fast_runner.SetEntry(nullptr);
 
   Status err;
   ValueObjectSP backend_addr(m_backend.AddressOf(err));
-  m_list_capping_size = 0;
-  if (m_backend.GetTargetSP())
-    m_list_capping_size =
-        m_backend.GetTargetSP()->GetMaximumNumberOfChildrenToDisplay();
-  if (m_list_capping_size == 0)
-    m_list_capping_size = 255;
   if (err.Fail() || !backend_addr)
     return false;
   m_node_address = backend_addr->GetValueAsUnsigned(0);
   if (!m_node_address || m_node_address == LLDB_INVALID_ADDRESS)
     return false;
   ValueObjectSP impl_sp(
       m_backend.GetChildMemberWithName(ConstString("__end_"), true));
   if (!impl_sp)
     return false;
-  CompilerType list_type = m_backend.GetCompilerType();
-  if (list_type.IsReferenceType())
-    list_type = list_type.GetNonReferenceType();
-
-  if (list_type.GetNumTemplateArguments() == 0)
-    return false;
-  lldb::TemplateArgumentKind kind;
-  m_element_type = list_type.GetTemplateArgument(0, kind);
   m_head = impl_sp->GetChildMemberWithName(ConstString("__next_"), true).get();
   m_tail = impl_sp->GetChildMemberWithName(ConstString("__prev_"), true).get();
   return false;
 }
 
-bool lldb_private::formatters::LibcxxStdListSyntheticFrontEnd::
-    MightHaveChildren() {
-  return true;
-}
-
-size_t lldb_private::formatters::LibcxxStdListSyntheticFrontEnd::
-    GetIndexOfChildWithName(const ConstString &name) {
-  return ExtractIndexFromString(name.GetCString());
+SyntheticChildrenFrontEnd *formatters::LibcxxStdListSyntheticFrontEndCreator(
+    CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
+  return (valobj_sp ? new ListFrontEnd(valobj_sp) : nullptr);
 }
 
 SyntheticChildrenFrontEnd *
-lldb_private::formatters::LibcxxStdListSyntheticFrontEndCreator(
+formatters::LibcxxStdForwardListSyntheticFrontEndCreator(
     CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
-  return (valobj_sp ? new LibcxxStdListSyntheticFrontEnd(valobj_sp) : nullptr);
+  return valobj_sp ? new ForwardListFrontEnd(*valobj_sp) : nullptr;
 }