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

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

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

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

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

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

299–301

?

303–310

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

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

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

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

182

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

251–260

Indeed it can.

299–301

Oops.

[labath]

Address review comments.

[jingham]

Great, this is much nicer.

source/Plugins/Language/CPlusPlus/LibCxxList.cpp

141

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

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

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',
+                             '}'])

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
+}

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,

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 *

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
+class AbstractListFrontEnd: public SyntheticChildrenFrontEnd {
+public:
+  size_t GetIndexOfChildWithName(const ConstString &name) override {
+    return ExtractIndexFromString(name.GetCString());
+  }
+  bool MightHaveChildren() override { return true; }
+  bool Update() override;
+
+protected:
+  AbstractListFrontEnd(ValueObject &valobj)
+      : SyntheticChildrenFrontEnd(valobj) {}
+
+  size_t m_count;
+  ValueObject *m_head;
+
+  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
+
+  bool HasLoop(size_t count);
+};
 
-namespace lldb_private {
-namespace formatters {
-class LibcxxStdListSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
+class ForwardListFrontEnd: public AbstractListFrontEnd {
+public:
+  ForwardListFrontEnd(ValueObject &valobj);
+
+  size_t CalculateNumChildren() override;
+  ValueObjectSP GetChildAtIndex(size_t idx) override;
+  bool Update() override;
+
+private:
+  size_t m_list_capping_size;
+  CompilerType m_element_type;
+  std::map<size_t, ListIterator> m_iterators;
+};
+
+class LibcxxStdListSyntheticFrontEnd : public AbstractListFrontEnd {
 public:
   LibcxxStdListSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp);
 
   ~LibcxxStdListSyntheticFrontEnd() override = default;
 
   size_t CalculateNumChildren() override;
 
   lldb::ValueObjectSP GetChildAtIndex(size_t idx) override;
 
   bool Update() override;
 
-  bool MightHaveChildren() override;
-
-  size_t GetIndexOfChildWithName(const ConstString &name) override;
-
 private:
-  bool HasLoop(size_t count);
-
   size_t m_list_capping_size;
-  static const 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
 
   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;
+  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() {
+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;
+
+  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));
+  if (!current_sp)
+    return nullptr;
+
+  m_iterators[idx] = current;
+
+  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();
+
+  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;
+
+  Status err;
+  ValueObjectSP backend_addr(m_backend.AddressOf(err));
+  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;*/
+
+  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);
+
+  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;
+}
+
+LibcxxStdListSyntheticFrontEnd::LibcxxStdListSyntheticFrontEnd(
+    lldb::ValueObjectSP valobj_sp)
+    : AbstractListFrontEnd(*valobj_sp), m_list_capping_size(0),
+      m_node_address(), m_tail(nullptr), m_element_type(), m_iterators() {
+  if (valobj_sp)
+    Update();
+}
+
+size_t LibcxxStdListSyntheticFrontEnd::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) {
       if (size > m_list_capping_size)
         break;
     }
     return m_count = (size - 1);
   }
 }
 
 lldb::ValueObjectSP
-lldb_private::formatters::LibcxxStdListSyntheticFrontEnd::GetChildAtIndex(
-    size_t idx) {
+LibcxxStdListSyntheticFrontEnd::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();
LibcxxStdListSyntheticFrontEnd::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() {
+bool LibcxxStdListSyntheticFrontEnd::Update() {
   m_iterators.clear();
   m_head = 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);
 
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 LibcxxStdListSyntheticFrontEnd(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;
 }