[LLDB] Fix MS STL variant with non-trivial types

[Nerixyz]

When using std::variant with non-trivial types, we need to go through multiple bases to find the _Which member. The MSVC STL implements this in xsmf_control.h which conditionally adds/deletes copy/move constructors/operators.

We now go to _Variant_base (the holder of _Which). This inherits from _Variant_storage, which is our entry point to finding the n-th storage (going through _Tail).

[llvmbot]

@llvm/pr-subscribers-lldb

Author: nerix (Nerixyz)

Changes

When using std::variant with non-trivial types, we need to go through multiple bases to find the _Which member. The MSVC STL implements this in xsmf_control.h which conditionally adds/deletes copy/move constructors/operators.

We now go to _Variant_base (the holder of _Which). This inherits from _Variant_storage, which is our entry point to finding the n-th storage (going through _Tail).

For the summary, we also need to get the active type from the head type when running with PDB.

---

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

3 Files Affected:

• (modified) lldb/source/Plugins/Language/CPlusPlus/MsvcStlVariant.cpp (+18-7)
• (modified) lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/variant/TestDataFormatterStdVariant.py (+23)
• (modified) lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/variant/main.cpp (+5)

diff --git a/lldb/source/Plugins/Language/CPlusPlus/MsvcStlVariant.cpp b/lldb/source/Plugins/Language/CPlusPlus/MsvcStlVariant.cpp
index 52a3d98d2af4b..3391ca33a9277 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/MsvcStlVariant.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/MsvcStlVariant.cpp
@@ -67,12 +67,18 @@ std::optional<int64_t> GetIndexValue(ValueObject &valobj) {
 ValueObjectSP GetNthStorage(ValueObject &outer, int64_t index) {
   // We need to find the std::_Variant_storage base class.
 
-  // -> std::_SMF_control (typedef to std::_Variant_base)
-  ValueObjectSP container_sp = outer.GetSP()->GetChildAtIndex(0);
-  if (!container_sp)
+  // Navigate "down" to std::_Variant_base by finding the holder of "_Which".
+  // This might be down a few levels if a variant member isn't trivially
+  // destructible/copyable/etc.
+  ValueObjectSP which_sp = outer.GetChildMemberWithName("_Which");
+  if (!which_sp)
+    return nullptr;
+  ValueObject *parent = which_sp->GetParent();
+  if (!parent)
     return nullptr;
-  // -> std::_Variant_storage
-  container_sp = container_sp->GetChildAtIndex(0);
+
+  // Now go to std::_Variant_storage
+  ValueObjectSP container_sp = parent->GetChildAtIndex(0);
   if (!container_sp)
     return nullptr;
 
@@ -119,8 +125,13 @@ bool formatters::MsvcStlVariantSummaryProvider(
     storage_type = storage_type.GetTypedefedType();
 
   CompilerType active_type = storage_type.GetTypeTemplateArgument(1, true);
-  if (!active_type)
-    return false;
+  if (!active_type) {
+    // PDB: get the type from the head as we don't have template arguments there
+    ValueObjectSP head = GetHead(*storage);
+    active_type = head->GetCompilerType();
+    if (!active_type)
+      return false;
+  }
 
   stream << " Active Type = " << active_type.GetDisplayTypeName() << " ";
   return true;
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/variant/TestDataFormatterStdVariant.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/variant/TestDataFormatterStdVariant.py
index 9f32ad97c1f0a..d5c4f5c34cfe0 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/variant/TestDataFormatterStdVariant.py
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/variant/TestDataFormatterStdVariant.py
@@ -9,6 +9,8 @@
 
 
 class StdVariantDataFormatterTestCase(TestBase):
+    TEST_WITH_PDB_DEBUG_INFO = True
+
     def do_test(self):
         """Test that that file and class static variables display correctly."""
 
@@ -48,6 +50,14 @@ def cleanup():
             ],
         )
 
+        self.expect_expr(
+            "v4",
+            result_summary=" Active Type = int ",
+            result_children=[
+                ValueCheck(name="Value", value="4"),
+            ],
+        )
+
         lldbutil.continue_to_breakpoint(self.process, bkpt)
 
         self.expect(
@@ -67,6 +77,19 @@ def cleanup():
             substrs=["v3 =  Active Type = char  {", "Value = 'A'", "}"],
         )
 
+        string_name = (
+            "std::basic_string<char, std::char_traits<char>, std::allocator<char>>"
+            if self.getDebugInfo() == "pdb"
+            else "std::basic_string<char>"
+        )
+        self.expect_expr(
+            "v4",
+            result_summary=f" Active Type = {string_name} ",
+            result_children=[
+                ValueCheck(name="Value", summary='"a string"'),
+            ],
+        )
+
         self.expect("frame variable v_valueless", substrs=["v_valueless =  No Value"])
 
         self.expect(
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/variant/main.cpp b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/variant/main.cpp
index 620b97b7306f9..9983104ca9628 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/variant/main.cpp
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/variant/main.cpp
@@ -49,6 +49,8 @@ int main() {
       S>
       v_300_types_valueless;
 
+  std::variant<int, bool, std::string> v4 = 4;
+
   v_valueless = 5;
   v_300_types_valueless.emplace<0>(10);
 
@@ -70,6 +72,9 @@ int main() {
   // state when we change its value.
   v1 = 2.0;
   d = std::get<double>(v1);
+
+  v4 = "a string";
+
   printf("%f\n", d); // break here
 
   try {

[JDevlieghere]

LGTM. Michael's more versed in this stuff, but this seems pretty straightforward.

[Michael137]

Is this related to making it run with PDB? Or is this about handling more kinds of variant layouts in general? Is that where the additional test-case comes from? If it's the latter I'd prefer it if we split it into a separate PR

[Michael137]

Oh the title of this PR answers my question. Yea probably better to do the two changes separately