[DataFormatter] Fix variant npos with `_LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION` enabled.

[rupprecht]

Summary

This data formatter should print "No Value" if a variant is unset. It does so by checking if __index has a value of -1, however it does so by interpreting it as a signed int.

By default, __index has type unsigned int. When _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION is enabled, the type of __index is either unsigned char, unsigned short, or unsigned int, depending on how many fields there are -- as small as possible. For example, when std::variant has only a few types, the index type is unsigned char, and the npos value will be interpreted by LLDB as 255 when it should be -1.

This change does not special case the variant optimization; it just reads the type instead of assuming it's unsigned int.

[labath]

lldb/source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp

74

I'd probably use fixed with types (uint8_t et al.) here.

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/variant/main.cpp

32

Maybe just add a quick note that the number 300 was chosen because it does not fit into a single byte.

[rupprecht]

• Add comment about choice of 300 types
• Use fixed-width types

[rupprecht]

I plan to land this now so we can remove this from our internal test exclusion list, but I'd be happy to address any post-commit feedback from other reviewers as a followup change.

lldb/source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp

 //===-- LibCxxVariant.cpp -------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "LibCxxVariant.h"
 #include "LibCxx.h"
 #include "lldb/DataFormatters/FormattersHelpers.h"
+#include "lldb/Symbol/CompilerType.h"
+#include "lldb/Utility/LLDBAssert.h"
 
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/ScopeExit.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 // libc++ variant implementation contains two members that we care about both

 
 namespace {
 // libc++ std::variant index could have one of three states
 // 1) Valid, we can obtain it and its not variant_npos
 // 2) Invalid, we can't obtain it or it is not a type we expect
 // 3) NPos, its value is variant_npos which means the variant has no value
 enum class LibcxxVariantIndexValidity { Valid, Invalid, NPos };
 
+uint64_t VariantNposValue(uint64_t index_byte_size) {
+  switch (index_byte_size) {
+  case 1:
+    return static_cast<uint8_t>(-1);
+  case 2:
+    return static_cast<uint16_t>(-1);
+  case 4:
+    return static_cast<uint32_t>(-1);
+  }
+  lldbassert(false && "Unknown index type size");
+  return static_cast<uint32_t>(-1); // Fallback to stable ABI type.
+}
+
 LibcxxVariantIndexValidity
 LibcxxVariantGetIndexValidity(ValueObjectSP &impl_sp) {
   ValueObjectSP index_sp(
       impl_sp->GetChildMemberWithName(ConstString("__index"), true));
 
   if (!index_sp)
     return LibcxxVariantIndexValidity::Invalid;
 
-  int64_t index_value = index_sp->GetValueAsSigned(0);
+  // In the stable ABI, the type of __index is just int.
+  // In the unstable ABI, where _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION is
+  // enabled, the type can either be unsigned char/short/int depending on
+  // how many variant types there are.
+  // We only need to do this here when comparing against npos, because npos is
+  // just `-1`, but that translates to different unsigned values depending on
+  // the byte size.
+  CompilerType index_type = index_sp->GetCompilerType();
+
+  llvm::Optional<uint64_t> index_type_bytes = index_type.GetByteSize(nullptr);
+  if (!index_type_bytes)
+    return LibcxxVariantIndexValidity::Invalid;
+
+  uint64_t npos_value = VariantNposValue(*index_type_bytes);
+  uint64_t index_value = index_sp->GetValueAsUnsigned(0);
 
-  if (index_value == -1)
+  if (index_value == npos_value)
     return LibcxxVariantIndexValidity::NPos;
 
   return LibcxxVariantIndexValidity::Valid;
 }
 
 llvm::Optional<uint64_t> LibcxxVariantIndexValue(ValueObjectSP &impl_sp) {
   ValueObjectSP index_sp(
       impl_sp->GetChildMemberWithName(ConstString("__index"), true));

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/variant/TestDataFormatterLibcxxVariant.py

def test_with_run_command(self):
 
         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_300_types_no_value",
+                    substrs=['v_300_types_no_value =  No Value'])

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/variant/main.cpp

 
 #if HAVE_VARIANT == 1
     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 300 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,
+        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_300_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) ;
#if HAVE_VARIANT == 1
     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_300_types_no_value.emplace<0>(S());
+     } catch (...) {
+     }
+
+     printf("%zu\n", v_300_types_no_value.index());
 #endif
 
     return 0; // break here
 }