Skip to content
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

Strip the ILLinkTrim.xml from the System.Diagnostics.StackTrace assembly #37659

Merged
merged 1 commit into from
Jun 9, 2020

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Jun 9, 2020

The PreserveDependency is already in place -

[PreserveDependency("GetSourceLineInfo", "System.Diagnostics.StackTraceSymbols", "System.Diagnostics.StackTrace")]
[PreserveDependency(".ctor()", "System.Diagnostics.StackTraceSymbols", "System.Diagnostics.StackTrace")]
.

@layomia layomia added this to the 5.0 milestone Jun 9, 2020
@layomia layomia requested review from eerhardt and joperezr June 9, 2020 18:36
@layomia layomia self-assigned this Jun 9, 2020
@ghost
Copy link

ghost commented Jun 9, 2020

Tagging subscribers to this area: @tommcdon, @krwq
Notify danmosemsft if you want to be subscribed.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM.

Side note: How are we tracking scenarios to add tests for?

@stephentoub
Copy link
Member

stephentoub commented Jun 9, 2020

I'm not understanding the change. I thought the purpose of this ILLinkTrim_LibraryBuild.xml was to keep the internal and unused API alive through the assembly build such that it would be available to corelib, and then corelib's PreserveDependency ensures it's not trimmed from a consuming app. Why is the PreserveDependency in corelib sufficient to keep the ctor alive in the other assembly in that assembly's build?

@stephentoub
Copy link
Member

Oh! I see, this is renaming it to be ILLinkTrim_LibraryBuild.xml, not deleting it. Ok.

@joperezr
Copy link
Member

joperezr commented Jun 9, 2020

Side note: How are we tracking scenarios to add tests for?

I think we are not 😢. I'm hoping to merge my test PR as soon as possible and will ping you and @layomia in case you had attributes you wanted to add tests for. Or we can also just add a new issue to track that work.

@joperezr
Copy link
Member

joperezr commented Jun 9, 2020

@layomia just to be sure, how did you look for places that required StackTraceSymbols? I'm just asking so we make sure we checked all platforms in netcoreapp in case we needed an additional attribute to be placed.

@layomia
Copy link
Contributor Author

layomia commented Jun 9, 2020

how did you look for places that required StackTraceSymbols?

In this case, I discussed with @eerhardt offline and performed this search. I realize that this static search may not uncover all the usages, and I have limited context regarding the various APIs and areas we are performing linker work for. So, I'm sort of relying on area owners to provide additional context. Not sure if there's additional tooling to help out here.

@joperezr
Copy link
Member

joperezr commented Jun 9, 2020

Not sure if there's additional tooling to help out here.

Fair enough that seems ok to me. @stephentoub are you aware of any other places where we might be using this via reflection? @eerhardt any other tips you have for easily finding linker blind spots for a specific API?

@eerhardt
Copy link
Member

eerhardt commented Jun 9, 2020

any other tips you have for easily finding linker blind spots for a specific API?

Searching is the best weapon I have as of now. That's why reflection is bad 😄.

For this one, the only place I am aware that is using this method is the code in CoreLib @layomia called out, which is already annotated with PreserveDependency. The way you trigger the code is to get an exception and inspect the stack trace and see if line numbers are inserted into the trace.

@layomia
Copy link
Contributor Author

layomia commented Jun 9, 2020

How are we tracking scenarios to add tests for?

Or we can also just add a new issue to track that work.

I opened #37677.

@layomia layomia merged commit d218662 into dotnet:master Jun 9, 2020
@layomia layomia deleted the stacktrace branch June 9, 2020 22:49
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants