[lldb][test] Add a layout simulator test for std::unique_ptr #98330

Michael137 · 2024-07-10T14:45:33Z

This is motivated by the upcoming refactor of libc++'s __compressed_pair in #76756

As this will require changes to numerous LLDB libc++ data-formatters (see early draft #96538), it would be nice to have a test-suite that will actually exercise both the old and new layout. We have a matrix bot that tests old versions of Clang (but currently those only date back to Clang-15). Having them in the test-suite will give us quicker signal on what broke.

We have an existing test that exercises various layouts of std::string over time in TestDataFormatterLibcxxStringSimulator.py, but that's the only STL type we have it for. This patch proposes a new libcxx-simulators directory which will take the same approach for all the STL types that we can feasibly support in this way (as @labath points out, for some types this might just not be possible due to their implementation complexity). Nonetheless, it'd be great to have a record of how the layout of libc++ types changed over time. Eventually we'll move the std::string simulator into this directory too.

Some related discussion:

[lldb] Make variant formatter work with libstdc++-14 #97568 (comment)

llvmbot · 2024-07-10T14:46:05Z

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

This is currently just a prototype.

This is motivated by the upcoming refactor of libc++'s __compressed_pair in #76756

As this will require changes to numerous LLDB libc++ data-formatters (see early draft #96538), it would be nice to have a test-suite that will actually exercise both the old and new layout. We have a matrix bot that tests old versions of Clang (but currently those only date back to Clang-15). Having them in the test-suite will give us quicker signal on what broke.

We have an existing test that exercises various layouts of std::string over time in TestDataFormatterLibcxxStringSimulator.py, but that's the only STL type we have it for.

This patch proposes a new libcxx-simulators directory which will take the same approach. It'll be great to have a record of how the layout of libc++ types. Eventually we'll move the std::string simulator into this directory too.

Some related discussion:

[lldb] Make variant formatter work with libstdc++-14 #97568 (comment)

Full diff: https://github.com/llvm/llvm-project/pull/98330.diff

4 Files Affected:

(added) lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/compressed_pair.h (+89)
(added) lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/unique_ptr/Makefile (+3)
(added) lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/unique_ptr/TestDataFormatterLibcxxUniquePtrSimulator.py (+32)
(added) lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/unique_ptr/main.cpp (+43)

diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/compressed_pair.h b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/compressed_pair.h
new file mode 100644
index 0000000000000..ec978b8053646
--- /dev/null
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/compressed_pair.h
@@ -0,0 +1,89 @@
+#ifndef STD_LLDB_COMPRESSED_PAIR_H
+#define STD_LLDB_COMPRESSED_PAIR_H
+
+#include <__memory/compressed_pair.h>
+#include <type_traits>
+
+namespace std {
+namespace __lldb {
+
+#if COMPRESSED_PAIR_REV == 0 // Post-c88580c layout
+struct __value_init_tag {};
+struct __default_init_tag {};
+
+template <class _Tp, int _Idx,
+          bool _CanBeEmptyBase =
+              std::is_empty<_Tp>::value && !std::is_final<_Tp>::value>
+struct __compressed_pair_elem {
+  explicit __compressed_pair_elem(__default_init_tag) {}
+  explicit __compressed_pair_elem(__value_init_tag) : __value_() {}
+
+  explicit __compressed_pair_elem(_Tp __t) : __value_(__t) {}
+
+  _Tp &__get() { return __value_; }
+
+private:
+  _Tp __value_;
+};
+
+template <class _Tp, int _Idx>
+struct __compressed_pair_elem<_Tp, _Idx, true> : private _Tp {
+  explicit __compressed_pair_elem(_Tp __t) : _Tp(__t) {}
+  explicit __compressed_pair_elem(__default_init_tag) {}
+  explicit __compressed_pair_elem(__value_init_tag) : _Tp() {}
+
+  _Tp &__get() { return *this; }
+};
+
+template <class _T1, class _T2>
+class __compressed_pair : private __compressed_pair_elem<_T1, 0>,
+                          private __compressed_pair_elem<_T2, 1> {
+public:
+  using _Base1 = __compressed_pair_elem<_T1, 0>;
+  using _Base2 = __compressed_pair_elem<_T2, 1>;
+
+  explicit __compressed_pair(_T1 __t1, _T2 __t2) : _Base1(__t1), _Base2(__t2) {}
+  explicit __compressed_pair()
+      : _Base1(__value_init_tag()), _Base2(__value_init_tag()) {}
+
+  template <class _U1, class _U2>
+  explicit __compressed_pair(_U1 &&__t1, _U2 &&__t2)
+      : _Base1(std::forward<_U1>(__t1)), _Base2(std::forward<_U2>(__t2)) {}
+
+  _T1 &first() { return static_cast<_Base1 &>(*this).__get(); }
+};
+#elif COMPRESSED_PAIR_REV == 1
+#define _LLDB_COMPRESSED_PAIR(T1, Initializer1, T2, Initializer2)              \
+  [[__gnu__::__aligned__(alignof(T2))]] [[no_unique_address]] T1 Initializer1; \
+  [[no_unique_address]] __compressed_pair_padding<T1> _LIBCPP_CONCAT3(         \
+      __padding1_, __LINE__, _);                                               \
+  [[no_unique_address]] T2 Initializer2;                                       \
+  [[no_unique_address]] __compressed_pair_padding<T2> _LIBCPP_CONCAT3(         \
+      __padding2_, __LINE__, _)
+
+#define _LLDB_COMPRESSED_TRIPLE(T1, Initializer1, T2, Initializer2, T3,        \
+                                Initializer3)                                  \
+  [[using __gnu__: __aligned__(alignof(T2)),                                   \
+    __aligned__(alignof(T3))]] [[no_unique_address]] T1 Initializer1;          \
+  [[no_unique_address]] __compressed_pair_padding<T1> _LIBCPP_CONCAT3(         \
+      __padding1_, __LINE__, _);                                               \
+  [[no_unique_address]] T2 Initializer2;                                       \
+  [[no_unique_address]] __compressed_pair_padding<T2> _LIBCPP_CONCAT3(         \
+      __padding2_, __LINE__, _);                                               \
+  [[no_unique_address]] T3 Initializer3;                                       \
+  [[no_unique_address]] __compressed_pair_padding<T3> _LIBCPP_CONCAT3(         \
+      __padding3_, __LINE__, _)
+#elif COMPRESSED_PAIR_REV == 2
+#define _LLDB_COMPRESSED_PAIR(T1, Name1, T2, Name2)                            \
+  [[no_unique_address]] T1 Name1;                                              \
+  [[no_unique_address]] T2 Name2
+
+#define _LLDB_COMPRESSED_TRIPLE(T1, Name1, T2, Name2, T3, Name3)               \
+  [[no_unique_address]] T1 Name1;                                              \
+  [[no_unique_address]] T2 Name2;                                              \
+  [[no_unique_address]] T3 Name3
+#endif
+} // namespace __lldb
+} // namespace std
+
+#endif // _H
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/unique_ptr/Makefile b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/unique_ptr/Makefile
new file mode 100644
index 0000000000000..38cfa81053488
--- /dev/null
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/unique_ptr/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+override CXXFLAGS_EXTRAS += -std=c++14
+include Makefile.rules
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/unique_ptr/TestDataFormatterLibcxxUniquePtrSimulator.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/unique_ptr/TestDataFormatterLibcxxUniquePtrSimulator.py
new file mode 100644
index 0000000000000..fe3570ff99ae5
--- /dev/null
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/unique_ptr/TestDataFormatterLibcxxUniquePtrSimulator.py
@@ -0,0 +1,32 @@
+"""
+Test we can understand various layouts of the libc++'s std::unique_ptr
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import functools
+
+class LibcxxUniquePtrDataFormatterSimulatorTestCase(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def _run_test(self, defines):
+        cxxflags_extras = " ".join(["-D%s" % d for d in defines])
+        self.build(dictionary=dict(CXXFLAGS_EXTRAS=cxxflags_extras))
+        lldbutil.run_to_source_breakpoint(
+            self, "// Break here", lldb.SBFileSpec("main.cpp")
+        )
+        self.expect("frame variable var_up", substrs=["pointer ="])
+        self.expect("frame variable var_up", substrs=["deleter ="], matching=False)
+        self.expect("frame variable var_with_deleter_up", substrs=["pointer =", "deleter ="])
+
+#for r in range(3):
+for r in range(1):
+    name = "test_r%d" % r
+    defines = ["COMPRESSED_PAIR_REV=%d" % r]
+    f = functools.partialmethod(
+        LibcxxUniquePtrDataFormatterSimulatorTestCase._run_test, defines
+    )
+    setattr(LibcxxUniquePtrDataFormatterSimulatorTestCase, name, f)
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/unique_ptr/main.cpp b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/unique_ptr/main.cpp
new file mode 100644
index 0000000000000..33066febc7623
--- /dev/null
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/unique_ptr/main.cpp
@@ -0,0 +1,43 @@
+#include "../compressed_pair.h"
+
+#include <__memory/allocator_traits.h>
+
+namespace std {
+namespace __lldb {
+template <class _Tp> struct default_delete {
+  default_delete() noexcept = default;
+
+  void operator()(_Tp *__ptr) const noexcept { delete __ptr; }
+};
+
+template <class _Tp, class _Dp = default_delete<_Tp>> class unique_ptr {
+public:
+  typedef _Tp element_type;
+  typedef _Dp deleter_type;
+  typedef typename __pointer<_Tp, deleter_type>::type pointer;
+
+#if COMPRESSED_PAIR_REV == 0
+  std::__lldb::__compressed_pair<pointer, deleter_type> __ptr_;
+  explicit unique_ptr(pointer __p) noexcept
+      : __ptr_(__p, std::__lldb::__value_init_tag()) {}
+#elif COMPRESSED_PAIR_REV == 1 || COMPRESSED_PAIR_REV == 2
+  _LLDB_COMPRESSED_PAIR(pointer, __ptr_, deleter_type, __deleter_);
+  explicit unique_ptr(pointer __p) noexcept : __ptr_(__p), __deleter_() {}
+#endif
+};
+} // namespace __lldb
+} // namespace std
+
+struct StatefulDeleter {
+  StatefulDeleter() noexcept = default;
+
+  void operator()(int *__ptr) const noexcept { delete __ptr; }
+
+  int m_state = 50;
+};
+
+int main() {
+  std::__lldb::unique_ptr<int> var_up(new int(5));
+  std::__lldb::unique_ptr<int, StatefulDeleter> var_with_deleter_up(new int(5));
+  return 0; // Break here
+}

Michael137 · 2024-07-10T14:48:07Z

It's a non-trivial maintenance cost but I think the upside is quite large:

we'll have more confidence that old layouts don't break over time
faster signal when we break compatibility
trimmed down reference implementation of the various STL layouts without having to trawl through git history
we could even test how the formatters behave on unexpected layouts (don't think we ever test the error paths)

github-actions · 2024-07-10T14:49:26Z

✅ With the latest revision this PR passed the Python code formatter.

labath

I'm like this approach. It can be a lot of work, so we may not be able to get every contributor to do this, but I'd certainly encourage everyone to try.

labath · 2024-07-11T10:13:28Z

...st/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/compressed_pair.h

+#elif COMPRESSED_PAIR_REV == 1
+#define _LLDB_COMPRESSED_PAIR(T1, Initializer1, T2, Initializer2)              \
+  [[__gnu__::__aligned__(alignof(T2))]] [[no_unique_address]] T1 Initializer1; \
+  [[no_unique_address]] __compressed_pair_padding<T1> _LIBCPP_CONCAT3(         \


Where are __compressed_pair_padding and _LIBCPP_CONCAT3 defined? in libc++? I don't think we want to depend on it for several reasons:

it makes the test non-hermetic

it means the test will not run without libc++

libc++ folks might not like us reaching into their private headers like that

I'm also not sure if we really need the _LIBCPP_CONCAT3 concat thing. Since its going to produce nondeterministic (well deterministic, but it will change very easily) values, we can't really rely on it anywhere, so maybe we could just hardcode something here?

Yea good point, relying on libc++ is what we wanted to avoid here in the first place. __compressed_pair_padding and _LIBCPP_CONCAT3 are currently not part of libc++ (but will be soon). I'll go with your suggestion

labath · 2024-07-11T10:18:56Z

.../API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/unique_ptr/main.cpp

@@ -0,0 +1,43 @@
+#include "../compressed_pair.h"


We have some common includes in packages/Python/lldbsuite/test/make/. I think it'd be better to put this there (probably in a subdirectory)

Yup will do, this was the easiest for the prototype. But I'll move it to packages/Python/lldbsuite/test/make/ as you suggest.

Michael137 · 2024-07-11T16:01:10Z

lldb/packages/Python/lldbsuite/test/make/libcxx-simulators-common/compressed_pair.h

+
+#define _LLDB_COMPRESSED_PAIR(T1, Initializer1, T2, Initializer2)              \
+  [[__gnu__::__aligned__(alignof(T2))]] [[no_unique_address]] T1 Initializer1; \
+  [[no_unique_address]] __compressed_pair_padding<T1> __padding1_;             \


For now this doesn't cause any clashes. I think I've seen one instance in libc++ where we would have two compressed pairs within the same type. Can probably tackle that once we get to that? Alternatively could add another suffix parameter to this macro for an easy way to avoid clashes

…ator

Michael137 · 2024-07-12T07:45:59Z

I removed the new compressed_pair layouts from this patch so we can land it in isolation. I'll open a separate PR that introduces those back which we can then land once libc++ actually introduces those new layouts in-tree.

@labath

) Summary: This is motivated by the upcoming refactor of libc++'s `__compressed_pair` in llvm#76756 As this will require changes to numerous LLDB libc++ data-formatters (see early draft llvm#96538), it would be nice to have a test-suite that will actually exercise both the old and new layout. We have a matrix bot that tests old versions of Clang (but currently those only date back to Clang-15). Having them in the test-suite will give us quicker signal on what broke. We have an existing test that exercises various layouts of `std::string` over time in `TestDataFormatterLibcxxStringSimulator.py`, but that's the only STL type we have it for. This patch proposes a new `libcxx-simulators` directory which will take the same approach for all the STL types that we can feasibly support in this way (as @labath points out, for some types this might just not be possible due to their implementation complexity). Nonetheless, it'd be great to have a record of how the layout of libc++ types changed over time. Some related discussion: * llvm#97568 (comment) Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D59822424

@labath

This is motivated by the upcoming refactor of libc++'s `__compressed_pair` in #76756 As this will require changes to numerous LLDB libc++ data-formatters (see early draft #96538), it would be nice to have a test-suite that will actually exercise both the old and new layout. We have a matrix bot that tests old versions of Clang (but currently those only date back to Clang-15). Having them in the test-suite will give us quicker signal on what broke. We have an existing test that exercises various layouts of `std::string` over time in `TestDataFormatterLibcxxStringSimulator.py`, but that's the only STL type we have it for. This patch proposes a new `libcxx-simulators` directory which will take the same approach for all the STL types that we can feasibly support in this way (as @labath points out, for some types this might just not be possible due to their implementation complexity). Nonetheless, it'd be great to have a record of how the layout of libc++ types changed over time. Some related discussion: * #97568 (comment)

…ests (#99012) This is a follow-up to #98330 for the upcoming `__compressed_pair` refactor in #93069 This patch just adds the 2 new copies of `_LIBCPP_COMPRESSED_PAIR` layouts to the simulator tests.

[WIP][lldb][test] Add a layout simulator test for std::unique_ptr

a25b3c8

Michael137 requested a review from JDevlieghere as a code owner July 10, 2024 14:45

Michael137 requested review from adrian-prantl, labath and dwblaikie July 10, 2024 14:45

llvmbot added the lldb label Jul 10, 2024

fixup! format python file

b25d2ab

labath reviewed Jul 11, 2024

View reviewed changes

fixup! remove dependency on internal libc++ headers; move common header

1575ca2

Michael137 commented Jul 11, 2024

View reviewed changes

Michael137 added 2 commits July 12, 2024 08:43

fixup! remove new compressed_pair layout for now

69c2a7c

fixup! use the common compressed_pair implementation for string simul…

1996194

…ator

Michael137 force-pushed the lldb/stl-format-simulators branch from acad452 to 1996194 Compare July 12, 2024 07:43

Michael137 changed the title ~~[WIP][lldb][test] Add a layout simulator test for std::unique_ptr~~ [lldb][test] Add a layout simulator test for std::unique_ptr Jul 12, 2024

labath approved these changes Jul 15, 2024

View reviewed changes

fixup! add missing header

c13766b

Michael137 merged commit 56c4ec9 into llvm:main Jul 16, 2024
6 checks passed

Michael137 deleted the lldb/stl-format-simulators branch July 16, 2024 04:21

Michael137 mentioned this pull request Jul 16, 2024

[lldb][test] Add a new __compressed_pair layout to libcxx simulator tests #99012

Merged

luporl mentioned this pull request Jul 17, 2024

[lldb][test] Fix simulator test for std::unique_ptr #99357

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb][test] Add a layout simulator test for std::unique_ptr #98330

[lldb][test] Add a layout simulator test for std::unique_ptr #98330

Michael137 commented Jul 10, 2024 •

edited

Loading

llvmbot commented Jul 10, 2024

Michael137 commented Jul 10, 2024 •

edited

Loading

github-actions bot commented Jul 10, 2024 •

edited

Loading

labath left a comment

labath Jul 11, 2024 •

edited

Loading

labath Jul 11, 2024 •

edited

Loading

Michael137 Jul 11, 2024

labath Jul 11, 2024

Michael137 Jul 11, 2024

Michael137 Jul 11, 2024

Michael137 commented Jul 12, 2024

[lldb][test] Add a layout simulator test for std::unique_ptr #98330

[lldb][test] Add a layout simulator test for std::unique_ptr #98330

Conversation

Michael137 commented Jul 10, 2024 • edited Loading

llvmbot commented Jul 10, 2024

Michael137 commented Jul 10, 2024 • edited Loading

github-actions bot commented Jul 10, 2024 • edited Loading

labath left a comment

Choose a reason for hiding this comment

labath Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

labath Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Michael137 Jul 11, 2024

Choose a reason for hiding this comment

labath Jul 11, 2024

Choose a reason for hiding this comment

Michael137 Jul 11, 2024

Choose a reason for hiding this comment

Michael137 Jul 11, 2024

Choose a reason for hiding this comment

Michael137 commented Jul 12, 2024

Michael137 commented Jul 10, 2024 •

edited

Loading

Michael137 commented Jul 10, 2024 •

edited

Loading

github-actions bot commented Jul 10, 2024 •

edited

Loading

labath Jul 11, 2024 •

edited

Loading

labath Jul 11, 2024 •

edited

Loading