[lldb/LibCxx] Have ExtractLibcxxStringInfo return an Optional result, NFC

[vsk]

Summary

... and a few other minor simplifications.

[JDevlieghere]

Thanks for doing this!

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

507

Would it make sense to split the computation of size_mode_value and short_mode into a helper?

[vsk]

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

507

My 2c is that having this inline seems a little more readable, but I'm not strongly opposed.

673–674

This causes some kind of use-after-free, I've backed this change out.

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

 
 // the field layout in a libc++ string (cap, side, data or data, size, cap)
 enum LibcxxStringLayoutMode {
   eLibcxxStringLayoutModeCSD = 0,
   eLibcxxStringLayoutModeDSC = 1,
   eLibcxxStringLayoutModeInvalid = 0xffff
 };
 
-// this function abstracts away the layout and mode details of a libc++ string
-// and returns the address of the data and the size ready for callers to
-// consume
-static bool ExtractLibcxxStringInfo(ValueObject &valobj,
-                                    ValueObjectSP &location_sp,
-                                    uint64_t &size) {
+/// Determine the size in bytes of \p valobj (a libc++ std::string object) and
+/// extract its data payload. Return the size + payload pair.
+static llvm::Optional<std::pair<uint64_t, ValueObjectSP>>
+ExtractLibcxxStringInfo(ValueObject &valobj) {
   ValueObjectSP D(valobj.GetChildAtIndexPath({0, 0, 0, 0}));
   if (!D)
-    return false;
+    return {};
 
   ValueObjectSP layout_decider(
     D->GetChildAtIndexPath(llvm::ArrayRef<size_t>({0, 0})));
 
   // this child should exist
   if (!layout_decider)
-    return false;
+    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
   LibcxxStringLayoutMode layout = (layout_decider->GetName() == g_data_name)
                                       ? eLibcxxStringLayoutModeDSC
                                       : eLibcxxStringLayoutModeCSD;
   uint64_t size_mode_value = 0;
 
   if (layout == eLibcxxStringLayoutModeDSC) {
     ValueObjectSP size_mode(D->GetChildAtIndexPath({1, 1, 0}));
     if (!size_mode)
-      return false;
+      return {};
 
     if (size_mode->GetName() != g_size_name) {
       // we are hitting the padding structure, move along
       size_mode = D->GetChildAtIndexPath({1, 1, 1});
       if (!size_mode)
-        return false;
+        return {};
     }
 
     size_mode_value = (size_mode->GetValueAsUnsigned(0));
     short_mode = ((size_mode_value & 0x80) == 0);
   } else {
     ValueObjectSP size_mode(D->GetChildAtIndexPath({1, 0, 0}));
     if (!size_mode)
-      return false;
+      return {};
 
     size_mode_value = (size_mode->GetValueAsUnsigned(0));
     short_mode = ((size_mode_value & 1) == 0);
   }
 
   if (short_mode) {
     ValueObjectSP s(D->GetChildAtIndex(1, true));
     if (!s)
-      return false;
-    location_sp = s->GetChildAtIndex(
+      return {};
+    ValueObjectSP location_sp = s->GetChildAtIndex(
         (layout == eLibcxxStringLayoutModeDSC) ? 0 : 1, true);
-    size = (layout == eLibcxxStringLayoutModeDSC)
+    const uint64_t size = (layout == eLibcxxStringLayoutModeDSC)
                               ? size_mode_value
                               : ((size_mode_value >> 1) % 256);
 
     // 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)
-      return false;
+    if (!max_bytes || size > *max_bytes || !location_sp)
+      return {};
+
+    return std::make_pair(size, location_sp);
+  }
 
-    return (location_sp.get() != nullptr);
-  } else {
-    ValueObjectSP l(D->GetChildAtIndex(0, true));
-    if (!l)
-      return false;
-    // we can use the layout_decider object as the data pointer
-    location_sp = (layout == eLibcxxStringLayoutModeDSC)
+    return {};
+  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));
-    if (!size_vo || !location_sp || !capacity_vo)
-      return false;
-    size = size_vo->GetValueAsUnsigned(LLDB_INVALID_OFFSET);
-    const uint64_t cap = capacity_vo->GetValueAsUnsigned(LLDB_INVALID_OFFSET);
-    if (size == LLDB_INVALID_OFFSET || cap == LLDB_INVALID_OFFSET || cap < size)
-      return false;
-    return true;
-  }
+    return {};
+  const uint64_t size = size_vo->GetValueAsUnsigned(LLDB_INVALID_OFFSET);
+  const uint64_t capacity =
+      capacity_vo->GetValueAsUnsigned(LLDB_INVALID_OFFSET);
+  if (size == LLDB_INVALID_OFFSET || capacity == LLDB_INVALID_OFFSET ||
+      capacity < size)
+    return {};
+  return std::make_pair(size, location_sp);
 }
 
 bool lldb_private::formatters::LibcxxWStringSummaryProvider(
     ValueObject &valobj, Stream &stream,
     const TypeSummaryOptions &summary_options) {
-  uint64_t size = 0;
-  ValueObjectSP location_sp;
-  if (!ExtractLibcxxStringInfo(valobj, location_sp, size))
+  auto string_info = ExtractLibcxxStringInfo(valobj);
+  if (!string_info)
     return false;
+  uint64_t size;
+  ValueObjectSP location_sp;
+  std::tie(size, location_sp) = *string_info;
+
   if (size == 0) {
     stream.Printf("L\"\"");
     return true;
   }
   if (!location_sp)
     return false;
 
-  DataExtractor extractor;
 
   StringPrinter::ReadBufferAndDumpToStreamOptions options(valobj);
-
   if (summary_options.GetCapping() == TypeSummaryCapping::eTypeSummaryCapped) {
     const auto max_size = valobj.GetTargetSP()->GetMaximumSizeOfStringSummary();
     if (size > max_size) {
       size = max_size;
       options.SetIsTruncated(true);
     }
   }
+
+  DataExtractor extractor;
   const size_t bytes_read = location_sp->GetPointeeData(extractor, 0, size);
   if (bytes_read < size)
     return false;
 
   // std::wstring::size() is measured in 'characters', not bytes
   TypeSystemClang *ast_context =
       TypeSystemClang::GetScratch(*valobj.GetTargetSP());
   if (!ast_context)
bool lldb_private::formatters::LibcxxWStringSummaryProvider(
   }
   return false;
 }
 
 template <StringPrinter::StringElementType element_type>
 bool LibcxxStringSummaryProvider(ValueObject &valobj, Stream &stream,
                                  const TypeSummaryOptions &summary_options,
                                  std::string prefix_token) {
-  uint64_t size = 0;
-  ValueObjectSP location_sp;
-  if (!ExtractLibcxxStringInfo(valobj, location_sp, size))
+  auto string_info = ExtractLibcxxStringInfo(valobj);
+  if (!string_info)
     return false;
+  uint64_t size;
+  ValueObjectSP location_sp;
+  std::tie(size, location_sp) = *string_info;
+
   if (size == 0) {
     stream.Printf("\"\"");
     return true;
   }
 
   if (!location_sp)
     return false;
 
   StringPrinter::ReadBufferAndDumpToStreamOptions options(valobj);
 
-  DataExtractor extractor;
   if (summary_options.GetCapping() == TypeSummaryCapping::eTypeSummaryCapped) {
     const auto max_size = valobj.GetTargetSP()->GetMaximumSizeOfStringSummary();
     if (size > max_size) {
       size = max_size;
       options.SetIsTruncated(true);
     }
   }
+
+  DataExtractor extractor;
   const size_t bytes_read = location_sp->GetPointeeData(extractor, 0, size);
   if (bytes_read < size)
     return false;
 
   options.SetData(extractor);
   options.SetStream(&stream);
-
   if (prefix_token.empty())
     options.SetPrefixToken(nullptr);
   else
     options.SetPrefixToken(prefix_token);
-
   options.SetQuote('"');
   options.SetSourceSize(size);
   options.SetBinaryZeroIsTerminator(false);
   return StringPrinter::ReadBufferAndDumpToStream<element_type>(options);
 }
 
 template <StringPrinter::StringElementType element_type>
 static bool formatStringImpl(ValueObject &valobj, Stream &stream,
                              const TypeSummaryOptions &summary_options,