[Lldb-commits] [PATCH] D73860: [lldb/StringPrinter] Avoid reading garbage in uninitialized strings

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:474
 // consume
 static bool ExtractLibcxxStringInfo(ValueObject &valobj,
                                     ValueObjectSP &location_sp,
----------------
Definitely something for another patch, but this is really bad, looking at the signature I have no idea what's in or output. This should return an optional<struct/pair/tuple>.


================
Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:611
   case 1:
-    StringPrinter::ReadBufferAndDumpToStream<
+    return StringPrinter::ReadBufferAndDumpToStream<
         lldb_private::formatters::StringPrinter::StringElementType::UTF8>(
----------------
This is so much better.


================
Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:656
   }
-  location_sp->GetPointeeData(extractor, 0, size);
+  const size_t bytes_read = location_sp->GetPointeeData(extractor, 0, size);
+  if (bytes_read < size)
----------------
Can we move the `extractor` closer to where it's used, so basically after the capping check?


================
Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:663
 
   if (prefix_token.empty())
     options.SetPrefixToken(nullptr);
----------------
Not code you touched, but...

```
options.SetPrefixToken(prefix_token.empty() ? nullptr : prefix_token); 
```
might prevent this visual break in setting the options.


================
Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:679
+  StreamString scratch_stream;
+  bool success = LibcxxStringSummaryProvider<element_type>(
+      valobj, scratch_stream, summary_options, prefix_token);
----------------
`const bool`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73860/new/

https://reviews.llvm.org/D73860