-
Notifications
You must be signed in to change notification settings - Fork 715
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
Fix Linux NGEN/R2R symbols #436
Conversation
Note that there is a break in the tests that looks like it is caused by this change. (ArgumentOutOfRange in the ZipDump test).
|
var suffixStart = name.IndexOf(NISuffix); | ||
var suffixEnd = suffixStart + NISuffix.Length - 1; // Leave the trailing '.' from ".ni." | ||
start = 0; | ||
end = name.Length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assignments of start and end are dead at this point. You should remove them.
Codecov Report
@@ Coverage Diff @@
## master #436 +/- ##
=========================================
Coverage ? 17.54%
=========================================
Files ? 213
Lines ? 122889
Branches ? 11839
=========================================
Hits ? 21557
Misses ? 100474
Partials ? 858
Continue to review full report at Codecov.
|
Thanks for the review @vancem. These should be fixed now. |
|
||
if (stripNiSuffix) | ||
{ | ||
var suffixStart = name.IndexOf(NISuffix); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised this works as you intend because NISuffix =".ni." (that is it has a trailing dot).
I would have expected that typically you would have a path = Foo.ni.dll and name would be Foo.ni (that is no trailing dot), and thus name.IndexOf(NISuffix) would FAIL, and the new code will never do anything.
Are we sure that actually works (it puts both Foo AND Foo.ni in the table)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The input path is for the map file and not for the NI. An example path looks like "traces/perfdemo.trace/System.Console.ni.{d2de1c76-d19d-4c52-8ff5-70e88349ea91}.map" Originally, this function reduced this to "System.Console.ni.{d2de1c76-d19d-4c52-8ff5-70e88349ea91}". If you specify true for stripNiSuffix then you get "System.Console.{d2de1c76-d19d-4c52-8ff5-70e88349ea91}". For .NET Core 2.0, the perfinfo-$pid.map file no longer contains the ".ni" and that's where things were breaking.
I have confirmed that it does work, and that we do put both Foo and Foo.ni in the table. You can see where we put both into the table here: https://github.com/Microsoft/perfview/pull/436/files#diff-151b5622877b35646b2ea5e3c9466af6R701
As of .NET Core 2.0, crossgen no longer includes the .ni.dll suffix for native images. PerfView still expects the .ni.dll suffix, and so it is not able to resolve any NGEN or R2R image symbols.
This PR fixes the issue in a backwards compatible way, allowing resolution of the image with or without the ni suffix.