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

Add a library mode for type hierarchy marking #2114

Merged
merged 6 commits into from
Jul 13, 2021

Conversation

LakshanF
Copy link
Member

@LakshanF LakshanF commented Jun 25, 2021

We added type hierarchy support to the linker via #1929 but the annotations to the derived types were only triggered via object.GetType() in code. This causes problems in library mode linking, especially for internal types when the assembly does not have code that triggers object.GetType(). Library mode linking can remove members in such cases which might be needed later by application code that invokes the removed internal code via reflection. A specific example is an internal type like NetEventSource in System.Net.Mail.dll library, where the library does not have code that triggers object.GetType(). Library mode linking removes nested types in such cases and application code patterns like this will fail in such cases.

Unfortunately, we cannot yet remove the special case handling that we currently have for EventSource in the trimmer with this change as discussed in #1949. This is because some library code relies in the trimmer to remove unused code in custom EventSources and that goal conflicts with this change since this change will keep all code in custom EventSources. There is a runtime issue 54859 that tracks this. This change moves the EventSource special handling to library mode

  • This fix is to allow a new CodeOptimizations, OptimizeTypeHierarchyAnnotations, that will be disabled in library mode linking. Disabling the optimization will cause types to be marked when object.GetType() is not seen and the type is used.
    - EventSource types will not be impacted due to the size concerns expressed above
  • We continue to special case EventSource in the trimmer. This is done by disabling a new option, DisableEventSourceSpecialHandling in library mode.
    - Type hierarchy annotation in EventSource types will not be triggered without object.GetType()
    - Also removed the special handling of the EventDataAttribute at MarkTypeSpecialCustomAttributes in this mode since EventSource type has annotations to keep all methods.
    - We are keeping the special handling of EventSource for earlier runtimes than .NET6.
  • Fixed the existing test case since it was not correctly testing EventSource
  • Added some tests to cover missing scenarios for library mode linking

@vitek-karas
Copy link
Member

We can't take this as is based on the discussion in dotnet/runtime#54859.
For event source we want to keep the existing one-off behavior in library mode (in normal mode we do want to use the type hierarchy marking as it's today).
The problem is that there are other types in the framework which make use of type hierarchy marking - for example https://github.com/dotnet/runtime/blob/46e5fe1c9a4584b006601d46063b353338219af0/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/MaskedTextProvider.cs#L22

For those we want to take this change. I think we'll have to make a "hack" and intentionally ignore the DAM on EventSource in the linker when in library mode. I don't like that solution, but I don't have anything better for now...

@LakshanF
Copy link
Member Author

LakshanF commented Jul 8, 2021

Unfortunately, internal custom EventSources like NetEventSource make it challenging to have this changes unless the library fixes to have only the code they need for each library tracing (or special case EventSource as suggested above). As things stand now, there are 23 [NonEvent] methods to 5 [Event] methods in NetEventSource. This change will keep the 23 methods that are not needed and this NetEventSource source is used in multiple net libraries.

Will move the special casing of EventSource in the trimmer to a library mode as suggested by @marek-safar in a new PR

@LakshanF LakshanF marked this pull request as draft July 8, 2021 17:12
@vitek-karas
Copy link
Member

What about the other use cases for type hierarchy marking - without the proper fix for library mode, it's bound to break (either now, or soon).

@LakshanF LakshanF changed the title Fix special handling of EventSource Add a library mode for type hierarchy marking Jul 9, 2021
@LakshanF
Copy link
Member Author

LakshanF commented Jul 9, 2021

Unfortunately, rebasing caused some changes that are not part of this fix to be displayed

@LakshanF LakshanF marked this pull request as ready for review July 9, 2021 18:53
Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

Looks good - please validate the impact on runtime repo before merging (running the runtime repo build with the new linker should basically make no difference)

src/linker/Linker/LinkContext.cs Outdated Show resolved Hide resolved
src/linker/Linker.Steps/MarkStep.cs Outdated Show resolved Hide resolved
src/linker/Linker/LinkContext.cs Outdated Show resolved Hide resolved
Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

One last nit, otherwise LGTM. Thank you!

@LakshanF LakshanF merged commit e1c0c83 into dotnet:main Jul 13, 2021
@LakshanF LakshanF deleted the EventSourceSpecialFix branch July 13, 2021 16:30
agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022
* Fix special handling of EventSource

FB

* special case EventSource handling to library mode

* test fixes

* Update src/linker/Linker/LinkContext.cs

Co-authored-by: Vitek Karas <[email protected]>

* FB

* FB

Co-authored-by: Vitek Karas <[email protected]>

Commit migrated from dotnet/linker@e1c0c83
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.

4 participants