[lldb] Fix NSURL data formatter truncation issue in Swift

[poya]

Summary

Remove hardcoded string prefix length assumption causing issues when
concatenating summary for NSURL in NSURLSummaryProvider. Provider relies
on concatenation of NSStringProvider results for summary, and while the
strings are prefixed with '@' in Objective-C, that is not the case in
Swift causing part of the description to be truncated.

https://bugs.swift.org/browse/SR-5994

[davide]

While I review this, can you create a corresponding change to swift.org which tests this? [I don't think we have a way of triggering this from pure Obj-C, but I may be wrong].

[davide]

Thanks again for your contribution!

This change broke one test. Can you please investigate?

RESULT: FAILED (0 passes, 3 failures, 0 errors, 1 skipped, 0 expected failures, 0 unexpected successes)

********************

Testing Time: 316.58s
********************
Failing Tests (1):
    lldb-api :: functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSURL.py

  Expected Passes    : 1624
  Expected Failures  : 8
  Unsupported Tests  : 184
  Unexpected Failures: 1

[poya]

@davide Thanks for the review, and while I haven't been able to reproduce the test failure locally with my configuration, I reviewed the code and corrected a potential memory issue that might have been the cause for the failure. If not, do you mind sharing more info on the failure?

I have also added a test to lldb swift fork, which I can create a PR for once this change lands there, or what's the normal workflow?
https://github.com/poya/llvm-project/commit/e884bf7b0cc2700b50cf36b1d3d6401046eec1bc

[teemperor]

You could make a PR with both this commit and the test against the swift fork. If that is accepted and merged, then the fix can be upstreamed. But having a test for this upstream is what should ideally happen instead of just testing this downstream.

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

662

I am not a big fan of the "Class_method" way of extending classes. Maybe ConcatSummaryForNSURL?

[teemperor]

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

662

first and second also not very descriptive variables names. What about the summary and base_summary names from where we call the method?

662

Having first as both a in and out parameter makes the control flow not very trivial to understand. You could just compute a result and return that result (and then assign this to the summary variable in the calling context)? Also all parameters to this function can just be llvm::StringRef from what I can see.

673

This can all be just:

first_str.consume_back(suffix);
first_str.consume_back("\"");

And the same thing below.

684

This could just be return first_str + " -- " + second_str; if you return instead of having an in/out parameter.

[poya]

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

662

Was trying to follow the coding style used elsewhere in the same file for the helper function name

662

Didn't think summary and base_summary made it very simple to reason about, perhaps destination and source, or front and back, would make more sense for a concatenation function?

673

Didn't want to make an assumption of the quote character used if it ever changed

[poya]

In D70393#1751514, @teemperor wrote:

But having a test for this upstream is what should ideally happen instead of just testing this downstream.

There is already a test for the NSURL data formatter upstream for the ObjC context, but the same summary provider is also used with Swift and it was only there it was being incorrectly truncated, because assumption of prefix length. So if I understood @davide correctly he wanted a test added in the Swift fork for the Swift case.

[teemperor]

In D70393#1751614, @poya wrote:

In D70393#1751514, @teemperor wrote:

But having a test for this upstream is what should ideally happen instead of just testing this downstream.

There is already a test for the NSURL data formatter upstream for the ObjC context, but the same summary provider is also used with Swift and it was only there it was being incorrectly truncated, because assumption of prefix length. So if I understood @davide correctly he wanted a test added in the Swift fork for the Swift case.

Usually code should not just be tested downstream but also here (which is of course tricky with Swift-specific stuff as Swift is only downstream). So if you see a way to test this without just testing it in swift-lldb that would be great, but on the other hand this is just touching ObjC formatters which are anyway Apple-specific code, so it's not the end of the world if not...

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

662

Was trying to follow the coding style used elsewhere in the same file for the helper function name

Fair point.

Didn't think summary and base_summary made it very simple to reason about, perhaps destination and source, or front and back, would make more sense for a concatenation function?

I mean it's not just concatenating first and second, but removes string prefix/suffix, one arbitrary character that is the a quotation character we don't want in summaries and then returns them with in the format for that we expect for summaries (A -- B), so it seems quite specific to expect a summary and summary_base.

673

If this changes we probably want to know (and we maybe should even assert that this happens).

[poya]

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

662

Agree that it's not a pure concatenation function, but just to avoid any misunderstandings I want to clarify that it retains the enclosing prefix/suffix and quote marks in its output.

It concatenates
@"test.html" and @"http://lldb.llvm.org"
to
@"test.html -- http://lldb.llvm.org"
which is the actual summary output

I can try switching the arg names though.

673

Got it. As I saw that in some contexts the quote character was ' instead of " (not here though), I was just wary about potentially introducing a similar failure point as I was fixing, '@' string prefix in obj-c vs no prefix in Swift, which had gone unnoticed.

[poya]

Sorry for marking comments as done, not used to the tool.

[poya]

Remove helper function
Hardcode quote character to " with assert to catch future changes

[davide]

LGTM

[davide]

commit 6f4398d1b9950d48ead91b2b550792f2bbe4778e (HEAD -> master, origin/master, origin/HEAD)
Author: Davide Italiano <[e-mail]>
Date:   Wed Nov 20 12:27:26 2019 -0800

    [lldb] Fix NSURL data formatter truncation issue
    
    Remove hardcoded string prefix length assumption causing issues when
    concatenating summary for NSURL in NSURLSummaryProvider. Provider relies
    on concatenation of NSStringProvider results for summary, and while the
    strings are prefixed with '@' in Objective-C, that is not the case in
    Swift causing part of the description to be truncated.
    
    This will be tested in the downstream fork.
    
    Patch by Martin Svensson!

[poya]

Removing Raphael as reviewer hoping that it will allow me to close this already merged revision.

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

bool lldb_private::formatters::NSDecimalNumberSummaryProvider(
 
   if (is_negative)
     stream.Printf("-");
 
   stream.Printf("%" PRIu64 " x 10^%" PRIi8, mantissa, exponent);
   return true;
 }
 
 bool lldb_private::formatters::NSURLSummaryProvider(
     ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
   ProcessSP process_sp = valobj.GetProcessSP();
   if (!process_sp)
     return false;
 
   ObjCLanguageRuntime *runtime = ObjCLanguageRuntime::Get(*process_sp);
 
   if (!runtime)
     return false;
 
   ObjCLanguageRuntime::ClassDescriptorSP descriptor(
       runtime->GetClassDescriptor(valobj));
 
   if (!descriptor || !descriptor->IsValid())
     return false;
 
   uint32_t ptr_size = process_sp->GetAddressByteSize();
 
   lldb::addr_t valobj_addr = valobj.GetValueAsUnsigned(0);
 
   if (!valobj_addr)
     return false;
 
   llvm::StringRef class_name = descriptor->GetClassName().GetStringRef();
 
   if (!class_name.equals("NSURL"))
     return false;
 
   uint64_t offset_text = ptr_size + ptr_size +
                          8; // ISA + pointer + 8 bytes of data (even on 32bit)
   uint64_t offset_base = offset_text + ptr_size;
   CompilerType type(valobj.GetCompilerType());
   ValueObjectSP text(valobj.GetSyntheticChildAtOffset(offset_text, type, true));
   ValueObjectSP base(valobj.GetSyntheticChildAtOffset(offset_base, type, true));
-  if (!text)
-    return false;
-  if (text->GetValueAsUnsigned(0) == 0)
+  if (!text || text->GetValueAsUnsigned(0) == 0)
     return false;
+
+  StreamString base_summary;
+  if (base && base->GetValueAsUnsigned(0)) {
+    if (!NSURLSummaryProvider(*base, base_summary, options))
+      base_summary.Clear();
+  }
+  if (base_summary.Empty())
+    return NSStringSummaryProvider(*text, stream, options);
+
   StreamString summary;
-  if (!NSStringSummaryProvider(*text, summary, options))
+  if (!NSStringSummaryProvider(*text, summary, options) || summary.Empty())
     return false;
-  if (base && base->GetValueAsUnsigned(0)) {
-    std::string summary_str = summary.GetString();
 
-    if (!summary_str.empty())
-      summary_str.pop_back();
-    summary_str += " -- ";
-    StreamString base_summary;
-    if (NSURLSummaryProvider(*base, base_summary, options) &&
-        !base_summary.Empty()) {
-      llvm::StringRef base_str = base_summary.GetString();
-      if (base_str.size() > 2)
-        base_str = base_str.drop_front(2);
-      summary_str += base_str;
+  const char quote_char = '"';
+  std::string prefix, suffix;
+  if (Language *language = Language::FindPlugin(options.GetLanguage())) {
+    if (!language->GetFormatterPrefixSuffix(*text, ConstString("NSString"),
+                                            prefix, suffix)) {
+      prefix.clear();
+      suffix.clear();
     }
-    summary.Clear();
-    summary.PutCString(summary_str);
   }
-  if (!summary.Empty()) {
-    stream.PutCString(summary.GetString());
+  // @"A" -> @"A
+  llvm::StringRef summary_str = summary.GetString();
+  bool back_consumed = summary_str.consume_back(quote_char + suffix);
+  assert(back_consumed); UNUSED_IF_ASSERT_DISABLED(back_consumed);
+  // @"B" -> B"
+  llvm::StringRef base_summary_str = base_summary.GetString();
+  bool front_consumed = base_summary_str.consume_front(prefix + quote_char);
+  assert(front_consumed); UNUSED_IF_ASSERT_DISABLED(front_consumed);
+  // @"A -- B"
+  if (!summary_str.empty() && !base_summary_str.empty()) {
+    stream.Printf("%s -- %s", summary_str.str().c_str(),
+                  base_summary_str.str().c_str());
     return true;
   }
 
   return false;
 }
 
 /// Bias value for tagged pointer exponents.
 /// Recommended values: