Page MenuHomePhabricator

[lldb] Fix NSURL data formatter truncation issue in Swift
ClosedPublic

Authored by poya on Nov 18 2019, 6:05 AM.

Details

Reviewers
davide
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

Diff Detail

Event Timeline

poya created this revision.Nov 18 2019, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2019, 6:05 AM

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].

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 updated this revision to Diff 230045.Nov 19 2019, 5:07 AM
poya added a comment.Nov 19 2019, 5:16 AM

@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

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 requested changes to this revision.Nov 19 2019, 5:36 AM
teemperor added inline comments.
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.

This revision now requires changes to proceed.Nov 19 2019, 5:36 AM
poya marked an inline comment as done.Nov 19 2019, 6:05 AM
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 added a comment.Nov 19 2019, 6:15 AM

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.

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 marked 2 inline comments as done.Nov 19 2019, 7:17 AM
poya added inline comments.
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 marked 3 inline comments as not done.Nov 19 2019, 7:23 AM

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

poya updated this revision to Diff 230248.Nov 20 2019, 6:10 AM

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

poya updated this revision to Diff 230251.Nov 20 2019, 6:46 AM
davide accepted this revision.Nov 20 2019, 12:26 PM

LGTM

commit 6f4398d1b9950d48ead91b2b550792f2bbe4778e (HEAD -> master, origin/master, origin/HEAD)
Author: Davide Italiano <ditaliano@apple.com>
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 removed a reviewer: teemperor.Nov 24 2019, 5:50 PM

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

This revision is now accepted and ready to land.Nov 24 2019, 5:50 PM
poya closed this revision.Nov 24 2019, 5:50 PM