[formatters] Add a libstdcpp formatter for bitset and unify tests across stdlibs

[danilashtefan]

Summary

This diff adds a data formatter for libstdcpp's bitset. Besides, it unifies the tests for bitset for libcxx and libstdcpp for maintainability.

[teemperor]

Thanks for working on this!

Some notes/nits:

• nit: The patch seems to have a bunch of unrelated clang-format changes for CPlusPlusLanguage.cpp in it. You can prevent that by using git clang-format on your local git commit before putting the diff on Phabricator (git tells clang-format which lines were modified by you and clang-format will only format those).
• LibStdcppBitset.cpp seems to be a 99% identical copy of LibCxxBitset.cpp. I think those two classes can just be one class with some variable for the different field name in each implementation.
• Same for the test. I think we can merge that into something like .../data-formatter-stl/generic/bitset and then just have two test_... methods where one is decorated for libc++ and one for libstdc++. You can change the Makefile values for each test with the dictionary argument of self.build (something like self.build(dictionary={"USE_LIBSTDCPP":"1"}) should work I believe).

lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt

9

I think the idea is to keep this sorted alphabetically (even though this position probably make more sense, but oh well...)

[wallace]

addressed all comments.
I was able to dedup all the implementation files and the tests, and indeed passing
the make flags to the build method was a good trick.

[wallace]

nit

[teemperor]

Thanks! This is looking pretty good, I just have some final minor comments about some code that I think we can also drop/split-out.

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

20–21

nit: /// to make this into a proper doc string.

23–28

I like it!

93

If you look in CPlusPlusLanguage.cpp there is a thing called stl_deref_flags and you can just use the same two lines that define that variable in the Libstdc++ init function. The stl_deref_flags.SetFrontEndWantsDereference(); will set a flag that does this whole deref-stuff here automatically (that's why this code wasn't necessary for the libc++ provider).

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py

66

is there a specific reason for these added self.expect("p ...", substrs=[<many lines>]) checks? I think testing the formatter with the expression evaluator is good, but there are far less verbose ways to test this (to be specific, self.primes could be a list of ValueCheck objects and you can pass that same ValueObject list as the result_children/children parameters to self.expect_expr or self.expect_var_path. This could be done in check so it would also cover all the checks in this test.).

So I would suggest we drop the added checks here and instead leave this up to a separate commit that refactors the test (you can do that if you want, but I don't mind doing it myself). This commit is in the current state just repurposing the existing libcxx formatter (with all the potentially existing bugs), so I think this is good enough for landing without expanding the test. Otherwise I think these checks could be implemented in a better way with the ValueCheck approach (see above).

[danilashtefan]

I have corrected everything according to the previous comments. Wallace will kindly help me to format the code, since I have the following formatter error Stack dump:
0. Program arguments: clang-format -lines=1:149 -lines=0:0 lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp
/lib/x86_64-linux-gnu/libLLVM-10.so.1(_ZN4llvm3sys15PrintStackTraceERNS_11raw_ostreamE+0x1f)[0x7fa7b5d204ff]
/lib/x86_64-linux-gnu/libLLVM-10.so.1(_ZN4llvm3sys17RunSignalHandlersEv+0x50)[0x7fa7b5d1e7b0]
/lib/x86_64-linux-gnu/libLLVM-10.so.1(+0x981ac5)[0x7fa7b5d20ac5]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x153c0)[0x7fa7bc4f03c0]
/lib/x86_64-linux-gnu/libclang-cpp.so.10(_ZNK5clang13SourceManager16translateLineColENS_6FileIDEjj+0xcf)[0x7fa7ba1cc83f]
clang-format[0x407e10]
clang-format[0x406b64]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3)[0x7fa7b4e4e0b3]
clang-format[0x40652e]
error: clang-format -lines=1:149 -lines=0:0 lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp failed

[wallace]

almost there!

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

20

Use /// instead of //, that way it'll appear in the official documentation

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py

32–35

What teemperor is saying is to something like:

children = [ValueCheck(name="<something goes here>", summary='<same here>')]
for i in range(size):
  children.append(
valobj = self.expect_var_path(
          name,
          type="std::bitset<" + size + ">", # you might need to change this depending on the stdlib you use for each test
          children=children,
      )

self.assertRegex(valobj.summary, r'^"hello"( strong=1)? weak=1$') # change this for the actual summary of the object

[wallace]

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py

32–35

Ugh, I made some mistakes

children = []
for i in range(size):
  children.append(
valobj = self.expect_var_path( // this will check the entire bitset and the children argument will check its children
          name,
          type="std::bitset<" + size + ">", // you might need to change this depending on the stdlib you use for each test
          children=children,
      )

self.assertRegex(valobj.summary, r'^"hello"( strong=1)? weak=1$') # change this for the actual summary of the object

[danilashtefan]

Minor corrections

[wallace]

I think you messed up when updating the diffs. I don't see the bitset diff anymore.

[danilashtefan]

Fixing

[danilashtefan]

With this changes I substitute the verbose long printed check with ValueCheck approach

[danilashtefan]

Refactoring

[danilashtefan]

Refactoring

[wallace]

great! I'll land it for you

[wallace]

Pushed upstream as 566bfbb740bb0ffef827ce983d7aa899236fcb88

lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt

 add_lldb_library(lldbPluginCPlusPlusLanguage PLUGIN
   BlockPointer.cpp
   CPlusPlusLanguage.cpp
   CPlusPlusNameParser.cpp
   CxxStringTypes.cpp
+  GenericBitset.cpp
   LibCxx.cpp
   LibCxxAtomic.cpp
-  LibCxxBitset.cpp
   LibCxxInitializerList.cpp
   LibCxxList.cpp
   LibCxxMap.cpp
   LibCxxOptional.cpp
   LibCxxQueue.cpp
   LibCxxTuple.cpp
   LibCxxUnorderedMap.cpp
   LibCxxVariant.cpp
   LibCxxVector.cpp

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

static void LoadLibStdcppFormatters(lldb::TypeCategoryImplSP cpp_category_sp) {
   cpp_category_sp->GetTypeSummariesContainer()->Add(
       ConstString("std::__cxx11::basic_string<wchar_t, "
                   "std::char_traits<wchar_t>, std::allocator<wchar_t> >"),
       cxx11_wstring_summary_sp);
 
   SyntheticChildren::Flags stl_synth_flags;
   stl_synth_flags.SetCascades(true).SetSkipPointers(false).SetSkipReferences(
       false);
+  SyntheticChildren::Flags stl_deref_flags = stl_synth_flags;
+  stl_deref_flags.SetFrontEndWantsDereference();
 
   cpp_category_sp->GetRegexTypeSyntheticsContainer()->Add(
       RegularExpression("^std::vector<.+>(( )?&)?$"),
       SyntheticChildrenSP(new ScriptedSyntheticChildren(
           stl_synth_flags,
           "lldb.formatters.cpp.gnu_libstdcpp.StdVectorSynthProvider")));
   cpp_category_sp->GetRegexTypeSyntheticsContainer()->Add(
       RegularExpression("^std::map<.+> >(( )?&)?$"),
       SyntheticChildrenSP(new ScriptedSyntheticChildren(
           stl_synth_flags,
           "lldb.formatters.cpp.gnu_libstdcpp.StdMapSynthProvider")));
   cpp_category_sp->GetRegexTypeSyntheticsContainer()->Add(
       RegularExpression("^std::(__cxx11::)?list<.+>(( )?&)?$"),
       SyntheticChildrenSP(new ScriptedSyntheticChildren(
           stl_synth_flags,
           "lldb.formatters.cpp.gnu_libstdcpp.StdListSynthProvider")));
   stl_summary_flags.SetDontShowChildren(false);
   stl_summary_flags.SetSkipPointers(true);
   cpp_category_sp->GetRegexTypeSummariesContainer()->Add(
+      RegularExpression("^std::bitset<.+>(( )?&)?$"),
+      TypeSummaryImplSP(
+          new StringSummaryFormat(stl_summary_flags, "size=${svar%#}")));
+  cpp_category_sp->GetRegexTypeSummariesContainer()->Add(
       RegularExpression("^std::vector<.+>(( )?&)?$"),
       TypeSummaryImplSP(
           new StringSummaryFormat(stl_summary_flags, "size=${svar%#}")));
   cpp_category_sp->GetRegexTypeSummariesContainer()->Add(
       RegularExpression("^std::map<.+> >(( )?&)?$"),
       TypeSummaryImplSP(
           new StringSummaryFormat(stl_summary_flags, "size=${svar%#}")));
   cpp_category_sp->GetRegexTypeSummariesContainer()->Add(
AddCXXSynthetic(
       "std::weak_ptr synthetic children",
       ConstString("^std::weak_ptr<.+>(( )?&)?$"), stl_synth_flags, true);
   AddCXXSynthetic(
       cpp_category_sp,
       lldb_private::formatters::LibStdcppTupleSyntheticFrontEndCreator,
       "std::tuple synthetic children", ConstString("^std::tuple<.+>(( )?&)?$"),
       stl_synth_flags, true);
 
+  AddCXXSynthetic(
+      cpp_category_sp,
+      lldb_private::formatters::LibStdcppBitsetSyntheticFrontEndCreator,
+      "std::bitset synthetic child", ConstString("^std::bitset<.+>(( )?&)?$"),
+      stl_deref_flags, true);
+
   AddCXXSummary(cpp_category_sp,
                 lldb_private::formatters::LibStdcppUniquePointerSummaryProvider,
                 "libstdc++ std::unique_ptr summary provider",
                 ConstString("^std::unique_ptr<.+>(( )?&)?$"), stl_summary_flags,
                 true);
   AddCXXSummary(cpp_category_sp,
                 lldb_private::formatters::LibStdcppSmartPointerSummaryProvider,
                 "libstdc++ std::shared_ptr summary provider",

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

-//===-- LibCxxBitset.cpp --------------------------------------------------===//
+//===-- GenericBitset.cpp //-----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "LibCxx.h"
+#include "LibStdcpp.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "lldb/DataFormatters/FormattersHelpers.h"
 #include "lldb/Target/Target.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 namespace {
 
-class BitsetFrontEnd : public SyntheticChildrenFrontEnd {
+// This class can be used for handling bitsets from both libcxx and libstdcpp.
+class GenericBitsetFrontEnd : public SyntheticChildrenFrontEnd {
 public:
-  BitsetFrontEnd(ValueObject &valobj);
+  enum class StdLib {
+    LibCxx,
+    LibStdcpp,
+  };
+
+  GenericBitsetFrontEnd(ValueObject &valobj, StdLib stdlib);
 
   size_t GetIndexOfChildWithName(ConstString name) override {
     return formatters::ExtractIndexFromString(name.GetCString());
   }
 
   bool MightHaveChildren() override { return true; }
   bool Update() override;
   size_t CalculateNumChildren() override { return m_elements.size(); }
   ValueObjectSP GetChildAtIndex(size_t idx) override;
 
 private:
+  ConstString GetDataContainerMemberName();
+
   // The lifetime of a ValueObject and all its derivative ValueObjects
   // (children, clones, etc.) is managed by a ClusterManager. These
   // objects are only destroyed when every shared pointer to any of them
   // is destroyed, so we must not store a shared pointer to any ValueObject
   // derived from our backend ValueObject (since we're in the same cluster).
   // Value objects created from raw data (i.e. in a different cluster) must
   // be referenced via shared pointer to keep them alive, however.
   std::vector<ValueObjectSP> m_elements;
-  ValueObject* m_first = nullptr;
+  ValueObject *m_first = nullptr;
   CompilerType m_bool_type;
   ByteOrder m_byte_order = eByteOrderInvalid;
   uint8_t m_byte_size = 0;
+  StdLib m_stdlib;
 };
 } // namespace
 
-BitsetFrontEnd::BitsetFrontEnd(ValueObject &valobj)
-    : SyntheticChildrenFrontEnd(valobj) {
+GenericBitsetFrontEnd::GenericBitsetFrontEnd(ValueObject &valobj, StdLib stdlib)
+    : SyntheticChildrenFrontEnd(valobj), m_stdlib(stdlib) {
   m_bool_type = valobj.GetCompilerType().GetBasicTypeFromAST(eBasicTypeBool);
   if (auto target_sp = m_backend.GetTargetSP()) {
     m_byte_order = target_sp->GetArchitecture().GetByteOrder();
     m_byte_size = target_sp->GetArchitecture().GetAddressByteSize();
     Update();
   }
 }
 
-bool BitsetFrontEnd::Update() {
+ConstString GenericBitsetFrontEnd::GetDataContainerMemberName() {
+  switch (m_stdlib) {
+  case StdLib::LibCxx:
+    return ConstString("__first_");
+  case StdLib::LibStdcpp:
+    return ConstString("_M_w");
+  }
+}
+
+bool GenericBitsetFrontEnd::Update() {
   m_elements.clear();
   m_first = nullptr;
 
   TargetSP target_sp = m_backend.GetTargetSP();
   if (!target_sp)
     return false;
   size_t capping_size = target_sp->GetMaximumNumberOfChildrenToDisplay();
 
   size_t size = 0;
+
   if (auto arg = m_backend.GetCompilerType().GetIntegralTemplateArgument(0))
     size = arg->value.getLimitedValue(capping_size);
 
   m_elements.assign(size, ValueObjectSP());
-
-  m_first = m_backend.GetChildMemberWithName(ConstString("__first_"), true).get();
+  m_first = m_backend.GetChildMemberWithName(GetDataContainerMemberName(), true)
+                .get();
   return false;
 }
 
-ValueObjectSP BitsetFrontEnd::GetChildAtIndex(size_t idx) {
+ValueObjectSP GenericBitsetFrontEnd::GetChildAtIndex(size_t idx) {
   if (idx >= m_elements.size() || !m_first)
     return ValueObjectSP();
 
   if (m_elements[idx])
     return m_elements[idx];
 
   ExecutionContext ctx = m_backend.GetExecutionContextRef().Lock(false);
   CompilerType type;
ValueObjectSP GenericBitsetFrontEnd::GetChildAtIndex(size_t idx) {
   DataExtractor data(&value, sizeof(value), m_byte_order, m_byte_size);
 
   m_elements[idx] = CreateValueObjectFromData(llvm::formatv("[{0}]", idx).str(),
                                               data, ctx, m_bool_type);
 
   return m_elements[idx];
 }
 
+SyntheticChildrenFrontEnd *formatters::LibStdcppBitsetSyntheticFrontEndCreator(
+    CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
+  if (valobj_sp)
+    return new GenericBitsetFrontEnd(*valobj_sp,
+                                     GenericBitsetFrontEnd::StdLib::LibStdcpp);
+  return nullptr;
+}
+
 SyntheticChildrenFrontEnd *formatters::LibcxxBitsetSyntheticFrontEndCreator(
     CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
   if (valobj_sp)
-    return new BitsetFrontEnd(*valobj_sp);
+    return new GenericBitsetFrontEnd(*valobj_sp,
+                                     GenericBitsetFrontEnd::StdLib::LibCxx);
   return nullptr;
 }

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

 LibstdcppMapIteratorSyntheticFrontEndCreator(CXXSyntheticChildren *,
                                              lldb::ValueObjectSP);
 
 SyntheticChildrenFrontEnd *
 LibStdcppTupleSyntheticFrontEndCreator(CXXSyntheticChildren *,
                                        lldb::ValueObjectSP);
 
 SyntheticChildrenFrontEnd *
+LibStdcppBitsetSyntheticFrontEndCreator(CXXSyntheticChildren *,
+                                        lldb::ValueObjectSP);
+
+SyntheticChildrenFrontEnd *
 LibStdcppVectorIteratorSyntheticFrontEndCreator(CXXSyntheticChildren *,
                                                 lldb::ValueObjectSP);
 
 SyntheticChildrenFrontEnd *
 LibStdcppSharedPtrSyntheticFrontEndCreator(CXXSyntheticChildren *,
                                            lldb::ValueObjectSP);
 
 SyntheticChildrenFrontEnd *
 LibStdcppUniquePtrSyntheticFrontEndCreator(CXXSyntheticChildren *,
                                            lldb::ValueObjectSP);
 
 } // namespace formatters
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_LANGUAGE_CPLUSPLUS_LIBSTDCPP_H

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/Makefile

 CXX_SOURCES := main.cpp
 
-USE_LIBCPP := 1
 include Makefile.rules

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py

 """
-Test lldb data formatter subsystem.
+Test lldb data formatter subsystem for bitset for libcxx and libstdcpp.
 """
 
 
 
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
 
-class TestDataFormatterLibcxxBitset(TestBase):
+class GenericBitsetDataFormatterTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     def setUp(self):
         TestBase.setUp(self)
-
         primes = [1]*300
         primes[0] = primes[1] = 0
         for i in range(2, len(primes)):
             for j in range(2*i, len(primes), i):
                 primes[j] = 0
         self.primes = primes
 
     def check(self, name, size):
         var = self.frame().FindVariable(name)
         self.assertTrue(var.IsValid())
         self.assertEqual(var.GetNumChildren(), size)
         for i in range(size):
             child = var.GetChildAtIndex(i)
             self.assertEqual(child.GetValueAsUnsigned(), self.primes[i],
                     "variable: %s, index: %d"%(name, size))
 
-    @add_test_categories(["libc++"])
-    def test_value(self):
+    def do_test_value(self, stdlib_type):
         """Test that std::bitset is displayed correctly"""
-        self.build()
+        self.build(dictionary={stdlib_type: "1"})
+
         lldbutil.run_to_source_breakpoint(self, '// break here',
                 lldb.SBFileSpec("main.cpp", False))
 
         self.check("empty", 0)
         self.check("small", 13)
-        self.check("large", 200)
+        self.check("large", 70)
+
+    @add_test_categories(["libstdcxx"])
+    def test_value_libstdcpp(self):
+        self.do_test_value(USE_LIBSTDCPP)
 
     @add_test_categories(["libc++"])
-    def test_ptr_and_ref(self):
+    def test_value_libcpp(self):
+        self.do_test_value(USE_LIBCPP)
+
+    def do_test_ptr_and_ref(self, stdlib_type):
         """Test that ref and ptr to std::bitset is displayed correctly"""
-        self.build()
+        self.build(dictionary={stdlib_type: "1"})
+
         (_, process, _, bkpt) = lldbutil.run_to_source_breakpoint(self,
                 'Check ref and ptr',
                 lldb.SBFileSpec("main.cpp", False))
 
         self.check("ref", 13)
         self.check("ptr", 13)
 
         lldbutil.continue_to_breakpoint(process, bkpt)
 
-        self.check("ref", 200)
-        self.check("ptr", 200)
+        self.check("ref", 70)
+        self.check("ptr", 70)
+
+    @add_test_categories(["libstdcxx"])
+    def test_ptr_and_ref_libstdcpp(self):
+        self.do_test_ptr_and_ref(USE_LIBSTDCPP)
+
+    @add_test_categories(["libc++"])
+    def test_ptr_and_ref_libcpp(self):
+        self.do_test_ptr_and_ref(USE_LIBCPP)

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/main.cpp

 #include <bitset>
 #include <stdio.h>
 
-template<std::size_t N>
-void fill(std::bitset<N> &b) {
+template <std::size_t N> void fill(std::bitset<N> &b) {
   b.set();
   b[0] = b[1] = false;
   for (std::size_t i = 2; i < N; ++i) {
-    for (std::size_t j = 2*i; j < N; j+=i)
+    for (std::size_t j = 2 * i; j < N; j += i)
       b[j] = false;
   }
 }
 
-template<std::size_t N>
+template <std::size_t N>
 void by_ref_and_ptr(std::bitset<N> &ref, std::bitset<N> *ptr) {
-    // Check ref and ptr
-    return;
 }
 
 int main() {
   std::bitset<0> empty;
   std::bitset<13> small;
   fill(small);
-  std::bitset<200> large;
+  std::bitset<70> large;
   fill(large);
   by_ref_and_ptr(small, &small); // break here
-  by_ref_and_ptr(large, &large);
+  by_ref_and_ptr(large, &large); 
   return 0;
 }