Implement data formatters for LibStdC++ std::variant #68012
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeffreytan81
requested review from
clayborg,
kusmour,
jimingham,
GeorgeHuyubo,
jdoerfert and
JDevlieghere
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@llvm/pr-subscribers-lldb ChangesThis patch implements the data formatters for LibStdC++ Full diff: https://github.com/llvm/llvm-project/pull/68012.diff 5 Files Affected:
diff --git a/lldb/examples/synthetic/gnu_libstdcpp.py b/lldb/examples/synthetic/gnu_libstdcpp.py
index 825b7f3787a010d..29c926167fb440c 100644
--- a/lldb/examples/synthetic/gnu_libstdcpp.py
+++ b/lldb/examples/synthetic/gnu_libstdcpp.py
@@ -892,3 +892,87 @@ def update(self):
except:
pass
return False
+
+
+def VariantSummaryProvider(valobj, dict):
+ raw_obj = valobj.GetNonSyntheticValue()
+ index_obj = raw_obj.GetChildMemberWithName("_M_index")
+ data_obj = raw_obj.GetChildMemberWithName("_M_u")
+ if not (index_obj and index_obj.IsValid() and data_obj and data_obj.IsValid()):
+ return "<Can't find _M_index or _M_u>"
+
+ def get_variant_npos_value(index_byte_size):
+ if index_byte_size == 1:
+ return 0xFF
+ elif index_byte_size == 2:
+ return 0xFFFF
+ else:
+ return 0xFFFFFFFF
+
+ npos_value = get_variant_npos_value(index_obj.GetByteSize())
+ index = index_obj.GetValueAsUnsigned(0)
+ if index == npos_value:
+ return " No Value"
+
+ active_type = data_obj.GetType().GetTemplateArgumentType(index)
+ return f" Active Type = {active_type.GetDisplayTypeName()} "
+
+
+class VariantSynthProvider:
+ def __init__(self, valobj, dict):
+ self.raw_obj = valobj.GetNonSyntheticValue()
+ self.is_valid = False
+ self.index = None
+ self.data_obj = None
+
+ def update(self):
+ try:
+ self.index = self.raw_obj.GetChildMemberWithName(
+ "_M_index"
+ ).GetValueAsSigned(-1)
+ self.is_valid = self.index != -1
+ self.data_obj = self.raw_obj.GetChildMemberWithName("_M_u")
+ except:
+ self.is_valid = False
+ return False
+
+ def has_children(self):
+ return True
+
+ def num_children(self):
+ return 1 if self.is_valid else 0
+
+ def get_child_index(self, name):
+ return 0
+
+ def get_child_at_index(self, index):
+ if not self.is_valid:
+ return None
+ cur = 0
+ node = self.data_obj
+ while cur < self.index:
+ node = node.GetChildMemberWithName("_M_rest")
+ cur += 1
+
+ # _M_storage's type depends on variant field's type "_Type".
+ # 1. if '_Type' is literal type: _Type _M_storage.
+ # 2. otherwise, __gnu_cxx::__aligned_membuf<_Type> _M_storage.
+ #
+ # For 2. we have to cast it to underlying template _Type.
+
+ value = node.GetChildMemberWithName("_M_first").GetChildMemberWithName(
+ "_M_storage"
+ )
+ template_type = value.GetType().GetTemplateArgumentType(0)
+
+ # Literal type will return None for GetTemplateArgumentType(0)
+ if (
+ template_type
+ and "__gnu_cxx::__aligned_membuf" in value.GetType().GetDisplayTypeName()
+ and template_type.IsValid()
+ ):
+ value = value.Cast(template_type)
+
+ if value.IsValid():
+ return value.Clone("Value")
+ return None
diff --git a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
index c1743a5e0a418dd..a285864ca2e1229 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -332,11 +332,11 @@ bool CPlusPlusLanguage::MethodName::ContainsPath(llvm::StringRef path) {
// If we can't parse the incoming name, then just check that it contains path.
if (m_parse_error)
return m_full.GetStringRef().contains(path);
-
+
llvm::StringRef identifier;
llvm::StringRef context;
std::string path_str = path.str();
- bool success
+ bool success
= CPlusPlusLanguage::ExtractContextAndIdentifier(path_str.c_str(),
context,
identifier);
@@ -372,7 +372,7 @@ bool CPlusPlusLanguage::MethodName::ContainsPath(llvm::StringRef path) {
return false;
if (haystack.empty() || !isalnum(haystack.back()))
return true;
-
+
return false;
}
@@ -388,7 +388,7 @@ bool CPlusPlusLanguage::IsCPPMangledName(llvm::StringRef name) {
return true;
}
-bool CPlusPlusLanguage::DemangledNameContainsPath(llvm::StringRef path,
+bool CPlusPlusLanguage::DemangledNameContainsPath(llvm::StringRef path,
ConstString demangled) const {
MethodName demangled_name(demangled);
return demangled_name.ContainsPath(path);
@@ -1104,6 +1104,11 @@ static void LoadLibStdcppFormatters(lldb::TypeCategoryImplSP cpp_category_sp) {
SyntheticChildrenSP(new ScriptedSyntheticChildren(
stl_synth_flags,
"lldb.formatters.cpp.gnu_libstdcpp.StdForwardListSynthProvider")));
+ cpp_category_sp->AddTypeSynthetic(
+ "^std::variant<.+>$", eFormatterMatchRegex,
+ SyntheticChildrenSP(new ScriptedSyntheticChildren(
+ stl_synth_flags,
+ "lldb.formatters.cpp.gnu_libstdcpp.VariantSynthProvider")));
stl_summary_flags.SetDontShowChildren(false);
stl_summary_flags.SetSkipPointers(false);
@@ -1148,6 +1153,11 @@ static void LoadLibStdcppFormatters(lldb::TypeCategoryImplSP cpp_category_sp) {
TypeSummaryImplSP(new ScriptSummaryFormat(
stl_summary_flags,
"lldb.formatters.cpp.gnu_libstdcpp.ForwardListSummaryProvider")));
+ cpp_category_sp->AddTypeSummary(
+ "^std::variant<.+>$", eFormatterMatchRegex,
+ TypeSummaryImplSP(new ScriptSummaryFormat(
+ stl_summary_flags,
+ "lldb.formatters.cpp.gnu_libstdcpp.VariantSummaryProvider")));
AddCXXSynthetic(
cpp_category_sp,
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/variant/Makefile b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/variant/Makefile
new file mode 100644
index 000000000000000..104f82809c7a35b
--- /dev/null
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/variant/Makefile
@@ -0,0 +1,5 @@
+CXX_SOURCES := main.cpp
+
+USE_LIBSTDCPP := 1
+CXXFLAGS_EXTRAS := -std=c++17
+include Makefile.rules
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
new file mode 100644
index 000000000000000..88be87a5469e196
--- /dev/null
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/variant/TestDataFormatterLibStdcxxVariant.py
@@ -0,0 +1,72 @@
+"""
+Test lldb data formatter for LibStdC++ std::variant.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+
+
+class LibStdcxxVariantDataFormatterTestCase(TestBase):
+ @add_test_categories(["libstdcxx"])
+ def test_with_run_command(self):
+ """Test LibStdC++ std::variant data formatter works correctly."""
+ self.build(dictionary={USE_LIBSTDCPP: "1"})
+
+ (self.target, self.process, _, bkpt) = lldbutil.run_to_source_breakpoint(
+ self, "// break here", lldb.SBFileSpec("main.cpp", False)
+ )
+
+ lldbutil.continue_to_breakpoint(self.process, bkpt)
+ self.assertEqual(3 + 4, 7)
+
+ 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", "}"],
+ )
+
+ self.expect(
+ "frame variable v_v1",
+ substrs=[
+ "v_v1 = Active Type = std::variant<int, double, char> {",
+ "Value = Active Type = int {",
+ "Value = 12",
+ "}",
+ "}",
+ ],
+ )
+
+ lldbutil.continue_to_breakpoint(self.process, bkpt)
+
+ self.expect(
+ "frame variable v1",
+ substrs=["v1 = Active Type = double {", "Value = 2", "}"],
+ )
+
+ lldbutil.continue_to_breakpoint(self.process, bkpt)
+
+ self.expect(
+ "frame variable v2",
+ substrs=["v2 = Active Type = double {", "Value = 2", "}"],
+ )
+
+ self.expect(
+ "frame variable v3",
+ substrs=["v3 = Active Type = char {", "Value = 'A'", "}"],
+ )
+
+ self.expect("frame variable v_no_value", substrs=["v_no_value = No Value"])
+
+ self.expect(
+ "frame variable v_many_types_no_value",
+ substrs=["v_many_types_no_value = No Value"],
+ )
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
new file mode 100644
index 000000000000000..545318f9358b673
--- /dev/null
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/variant/main.cpp
@@ -0,0 +1,79 @@
+#include <cstdio>
+#include <string>
+#include <variant>
+#include <vector>
+
+struct S {
+ operator int() { throw 42; }
+};
+
+int main() {
+ bool has_variant = true;
+
+ printf("%d\n", has_variant); // break here
+
+ std::variant<int, double, char> v1;
+ std::variant<int, double, char> &v1_ref = v1;
+ std::variant<int, double, char> v2;
+ std::variant<int, double, char> v3;
+ std::variant<std::variant<int, double, char>> v_v1;
+ std::variant<int, double, char> v_no_value;
+ // The next variant has many types, meaning the type index does not fit in
+ // a byte and must be `unsigned short` instead of `unsigned char` when
+ // using the unstable libc++ ABI. With stable libc++ ABI, the type index
+ // is always just `unsigned int`.
+ std::variant<
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int, int, int, int,
+ int, int, int, int, int, int, int, int, int, int, int, int>
+ v_many_types_no_value;
+
+ v1 = 12; // v contains int
+ v_v1 = v1;
+ int i = std::get<int>(v1);
+ printf("%d\n", i); // break here
+
+ v2 = 2.0;
+ double d = std::get<double>(v2);
+ printf("%f\n", d);
+
+ v3 = 'A';
+ char c = std::get<char>(v3);
+ printf("%d\n", c);
+
+ // Checking v1 above and here to make sure we done maintain the incorrect
+ // state when we change its value.
+ v1 = 2.0;
+ d = std::get<double>(v1);
+ printf("%f\n", d); // break here
+
+ try {
+ v_no_value.emplace<0>(S());
+ } catch (...) {
+ }
+
+ printf("%zu\n", v_no_value.index());
+
+ try {
+ v_many_types_no_value.emplace<0>(S());
+ } catch (...) {
+ }
+
+ printf("%zu\n", v_many_types_no_value.index());
+
+ return 0; // break here
+}
|
|
LGTM with some comments. |
medismailben
approved these changes
Oct 2, 2023
| @add_test_categories(["libstdcxx"]) | ||
| def test_with_run_command(self): | ||
| """Test LibStdC++ std::variant data formatter works correctly.""" | ||
| self.build(dictionary={USE_LIBSTDCPP: "1"}) |
There was a problem hiding this comment.
Do you need to specify this since you already set USE_LIBSTDCPP in the Makefile ?
There was a problem hiding this comment.
Good point. Will remove.
| ) | ||
|
|
||
| lldbutil.continue_to_breakpoint(self.process, bkpt) | ||
| self.assertEqual(3 + 4, 7) |
There was a problem hiding this comment.
lol, this is an old diff that I drafted several months ago so totally forgot why I added this in the first place :-)
|
Hello, |
|
Thanks for the heads-up. I have done some investigation. The test passes on my CentOS Linux and Macbook. It is unclear why it fails in this two bots.
|
medismailben
pushed a commit
that referenced
this pull request
Oct 3, 2023
#68012 works on my CentOS Linux and Macbook but seems to fail for certain build bots. The error log complains "No Value" check failure for `std::variant` but not very actionable without a reproduce. To unblock the build bots, I am commenting out the "No Value" checks. Co-authored-by: jeffreytan81 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch implements the data formatters for LibStdC++
std::variant.