[lldb] Make variant formatter work with libstdc++-14 #97568
Conversation
In this version the internal data member has grown an additional template parameter (bool), which was throwing the summary provider off. This patch uses the type of the entire variant object. This is part of the API/ABI, so it should be more stable, but it means we have to explicitly strip typedefs and references to get to the interesting bits, which is why I've extended the test case with examples of those.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesIn this version the internal data member has grown an additional template parameter (bool), which was throwing the summary provider off. This patch uses the type of the entire variant object. This is part of the API/ABI, so it should be more stable, but it means we have to explicitly strip typedefs and references to get to the interesting bits, which is why I've extended the test case with examples of those. Full diff: https://github.com/llvm/llvm-project/pull/97568.diff 3 Files Affected:
diff --git a/lldb/examples/synthetic/gnu_libstdcpp.py b/lldb/examples/synthetic/gnu_libstdcpp.py
index f778065aaca37..59970574a3604 100644
--- a/lldb/examples/synthetic/gnu_libstdcpp.py
+++ b/lldb/examples/synthetic/gnu_libstdcpp.py
@@ -914,12 +914,15 @@ def get_variant_npos_value(index_byte_size):
if index == npos_value:
return " No Value"
+ # Strip references and typedefs.
+ variant_type = raw_obj.GetType().GetCanonicalType().GetDereferencedType();
+ template_arg_count = variant_type.GetNumberOfTemplateArguments()
+
# Invalid index can happen when the variant is not initialized yet.
- template_arg_count = data_obj.GetType().GetNumberOfTemplateArguments()
if index >= template_arg_count:
return " <Invalid>"
- active_type = data_obj.GetType().GetTemplateArgumentType(index)
+ active_type = variant_type.GetTemplateArgumentType(index)
return f" Active Type = {active_type.GetDisplayTypeName()} "
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/variant/TestDataFormatterLibStdcxxVariant.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/variant/TestDataFormatterLibStdcxxVariant.py
index ba1641888b6f3..05f31087a566e 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/variant/TestDataFormatterLibStdcxxVariant.py
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/variant/TestDataFormatterLibStdcxxVariant.py
@@ -21,15 +21,17 @@ def test_with_run_command(self):
lldbutil.continue_to_breakpoint(self.process, bkpt)
- self.expect(
- "frame variable v1",
- substrs=["v1 = Active Type = int {", "Value = 12", "}"],
- )
-
- self.expect(
- "frame variable v1_ref",
- substrs=["v1_ref = Active Type = int : {", "Value = 12", "}"],
- )
+ for name in ["v1", "v1_typedef"]:
+ self.expect(
+ "frame variable " + name,
+ substrs=[name + " = Active Type = int {", "Value = 12", "}"],
+ )
+
+ for name in ["v1_ref", "v1_typedef_ref"]:
+ self.expect(
+ "frame variable " + name,
+ substrs=[name + " = Active Type = int : {", "Value = 12", "}"],
+ )
self.expect(
"frame variable v_v1",
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/variant/main.cpp b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/variant/main.cpp
index 545318f9358b6..36e0f74f831f8 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/variant/main.cpp
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/variant/main.cpp
@@ -14,6 +14,10 @@ int main() {
std::variant<int, double, char> v1;
std::variant<int, double, char> &v1_ref = v1;
+ using V1_typedef = std::variant<int, double, char>;
+ V1_typedef v1_typedef;
+ V1_typedef &v1_typedef_ref = v1_typedef;
+
std::variant<int, double, char> v2;
std::variant<int, double, char> v3;
std::variant<std::variant<int, double, char>> v_v1;
@@ -43,6 +47,7 @@ int main() {
v_many_types_no_value;
v1 = 12; // v contains int
+ v1_typedef = v1;
v_v1 = v1;
int i = std::get<int>(v1);
printf("%d\n", i); // break here
|
|
✅ With the latest revision this PR passed the Python code formatter. |
There was a problem hiding this comment.
LGTM
Been wondering if we should follow the same principle as we did with std::string and simulate the layouts for all the STL types, keeping a record of them as they change. So we know we don't break the old layouts. Of course it has a separate maintenance cost
Yeah, I think the biggest hurdle is the initial work in getting a stripped down version of the class in place. The problem is that the container types use a lot of template metaprogramming to implement various optimizations like empty base class and whatnot. Copying that verbatim would make the test huge (and in the case of libstdc++ its probably not even possible), and recreating it from scratch is not trivial. I believe that after I wrote the std::string test this way I tried to also do the same for some of the container types (it may have actually been variant), and just gave up after I saw hold long it would take. Nonetheless, I think there's definitely value in tests like this (for one, going through the implementation in this way makes it obvious how many corner cases need testing), and it's nice to see there's interest for these tests. I'll keep that in mind when working on future changes. |
Agreed, they can get pretty unwieldy. I'll see what the |
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)
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)
In this version the internal data member has grown an additional template parameter (bool), which was throwing the summary provider off.
This patch uses the type of the entire variant object. This is part of the API/ABI, so it should be more stable, but it means we have to explicitly strip typedefs and references to get to the interesting bits, which is why I've extended the test case with examples of those.