Skip to content

Add error reporting when looking up types by demangled name. #33585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 28, 2020

Conversation

mikeash
Copy link
Contributor

@mikeash mikeash commented Aug 21, 2020

When mangled name lookups fail, it can be pretty hard to track down the cause. There are many places where it can fail, and when nested generics are involved you can end up recursing a lot. My technique so far has involved adding lots and lots of ad-hoc logging calls to track down the failure.

This PR plumbs error reporting through the whole structure. Errors are recorded as a function pointer and a context pointer, allowing errors to gather relevant information, but also avoiding the overhead of string building on paths where the caller is going to have some sort of fallback instead of reporting the error.

For an example of what this does, here's an error I debugged the other day:

failed to demangle witness for associated type 'Body' in conformance 'Syndrome.ProcessPaymentButton: View' from mangled name '+�'

And here's what that error looks like with this PR:

failed to demangle witness for associated type 'Body' in conformance 'Syndrome.ProcessPaymentButton: View' from mangled name '+�' - _gatherGenericParameters: context: $s7SwiftUI12ProgressViewVMn 0x1a214efd0 <SwiftUI.EmptyView> parent: <null> - incorrect number of generic args (1), 2 local params, 2 total params

This would have saved a huge amount of debugging time (and hopefully will save a huge amount in the future!).

@mikeash mikeash requested a review from tbkka August 21, 2020 16:52
@mikeash mikeash force-pushed the type-lookup-error-reporting branch 3 times, most recently from c598c14 to de3efaf Compare August 24, 2020 20:50
@mikeash
Copy link
Contributor Author

mikeash commented Aug 24, 2020

@swift-ci please benchmark

@swift-ci
Copy link
Contributor

Build failed before running benchmark.

@mikeash mikeash force-pushed the type-lookup-error-reporting branch from de3efaf to 0854fb6 Compare August 25, 2020 13:58
@mikeash
Copy link
Contributor Author

mikeash commented Aug 25, 2020

@swift-ci please benchmark

@swift-ci
Copy link
Contributor

Build failed before running benchmark.

@mikeash
Copy link
Contributor Author

mikeash commented Aug 25, 2020

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 0854fb609e77b7dab999a019d75f6df2e1f50b96

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 0854fb609e77b7dab999a019d75f6df2e1f50b96

@mikeash
Copy link
Contributor Author

mikeash commented Aug 25, 2020

swiftlang/llvm-project#1702

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 0854fb609e77b7dab999a019d75f6df2e1f50b96

@mikeash mikeash force-pushed the type-lookup-error-reporting branch from 0854fb6 to 1dec952 Compare August 26, 2020 15:27
@mikeash
Copy link
Contributor Author

mikeash commented Aug 26, 2020

swiftlang/llvm-project#1702

@swift-ci please clean test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 0854fb609e77b7dab999a019d75f6df2e1f50b96

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 0854fb609e77b7dab999a019d75f6df2e1f50b96

@mikeash mikeash force-pushed the type-lookup-error-reporting branch from 1dec952 to aa800c6 Compare August 27, 2020 16:09
@mikeash
Copy link
Contributor Author

mikeash commented Aug 27, 2020

swiftlang/llvm-project#1702

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 1dec95205a759db9b81ca42d6e2a52893f190633

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 1dec95205a759db9b81ca42d6e2a52893f190633

@@ -1183,14 +1184,14 @@ class MetadataReader {
MangledNameKind::Type, Dem);
}

BuiltType
TypeLookupErrorOr<typename BuilderType::BuiltType>
Copy link
Contributor

@tbkka tbkka Aug 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"ErrorOr" happens to be my personal favorite name for this pattern. 😄

@mikeash
Copy link
Contributor Author

mikeash commented Aug 28, 2020

swiftlang/llvm-project#1702

@swift-ci please benchmark

@swift-ci
Copy link
Contributor

Build failed before running benchmark.

@shahmishal
Copy link
Member

swiftlang/llvm-project#1702

@swift-ci please benchmark

@swift-ci
Copy link
Contributor

Build failed before running benchmark.

@mikeash mikeash force-pushed the type-lookup-error-reporting branch from aa800c6 to fd6922f Compare August 28, 2020 18:44
@mikeash
Copy link
Contributor Author

mikeash commented Aug 28, 2020

swiftlang/llvm-project#1702

@swift-ci please test

@mikeash
Copy link
Contributor Author

mikeash commented Aug 28, 2020

swiftlang/llvm-project#1702

@swift-ci please benchmark

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - aa800c67906b49885e167a8bb6a7e1d9de382d60

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - aa800c67906b49885e167a8bb6a7e1d9de382d60

@mikeash
Copy link
Contributor Author

mikeash commented Aug 28, 2020

swiftlang/llvm-project#1702

@swift-ci please benchmark

@swift-ci
Copy link
Contributor

Performance: -O

Improvement OLD NEW DELTA RATIO
Set.isSubset.Int.Empty 51 43 -15.7% 1.19x
Set.isStrictSubset.Int.Empty 51 45 -11.8% 1.13x (?)
Set.isSuperset.Seq.Empty.Int 50 45 -10.0% 1.11x (?)

Code size: -O

Performance: -Osize

Regression OLD NEW DELTA RATIO
ObjectiveCBridgeStubToNSStringRef 70 79 +12.9% 0.89x (?)
RemoveWhereFilterInts 24 27 +12.5% 0.89x
 
Improvement OLD NEW DELTA RATIO
FlattenListFlatMap 4535 3627 -20.0% 1.25x (?)
Set.isStrictSubset.Int.Empty 54 47 -13.0% 1.15x (?)
Set.isDisjoint.Seq.Int.Empty 51 47 -7.8% 1.09x (?)

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview
  Model Name: Mac mini
  Model Identifier: Macmini8,1
  Processor Name: 6-Core Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants