[lldb] Adjust libc++ string formatter for changes in D123580

[labath]

Summary

The code needs more TLC, but for now I've tried making only the changes
that are necessary to get the tests passing -- postponing the more
invasive changes after I create a more comprehensive test.

In a couple of places I have changed the index-based element accesses to
name-based ones (as these are less sensitive to code perturbations). I'm
not sure why the code was using indexes in the first place, but I've
(manually) tested the change with various libc++ versions, and found no
issues with this approach.

[philnik]

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

660

This should only be done if the string is in the normal layout and little endian or in the alternate layout and big endian. Why do you care about the capacity at all? Isn't that just another point of failure?

[shafik]

Thank you, the names make a lot more sense to me but I am not sure why this was originally used either.

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

650

nitpick /*can_create=*/true and same for the next two.

[labath]

• address review comments

[labath]

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

660

I've added the layout check and made a note that big-endian is not supported. The capacity check comes from https://reviews.llvm.org/D73860, but I'm not sure of the overall purpose. I guess it was trying to screen out corrupt/uninitialized string objects, but I'm not convinced that we're doing anyone a favour by refusing to format those -- after all, the application itself might access such objects without checking the capacity, and operate on the data that we've decided to ignore.

[philnik]

LGTM assuming you checked that it actually works with my patch applied. Do you want to land your patch first, or should we do it the other way around?

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

660

Maybe it would make sense to say that the string is uninitialized or corrupted when the __invariants() fail? That would probably catch more bugs, but I don't know if you can just run a member function in the pretty printer.

[labath]

In D124113#3464287, @philnik wrote:

LGTM assuming you checked that it actually works with my patch applied. Do you want to land your patch first, or should we do it the other way around?

I've confirmed this with the real patch and with my magic string simulator. For me, it would be better if this patch went in first (so I guess I'll land it now).

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

660

Technically, we can run functions from pretty-printers, but we try hard to avoid that. If calling functions was ok, we could just call data() and get rid of this mess. :)

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

 enum LibcxxStringLayoutMode {
   eLibcxxStringLayoutModeCSD = 0,
   eLibcxxStringLayoutModeDSC = 1,
   eLibcxxStringLayoutModeInvalid = 0xffff
 };
 
 /// Determine the size in bytes of \p valobj (a libc++ std::string object) and
 /// extract its data payload. Return the size + payload pair.
+// TODO: Support big-endian architectures.
 static llvm::Optional<std::pair<uint64_t, ValueObjectSP>>
 ExtractLibcxxStringInfo(ValueObject &valobj) {
   ValueObjectSP D(valobj.GetChildAtIndexPath({0, 0, 0, 0}));
   if (!D)
     return {};
 
   ValueObjectSP layout_decider(
       D->GetChildAtIndexPath(llvm::ArrayRef<size_t>({0, 0})));
 
   // this child should exist
   if (!layout_decider)
     return {};
 
   ConstString g_data_name("__data_");
   ConstString g_size_name("__size_");
   bool short_mode = false; // this means the string is in short-mode and the
                            // data is stored inline
+  bool using_bitmasks = true; // Whether the class uses bitmasks for the mode
+                              // flag (pre-D123580).
+  uint64_t size;
   LibcxxStringLayoutMode layout = (layout_decider->GetName() == g_data_name)
                                       ? eLibcxxStringLayoutModeDSC
                                       : eLibcxxStringLayoutModeCSD;
   uint64_t size_mode_value = 0;
 
-  if (layout == eLibcxxStringLayoutModeDSC) {
+  if (ValueObjectSP is_long = D->GetChildAtNamePath(
+          {ConstString("__s"), ConstString("__is_long_")})) {
+    using_bitmasks = false;
+    short_mode = !is_long->GetValueAsUnsigned(/*fail_value=*/0);
+    if (ValueObjectSP size_member =
+            D->GetChildAtNamePath({ConstString("__s"), ConstString("__size_")}))
+      size = size_member->GetValueAsUnsigned(/*fail_value=*/0);
+    else
+      return {};
+  } else if (layout == eLibcxxStringLayoutModeDSC) {
     llvm::SmallVector<size_t, 3> size_mode_locations[] = {
-        {1, 2}, // Post-c3d0205ee771 layout
+        {1, 2}, // Post-c3d0205ee771 layout. This was in use for only a brief
+                // period, so we can delete it if it becomes a burden.
         {1, 1, 0},
         {1, 1, 1},
     };
     ValueObjectSP size_mode;
     for (llvm::ArrayRef<size_t> loc : size_mode_locations) {
       size_mode = D->GetChildAtIndexPath(loc);
       if (size_mode && size_mode->GetName() == g_size_name)
         break;
ExtractLibcxxStringInfo(ValueObject &valobj) {
   }
 
   if (short_mode) {
     ValueObjectSP s(D->GetChildAtIndex(1, true));
     if (!s)
       return {};
     ValueObjectSP location_sp = s->GetChildAtIndex(
         (layout == eLibcxxStringLayoutModeDSC) ? 0 : 1, true);
-    const uint64_t size = (layout == eLibcxxStringLayoutModeDSC)
-                              ? size_mode_value
-                              : ((size_mode_value >> 1) % 256);
+    if (using_bitmasks)
+      size = (layout == eLibcxxStringLayoutModeDSC)
 
     // When the small-string optimization takes place, the data must fit in the
     // inline string buffer (23 bytes on x86_64/Darwin). If it doesn't, it's
     // likely that the string isn't initialized and we're reading garbage.
     ExecutionContext exe_ctx(location_sp->GetExecutionContextRef());
     const llvm::Optional<uint64_t> max_bytes =
         location_sp->GetCompilerType().GetByteSize(
             exe_ctx.GetBestExecutionContextScope());
     if (!max_bytes || size > *max_bytes || !location_sp)
       return {};
 
     return std::make_pair(size, location_sp);
   }
 
   ValueObjectSP l(D->GetChildAtIndex(0, true));
   if (!l)
     return {};
   // we can use the layout_decider object as the data pointer
-  ValueObjectSP location_sp = (layout == eLibcxxStringLayoutModeDSC)
-                                  ? layout_decider
-                                  : l->GetChildAtIndex(2, true);
-  ValueObjectSP size_vo(l->GetChildAtIndex(1, true));
-  const unsigned capacity_index =
-      (layout == eLibcxxStringLayoutModeDSC) ? 2 : 0;
-  ValueObjectSP capacity_vo(l->GetChildAtIndex(capacity_index, true));
+  ValueObjectSP location_sp =
+      l->GetChildMemberWithName(ConstString("__data_"), /*can_create=*/true);
+  ValueObjectSP size_vo =
+      l->GetChildMemberWithName(ConstString("__size_"), /*can_create=*/true);
+  ValueObjectSP capacity_vo =
+      l->GetChildMemberWithName(ConstString("__cap_"), /*can_create=*/true);
   if (!size_vo || !location_sp || !capacity_vo)
     return {};
-  const uint64_t size = size_vo->GetValueAsUnsigned(LLDB_INVALID_OFFSET);
-  const uint64_t capacity =
-      capacity_vo->GetValueAsUnsigned(LLDB_INVALID_OFFSET);
+  size = size_vo->GetValueAsUnsigned(LLDB_INVALID_OFFSET);
+  uint64_t capacity = capacity_vo->GetValueAsUnsigned(LLDB_INVALID_OFFSET);
+  if (!using_bitmasks && layout == eLibcxxStringLayoutModeCSD)
+    capacity *= 2;
   if (size == LLDB_INVALID_OFFSET || capacity == LLDB_INVALID_OFFSET ||
       capacity < size)
     return {};
   return std::make_pair(size, location_sp);
 }
 
 static bool
 LibcxxWStringSummaryProvider(ValueObject &valobj, Stream &stream,