[Lldb-commits] [lldb] [lldb] Remove StringPrinter workaround for NSString (PR #196311)

[via lldb-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Nerixyz (Nerixyz)

<details>
<summary>Changes</summary>

`StringPrinter` had a special case for ASCII strings that indicated they'd know their size (`SetHasSourceSize(true)`), supplied zero as the size, but actually wanted the printer to search for a null terminator.
The only user doing this was the `NSString` summary provider. The C++ formatters only use `SetHasSourceSize(true)` if they're formatting an array (https://github.com/llvm/llvm-project/pull/195514).

I don't know much about `NSString` as I couldn't find good documentation on its layout and couldn't test it. The following is based on the code here.

`NSString` used the `has_explicit_length` flag, however if that was set the formatter didn't always read that length. For example, if the info bits indicated that the string is null terminated (`has_null`), then the length wasn't read even if `has_explicit_length` was set. However, the formatter still used `SetHasSourceSize(true)` in that case. So without the previous workaround, we'd read zero bytes.

This PR adds a `has_source_size` to indicate that we have actually read the size.



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


2 Files Affected:

- (modified) lldb/source/DataFormatters/StringPrinter.cpp (+1-11) 
- (modified) lldb/source/Plugins/Language/ObjC/NSString.cpp (+16-10) 


``````````diff
diff --git a/lldb/source/DataFormatters/StringPrinter.cpp b/lldb/source/DataFormatters/StringPrinter.cpp
index 3a6a55e9d4e02..5c556bede3ae4 100644
--- a/lldb/source/DataFormatters/StringPrinter.cpp
+++ b/lldb/source/DataFormatters/StringPrinter.cpp
@@ -440,17 +440,7 @@ static bool ReadEncodedBufferAndDumpToStream(
   const auto max_size = target_sp->GetMaximumSizeOfStringSummary();
 
   uint32_t sourceSize;
-  if (elem_type == StringElementType::ASCII && !options.GetSourceSize()) {
-    // FIXME: The NSString formatter sets HasSourceSize(true) when the size is
-    // actually unknown, as well as SetZeroTermination(Ignore). IIUC the
-    // C++ formatter also sets SetZeroTermination(Ignore) when it doesn't
-    // mean to. I don't see how this makes sense: we should fix the formatters.
-    //
-    // Until then, the behavior that's expected for ASCII strings with unknown
-    // lengths is to read up to the max size and then null-terminate. Do that.
-    sourceSize = max_size;
-    needs_zero_terminator = true;
-  } else if (options.HasSourceSize()) {
+  if (options.HasSourceSize()) {
     sourceSize = options.GetSourceSize();
     if (!options.GetIgnoreMaxLength()) {
       if (sourceSize > max_size) {
diff --git a/lldb/source/Plugins/Language/ObjC/NSString.cpp b/lldb/source/Plugins/Language/ObjC/NSString.cpp
index 7a295119bd031..7d1dbf2f7afcd 100644
--- a/lldb/source/Plugins/Language/ObjC/NSString.cpp
+++ b/lldb/source/Plugins/Language/ObjC/NSString.cpp
@@ -100,6 +100,10 @@ bool lldb_private::formatters::NSStringSummaryProvider(
   bool has_null = (info_bits & 8) == 8;
 
   size_t explicit_length = 0;
+  // Even though the explicit length flag is set, we might only want to print
+  // the string until the first null terminator. `has_source_size` indicates if
+  // the string printer should use the size we gave it.
+  bool has_source_size = false;
   if (!has_null && has_explicit_length && !is_path_store) {
     lldb::addr_t explicit_length_offset = 2 * ptr_size;
     if (is_mutable && !is_inline)
@@ -117,6 +121,7 @@ bool lldb_private::formatters::NSStringSummaryProvider(
       explicit_length_offset = valobj_addr + explicit_length_offset;
       explicit_length = process_sp->ReadUnsignedIntegerFromMemory(
           explicit_length_offset, 4, 0, error);
+      has_source_size = true;
     }
   }
 
@@ -150,7 +155,7 @@ bool lldb_private::formatters::NSStringSummaryProvider(
       options.SetStream(&stream);
       options.SetQuote('"');
       options.SetSourceSize(explicit_length);
-      options.SetHasSourceSize(has_explicit_length);
+      options.SetHasSourceSize(has_source_size);
       options.SetZeroTermination(StringPrinter::ZeroTermination::Ignore);
       options.SetIgnoreMaxLength(summary_options.GetCapping() ==
                                  TypeSummaryCapping::eTypeSummaryUncapped);
@@ -161,7 +166,7 @@ bool lldb_private::formatters::NSStringSummaryProvider(
       options.SetTargetSP(valobj.GetTargetSP());
       options.SetStream(&stream);
       options.SetSourceSize(explicit_length);
-      options.SetHasSourceSize(has_explicit_length);
+      options.SetHasSourceSize(has_source_size);
       options.SetZeroTermination(StringPrinter::ZeroTermination::Ignore);
       options.SetIgnoreMaxLength(summary_options.GetCapping() ==
                                  TypeSummaryCapping::eTypeSummaryUncapped);
@@ -177,7 +182,7 @@ bool lldb_private::formatters::NSStringSummaryProvider(
     options.SetStream(&stream);
     options.SetQuote('"');
     options.SetSourceSize(explicit_length);
-    options.SetHasSourceSize(has_explicit_length);
+    options.SetHasSourceSize(has_source_size);
     options.SetIgnoreMaxLength(summary_options.GetCapping() ==
                                TypeSummaryCapping::eTypeSummaryUncapped);
     return StringPrinter::ReadStringAndDumpToStream<
@@ -199,8 +204,8 @@ bool lldb_private::formatters::NSStringSummaryProvider(
     options.SetStream(&stream);
     options.SetQuote('"');
     options.SetSourceSize(explicit_length);
-    options.SetHasSourceSize(has_explicit_length);
-    if (has_explicit_length)
+    options.SetHasSourceSize(has_source_size);
+    if (has_source_size)
       options.SetZeroTermination(StringPrinter::ZeroTermination::Ignore);
     else
       options.SetZeroTermination(StringPrinter::ZeroTermination::ZeroTerminate);
@@ -227,8 +232,8 @@ bool lldb_private::formatters::NSStringSummaryProvider(
     options.SetStream(&stream);
     options.SetQuote('"');
     options.SetSourceSize(explicit_length);
-    options.SetHasSourceSize(has_explicit_length);
-    if (has_explicit_length)
+    options.SetHasSourceSize(has_source_size);
+    if (has_source_size)
       options.SetZeroTermination(StringPrinter::ZeroTermination::Ignore);
     else
       options.SetZeroTermination(StringPrinter::ZeroTermination::ZeroTerminate);
@@ -245,20 +250,21 @@ bool lldb_private::formatters::NSStringSummaryProvider(
       explicit_length =
           process_sp->ReadUnsignedIntegerFromMemory(location, 1, 0, error);
       has_explicit_length = !(error.Fail() || explicit_length == 0);
+      has_source_size = has_explicit_length;
       location++;
     }
     options.SetLocation(Address(location));
     options.SetTargetSP(valobj.GetTargetSP());
     options.SetStream(&stream);
     options.SetSourceSize(explicit_length);
-    options.SetHasSourceSize(has_explicit_length);
-    if (has_explicit_length)
+    options.SetHasSourceSize(has_source_size);
+    if (has_source_size)
       options.SetZeroTermination(StringPrinter::ZeroTermination::Ignore);
     else
       options.SetZeroTermination(StringPrinter::ZeroTermination::ZeroTerminate);
     options.SetIgnoreMaxLength(summary_options.GetCapping() ==
                                TypeSummaryCapping::eTypeSummaryUncapped);
-    if (has_explicit_length)
+    if (has_source_size)
       return StringPrinter::ReadStringAndDumpToStream<
           StringPrinter::StringElementType::UTF8>(options);
     else

``````````

</details>


https://github.com/llvm/llvm-project/pull/196311