Teach LLDB's pretty-printer about libc++'s various std::vector layouts - #202438
Conversation
PR llvm#155330 changes `std::vector` from unconditionally using three pointers to represent its layout to potentially using three pointers or a begin pointer and two integers. This commit changes LLDB so that it can robustly work with the legacy vector layout, the new pointer layout, and the new size-based layout.
|
@llvm/pr-subscribers-lldb Author: Christopher Di Bella (cjdb) ChangesPR #155330 changes Full diff: https://github.com/llvm/llvm-project/pull/202438.diff 6 Files Affected:
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp
index 95d12e8ee4f06..bf9010a304197 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp
@@ -37,6 +37,8 @@ class LibcxxStdVectorSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
llvm::Expected<size_t> GetIndexOfChildWithName(ConstString name) override;
private:
+ lldb::ChildCacheState UpdateVectorWithLayoutSubobject(ValueObject *layout);
+
ValueObject *m_start = nullptr;
ValueObject *m_finish = nullptr;
CompilerType m_element_type;
@@ -126,40 +128,50 @@ lldb_private::formatters::LibcxxStdVectorSyntheticFrontEnd::GetChildAtIndex(
m_element_type);
}
-static ValueObjectSP GetDataPointer(ValueObject &root) {
- auto [cap_sp, is_compressed_pair] =
- GetValueOrOldCompressedPair(root, "__cap_", "__end_cap_");
- if (!cap_sp)
- return nullptr;
-
- if (is_compressed_pair)
- return GetFirstValueOfLibCXXCompressedPair(*cap_sp);
-
- return cap_sp;
-}
-
lldb::ChildCacheState
lldb_private::formatters::LibcxxStdVectorSyntheticFrontEnd::Update() {
m_start = m_finish = nullptr;
- ValueObjectSP data_sp(GetDataPointer(m_backend));
- if (!data_sp)
+ // Determine if this version of libc++'s `std::vector` uses `__vector_layout`.
+ ValueObjectSP layout_sp = m_backend.GetChildMemberWithName("__layout_");
+ ValueObject *target = layout_sp ? layout_sp.get() : &m_backend;
+
+ ValueObjectSP begin_sp = target->GetChildMemberWithName("__begin_");
+ if (!begin_sp)
return lldb::ChildCacheState::eRefetch;
- m_element_type = data_sp->GetCompilerType().GetPointeeType();
+ m_element_type = begin_sp->GetCompilerType().GetPointeeType();
llvm::Expected<uint64_t> size_or_err = m_element_type.GetByteSize(nullptr);
- if (!size_or_err)
+ if (!size_or_err) {
LLDB_LOG_ERRORV(GetLog(LLDBLog::DataFormatters), size_or_err.takeError(),
"{0}");
- else {
- m_element_size = *size_or_err;
-
- if (m_element_size > 0) {
- // store raw pointers or end up with a circular dependency
- m_start = m_backend.GetChildMemberWithName("__begin_").get();
- m_finish = m_backend.GetChildMemberWithName("__end_").get();
- }
+ return lldb::ChildCacheState::eRefetch;
+ }
+
+ m_element_size = *size_or_err;
+ if (m_element_size == 0) {
+ return lldb::ChildCacheState::eRefetch;
}
+
+ // store raw pointers or end up with a circular dependency
+ m_start = begin_sp.get();
+
+ if (ValueObjectSP end_sp = target->GetChildMemberWithName("__end_")) {
+ m_finish = end_sp.get();
+ return lldb::ChildCacheState::eRefetch;
+ }
+
+ ValueObjectSP size_sp = target->GetChildMemberWithName("__size_");
+ if (!size_sp || !size_sp->GetCompilerType().IsInteger())
+ return lldb::ChildCacheState::eRefetch;
+
+ uint64_t begin_addr = m_start->GetValueAsUnsigned(0);
+ uint64_t size = size_sp->GetValueAsUnsigned(0);
+ uint64_t end_addr = begin_addr + size * m_element_size;
+ m_finish = CreateChildValueObjectFromAddress(
+ "__end_", end_addr, m_backend.GetExecutionContextRef(),
+ m_start->GetCompilerType(), false)
+ .get();
return lldb::ChildCacheState::eRefetch;
}
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/invalid-vector/TestDataFormatterLibcxxInvalidVectorSimulator.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/invalid-vector/TestDataFormatterLibcxxInvalidVectorSimulator.py
index c3d51a49c3f5b..e4783746b3499 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/invalid-vector/TestDataFormatterLibcxxInvalidVectorSimulator.py
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/invalid-vector/TestDataFormatterLibcxxInvalidVectorSimulator.py
@@ -37,5 +37,81 @@ def test(self):
)
self.expect(
"frame variable v5",
+ substrs=["size=error: failed to determine start/end of vector data"],
+ )
+ self.expect(
+ "frame variable v6",
+ substrs=["size=error: failed to determine start/end of vector data"],
+ )
+ self.expect(
+ "frame variable v7",
+ substrs=["size=error: invalid value for end of vector"],
+ )
+ self.expect(
+ "frame variable v8",
+ substrs=["size=error: failed to determine start/end of vector data"],
+ )
+ self.expect(
+ "frame variable v9",
+ substrs=["size=error: failed to determine start/end of vector data"],
+ )
+ self.expect(
+ "frame variable v10",
+ substrs=["size=error: invalid value for end of vector"],
+ )
+ self.expect(
+ "frame variable v11",
+ substrs=["size=error: invalid value for start of vector"],
+ )
+ self.expect(
+ "frame variable v12",
+ substrs=["size=error: start of vector data begins after end pointer"],
+ )
+ self.expect(
+ "frame variable v13",
+ substrs=["size=error: failed to determine start/end of vector data"],
+ )
+ self.expect(
+ "frame variable v14",
+ substrs=["size=error: invalid value for end of vector"],
+ )
+ self.expect(
+ "frame variable v15",
+ substrs=["size=1"],
+ )
+ self.expect(
+ "frame variable v16",
+ substrs=["size=error: failed to determine start/end of vector data"],
+ )
+ self.expect(
+ "frame variable v17",
+ substrs=["size=error: failed to determine start/end of vector data"],
+ )
+ self.expect(
+ "frame variable v18",
+ substrs=["size=error: failed to determine start/end of vector data"],
+ )
+ self.expect(
+ "frame variable v19",
substrs=["size=error: size not multiple of element size"],
)
+ self.expect(
+ "frame variable v20",
+ substrs=["size=error: size not multiple of element size"],
+ )
+ self.expect(
+ "frame variable v21",
+ substrs=["size=1"],
+ )
+ self.expect(
+ "frame variable v23",
+ substrs=["size=error: failed to determine start/end of vector data"],
+ )
+ self.expect(
+ "frame variable v24",
+ substrs=["size=error: failed to determine start/end of vector data"],
+ )
+ self.expect(
+ "frame variable v25",
+ substrs=["size=error: failed to determine start/end of vector data"],
+ )
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/invalid-vector/main.cpp b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/invalid-vector/main.cpp
index 5943b35deab8b..f7a7e13356557 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/invalid-vector/main.cpp
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/invalid-vector/main.cpp
@@ -1,37 +1,199 @@
#define COMPRESSED_PAIR_REV 4
#include <libcxx-simulators-common/compressed_pair.h>
+#include <stddef.h>
namespace std {
-inline namespace __1 {
+inline namespace __ValidLegacyVector {
template <typename T> struct vector {
T *__begin_;
T *__end_;
- _LLDB_COMPRESSED_PAIR(T *, __cap_ = nullptr, void *, __alloc_);
};
-} // namespace __1
+} // namespace __ValidLegacyVector
-inline namespace __2 {
-template <typename T> struct vector {};
-} // namespace __2
+inline namespace __LegacyVectorMissingBegin {
+template <typename T> struct vector {
+ T *__end_;
+};
+} // namespace __LegacyVectorMissingBegin
-inline namespace __3 {
+inline namespace __LegacyVectorNonPointerBegin {
template <typename T> struct vector {
+ int __begin_;
+ T *__end_;
+};
+} // namespace __LegacyVectorNonPointerBegin
+
+inline namespace __LegacyMissingEnd {
+template <typename T> struct vector {
+ T *__begin_;
+};
+} // namespace __LegacyMissingEnd
+
+inline namespace __LegacyVectorNonPointerEnd {
+template <typename T> struct vector {
+ T *__begin_;
+ size_t __end_;
+};
+} // namespace __LegacyVectorNonPointerEnd
+
+inline namespace __LegacyVectorSizeBased {
+template <typename T> struct vector {
+ T *__begin_;
+ size_t __end_;
+};
+} // namespace __LegacyVectorSizeBased
+
+inline namespace __ValidPointerLayout {
+template <typename T> struct __vector_layout {
T *__begin_;
T *__end_;
- _LLDB_COMPRESSED_PAIR(short *, __cap_ = nullptr, void *, __alloc_);
};
-} // namespace __3
+
+template <typename T> struct vector {
+ __vector_layout<T> __layout_;
+};
+} // namespace __ValidPointerLayout
+
+inline namespace __PointerLayoutNonPointerBegin {
+template <typename T> struct __vector_layout {
+ size_t __begin_;
+ T *__end_;
+};
+
+template <typename T> struct vector {
+ __vector_layout<T> __layout_;
+};
+} // namespace __PointerLayoutNonPointerBegin
+
+inline namespace __PointerLayoutNonPointerEnd {
+template <typename T> struct __vector_layout {
+ T *__begin_;
+ size_t __end_;
+};
+
+template <typename T> struct vector {
+ __vector_layout<T> __layout_;
+};
+} // namespace __PointerLayoutNonPointerEnd
+
+inline namespace __LayoutStructMissingBegin {
+template <typename T> struct __vector_layout {
+ // LLDB short-circuits when it can't find `__begin_`, so other members aren't
+ // required for this type.
+};
+
+template <typename T> struct vector {
+ __vector_layout<T> __layout_;
+};
+} // namespace __LayoutStructMissingBegin
+
+inline namespace __LayoutStructMissingSecondMember {
+template <typename T> struct __vector_layout {
+ T *__begin_;
+};
+
+template <typename T> struct vector {
+ __vector_layout<T> __layout_;
+};
+} // namespace __LayoutStructMissingSecondMember
+
+inline namespace __ValidSizeLayout {
+template <typename T> struct __vector_layout {
+ T *__begin_;
+ size_t __size_;
+};
+template <typename T> struct vector {
+ __vector_layout<T> __layout_;
+};
+} // namespace __ValidSizeLayout
+
+inline namespace __SizeLayoutMissingBegin {
+template <typename T> struct __vector_layout {
+ size_t __size_;
+};
+template <typename T> struct vector {
+ __vector_layout<T> __layout_;
+};
+} // namespace __SizeLayoutMissingBegin
+
+inline namespace __SizeLayoutNonPointerBegin {
+template <typename T> struct __vector_layout {
+ size_t __begin_;
+ size_t __size_;
+};
+template <typename T> struct vector {
+ __vector_layout<T> __layout_;
+};
+} // namespace __SizeLayoutNonPointerBegin
+
+inline namespace __SizeLayoutNonIntegerSize {
+template <typename T> struct __vector_layout {
+ T *__begin_;
+ T *__size_;
+};
+template <typename T> struct vector {
+ __vector_layout<T> __layout_;
+};
+} // namespace __SizeLayoutNonIntegerSize
} // namespace std
int main() {
int arr[] = {1, 2, 3};
- std::__1::vector<int> v1{.__begin_ = arr, .__end_ = nullptr};
- std::__1::vector<int> v2{.__begin_ = nullptr, .__end_ = arr};
- std::__1::vector<int> v3{.__begin_ = &arr[2], .__end_ = arr};
- std::__2::vector<int> v4;
+ std::__ValidLegacyVector::vector<int> v1{.__begin_ = arr, .__end_ = nullptr};
+ std::__ValidLegacyVector::vector<int> v2{.__begin_ = nullptr, .__end_ = arr};
+ std::__ValidLegacyVector::vector<int> v3{.__begin_ = &arr[2], .__end_ = arr};
+ std::__LegacyVectorMissingBegin::vector<int> v4{.__end_ = arr};
+ std::__LegacyMissingEnd::vector<int> v5{.__begin_ = arr};
+ std::__LegacyVectorNonPointerBegin::vector<int> v6{.__begin_ = 0,
+ .__end_ = arr};
+ std::__LegacyVectorNonPointerEnd::vector<int> v7{.__begin_ = arr,
+ .__end_ = 0};
+
+ std::__LayoutStructMissingBegin::vector<int> v8{.__layout_ = {}};
+ std::__LayoutStructMissingSecondMember::vector<int> v9{
+ .__layout_ = {.__begin_ = arr}};
+
+ std::__ValidPointerLayout::vector<int> v10{
+ .__layout_ = {.__begin_ = arr, .__end_ = nullptr}};
+ std::__ValidPointerLayout::vector<int> v11{
+ .__layout_ = {.__begin_ = nullptr, .__end_ = arr}};
+ std::__ValidPointerLayout::vector<int> v12{
+ .__layout_ = {.__begin_ = &arr[2], .__end_ = arr}};
+
+ std::__PointerLayoutNonPointerBegin::vector<int> v13{
+ .__layout_ = {.__begin_ = 0, .__end_ = arr}};
+ std::__PointerLayoutNonPointerEnd::vector<int> v14{
+ .__layout_ = {.__begin_ = arr, .__end_ = 0}};
+
+ std::__ValidSizeLayout::vector<int> v15{
+ .__layout_ = {.__begin_ = arr, .__size_ = 1}};
+
+ std::__SizeLayoutMissingBegin::vector<int> v16{.__layout_ = {.__size_ = 1}};
+ std::__SizeLayoutNonPointerBegin::vector<int> v17{
+ .__layout_ = {.__begin_ = 0, .__size_ = 0}};
+ std::__SizeLayoutNonIntegerSize::vector<int> v18{
+ .__layout_ = {.__begin_ = arr, .__size_ = 0}};
char carr[] = {'a'};
- std::__3::vector<char> v5{.__begin_ = carr, .__end_ = carr + 1};
+ std::__ValidLegacyVector::vector<short> v19{
+ .__begin_ = reinterpret_cast<short *>(carr),
+ .__end_ = reinterpret_cast<short *>(carr + 1)};
+ std::__ValidPointerLayout::vector<short> v20{
+ .__layout_ = {.__begin_ = reinterpret_cast<short *>(carr),
+ .__end_ = reinterpret_cast<short *>(carr + 1)}};
+ std::__ValidSizeLayout::vector<short> v21{
+ .__layout_ = {.__begin_ = reinterpret_cast<short *>(carr), .__size_ = 1}};
+
+ struct ZeroSizeStruct {
+ int x[0];
+ };
+ static_assert(sizeof(ZeroSizeStruct) == 0);
+ std::__ValidLegacyVector::vector<ZeroSizeStruct> v23{.__begin_ = nullptr,
+ .__end_ = nullptr};
+ std::__ValidPointerLayout::vector<ZeroSizeStruct> v24{
+ .__layout_ = {.__begin_ = nullptr, .__end_ = nullptr}};
+ std::__ValidSizeLayout::vector<ZeroSizeStruct> v25{
+ .__layout_ = {.__begin_ = nullptr, .__size_ = 0}};
return 0;
}
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/vector/Makefile b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/vector/Makefile
new file mode 100644
index 0000000000000..8ce653ffd6871
--- /dev/null
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/vector/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+override CXXFLAGS_EXTRAS += -std=c++11
+include Makefile.rules
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/vector/TestDataFormatterLibcxxVectorSimulator.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/vector/TestDataFormatterLibcxxVectorSimulator.py
new file mode 100644
index 0000000000000..46b22ad473191
--- /dev/null
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/vector/TestDataFormatterLibcxxVectorSimulator.py
@@ -0,0 +1,44 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class LibcxxVectorDataFormatterSimulatorTestCase(TestBase):
+ SHARED_BUILD_TESTCASE = False
+ NO_DEBUG_INFO_TESTCASE = True
+ test_cases = {
+ "LLDB_TEST_VECTOR_WITHOUT_LAYOUT_DATA_MEMBER": 0,
+ "LLDB_TEST_VECTOR_WITH_POINTER_LAYOUT": 1,
+ "LLDB_TEST_VECTOR_WITH_SIZE_LAYOUT": 2,
+ }
+
+ def _run_test(self, test_case):
+ cxxflags_extras = f"-DLLDB_TEST_CASE={test_case}"
+ self.build(dictionary=dict(CXXFLAGS_EXTRAS=cxxflags_extras))
+ lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.cpp"))
+
+ self.expect(
+ "frame variable v0",
+ substrs=["size=0"],
+ )
+ self.expect(
+ "frame variable v1",
+ substrs=["size=1", "[0] = 10"],
+ )
+ self.expect(
+ "frame variable v2",
+ substrs=["size=2", "[0] = -10", "[1] = -20"],
+ )
+ self.expect(
+ "frame variable v3",
+ substrs=["size=3", "[0] = 56", "[1] = 10", "[2] = 87"],
+ )
+
+ def test_without_layout_member(self):
+ self._run_test(self.test_cases["LLDB_TEST_VECTOR_WITHOUT_LAYOUT_DATA_MEMBER"])
+
+ def test_with_pointer_layout(self):
+ self._run_test(self.test_cases["LLDB_TEST_VECTOR_WITH_POINTER_LAYOUT"])
+
+ def test_with_size_layout(self):
+ self._run_test(self.test_cases["LLDB_TEST_VECTOR_WITH_SIZE_LAYOUT"])
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/vector/main.cpp b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/vector/main.cpp
new file mode 100644
index 0000000000000..9bfbddba63238
--- /dev/null
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/vector/main.cpp
@@ -0,0 +1,78 @@
+#include <stddef.h>
+
+#define LLDB_TEST_VECTOR_WITHOUT_LAYOUT_DATA_MEMBER 0
+#define LLDB_TEST_VECTOR_WITH_POINTER_LAYOUT 1
+#define LLDB_TEST_VECTOR_WITH_SIZE_LAYOUT 2
+#define LLDB_TEST_VECTOR_WITH_LAYOUT_MISSING_DATA_MEMBERS 3
+
+#ifndef LLDB_TEST_CASE
+#error LLDB_TEST_CASE must be defined as an integer
+#endif
+
+namespace std {
+namespace __lldb {
+
+#if LLDB_TEST_CASE == LLDB_TEST_VECTOR_WITHOUT_LAYOUT_DATA_MEMBER
+template <typename T> class vector {
+public:
+ typedef T *pointer;
+
+ vector(pointer begin, size_t size)
+ : __begin_(begin), __end_(begin + size) {}
+
+private:
+ pointer __begin_;
+ pointer __end_;
+ // __cap_ and __alloc_ aren't used, so they've been removed for simplicity.
+};
+#elif LLDB_TEST_CASE == LLDB_TEST_VECTOR_WITH_POINTER_LAYOUT
+template <typename T> struct __vector_layout {
+ T *__begin_;
+ T *__end_;
+};
+
+template <typename T> class vector {
+public:
+ vector(T *begin, size_t size) : __layout_{begin, begin + size} {}
+
+private:
+ __vector_layout<T> __layout_;
+};
+
+#elif LLDB_TEST_CASE == LLDB_TEST_VECTOR_WITH_SIZE_LAYOUT
+template <typename T> struct __vector_layout {
+ T *__begin_;
+ size_t __size_;
+};
+
+template <typename T> class vector {
+public:
+ vector(T *begin, size_t size) : __layout_{begin, size} {}
+
+private:
+ __vector_layout<T> __layout_;
+};
+
+#else
+#error LLDB_TEST_CASE defined out-of-range
+#undef LLDB_TEST_CASE
+#endif
+
+} // namespace __lldb
+} // namespace std
+
+int main() {
+#ifdef LLDB_TEST_CASE
+ int a1[] = {10};
+ std::__lldb::vector<int> v0(a1, 0);
+ std::__lldb::vector<int> v1(a1, 1);
+
+ int a2[] = {-10, -20};
+ std::__lldb::vector<int> v2(a2, 2);
+
+ int a3[] = {56, 10, 87};
+ std::__lldb::vector<int> v3(a3, 3);
+
+ return 0; // break here
+#endif
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
✅ With the latest revision this PR passed the Python code formatter. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
labath
left a comment
labath
left a comment
There was a problem hiding this comment.
I think it would be better (unless I'm missing something) to reverse the logic in the implementation (see inline comment), but otherwise, the patch seems straight-forward enough. I like the extra test coverage.
| m_finish = CreateChildValueObjectFromAddress( | ||
| "__end_", end_addr, m_backend.GetExecutionContextRef(), | ||
| m_start->GetCompilerType(), false) | ||
| .get(); |
There was a problem hiding this comment.
What would you say to doing this the other way around -- instead of creating a virtual m_finish object, have both paths populate a uint64_t m_size member (or `optional<uint64_t>, or something like that)?
The reason I'm thinking about this is that the size-based std::vector is actually a better match for how lldb data formatters work internally, so I think it would be better to standardize on that, rather than having LibcxxStdVectorSyntheticFrontEnd::CalculateNumChildren effectively undo this transformation.
There was a problem hiding this comment.
I ended up writing it this way because I suspect most code will want either a legacy vector or a pointer-based layout, not a size-based layout, and figured it would be best to prioritise that case absent performance data.
I like the code shape you're asking for, but do you think it'll have any tangible performance impact on the common use-case?
There was a problem hiding this comment.
I don't expect any impact on performance here (in either direction). I'm thinking mainly about code readability here. I think that using the size-based approach will result in less code overall.
It's true that the two-pointer representation is going to be the majority case, but I don't think that matters much since we have to maintain both version anyway. And I wouldn't be surprised if, even without the size-based vector representation, a size-based formatter would be more readable.
| std::__LegacyLayout::vector<int> legacy_layout0(a1, 0); | ||
| std::__LegacyLayout::vector<int> legacy_layout1(a1, 1); | ||
| std::__LegacyLayout::vector<int> legacy_layout2(a2, 2); | ||
| std::__LegacyLayout::vector<int> legacy_layout3(a3, 3); | ||
|
|
||
| std::__PointerBasedLayout::vector<int> pointer_based_layout0(a1, 0); | ||
| std::__PointerBasedLayout::vector<int> pointer_based_layout1(a1, 1); | ||
| std::__PointerBasedLayout::vector<int> pointer_based_layout2(a2, 2); | ||
| std::__PointerBasedLayout::vector<int> pointer_based_layout3(a3, 3); | ||
|
|
||
| std::__SizeBasedLayout::vector<int> size_based_layout0(a1, 0); | ||
| std::__SizeBasedLayout::vector<int> size_based_layout1(a1, 1); | ||
| std::__SizeBasedLayout::vector<int> size_based_layout2(a2, 2); | ||
| std::__SizeBasedLayout::vector<int> size_based_layout3(a3, 3); |
There was a problem hiding this comment.
This is a nice way of iterating the layouts 👍
(maybe we should do that for our other simulators as opposed to the #ifdefs :))
There was a problem hiding this comment.
Thanks! I had a tough time reading the #ifdefs and __1, __2, etc. :)
| private: | ||
| pointer __begin_; | ||
| pointer __end_; | ||
| // __cap_ and __alloc_ aren't used, so they've been removed for simplicity. |
There was a problem hiding this comment.
Lets be faithful to the legacy layout and add the compressed pair here
There was a problem hiding this comment.
What does re-adding the compressed pair preserve?
The new pretty-printer is only interested in the first two members, so I'm not sure what the goal of adding unused members is. We also end up with duplicate test cases, since I'll need to add both compressed pair cases for both pointer layouts:
__LegacyLayoutWithCompressedPairMacrowill be testing the same thing as__LegacyLayoutWithCompressedPairStruct__PointerBasedLayoutWithCompressedMacrowill be testing the same thing as__PointerBasedLayoutWithCompressedStruct
There was a problem hiding this comment.
After re-reading https://github.com/llvm/llvm-project/pull/202438/changes#r3378801839, it sounds to me like you're concerned about losing [[no_unique_address]] coverage. Is that correct?
If that's the case, would you be okay with me adding a dedicated [[no_unique_address]] test instead? Based on the information that I have, I can see two advantages:
- It decouples the
[[no_unique_address]]test from the vector test, which simplifies both tests. - It ensures that LLDB is testing
[[no_unique_address]](which will not happen in vector tests).
There was a problem hiding this comment.
If that's the case, would you be okay with me adding a dedicated [[no_unique_address]] test instead?
There's already no_unique_address tests for the other STL containers (and also dedicated ones in the API test suite). So there's no need for special testing of this.
I just wanted the LegacyLayout to reflect the layout that std::vector used to have. For documentation/consistency purposes. I understand that the class itself is a minimally stripped version of the old layout, but with all the other simulator tests we copy all the members over. I like doing that because it accurately gives us an audit trail of how the layout changed over time without having to trawl through the libc++ git log.
There was a problem hiding this comment.
@Michael137 is there any action to take here before merging?
There was a problem hiding this comment.
I guess its a bit weird to add LLDB_COMPRESSED_PAIR here. Since we're not setting the LLDB_COMPRESSED_PAIR_REV (or whatever its called).
I don't feel super strongly about it, so its fine if you want to leave it out
There was a problem hiding this comment.
Oops, it looks like GitHub auto-refreshes only part of the page. Your LGTM below was added, but not two messages up (I thought you hadn't replied at all). Sorry about that.
I just wanted the
LegacyLayoutto reflect the layout thatstd::vectorused to have. For documentation/consistency purposes. I understand that the class itself is a minimally stripped version of the old layout, but with all the other simulator tests we copy all the members over. I like doing that because it accurately gives us an audit trail of how the layout changed over time without having to trawl through the libc++ git log.
...
I guess its a bit weird to addLLDB_COMPRESSED_PAIRhere. Since we're not setting theLLDB_COMPRESSED_PAIR_REV(or whatever its called).
I've added a comment block to help with auditing.
labath
left a comment
labath
left a comment
There was a problem hiding this comment.
I like this.
What I might try is making m_finish a PointerIntPair -- not because I worry about the extra space, but because makes it harder to forget that the interpretation of that object depends on the vector mode.
@Michael137, I'll let you approve once your comments are addressed.
Michael137
left a comment
Michael137
left a comment
There was a problem hiding this comment.
Looks good, thanks!
(I agree with Pavel re. PointerIntPair, if we can pull it of. Or at least add a brief comment next to m_finish to explain that it can either be a pointer or a scalar size)
Naming this member has been the bane of my existence for over a year. Would you like me to tackle that in this PR, or should I just add a comment for now? |
Fair, yes a comment would be great. Feel free to merge once that's done |
3 participants
PR #155330 changes
std::vectorfrom unconditionally using three pointers to represent its layout to potentially using three pointers or a begin pointer and two integers. This commit changes LLDB so that it can robustly work with the legacy vector layout, the new pointer layout, and the new size-based layout.