-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 EventSource trimmer warning #51237
Conversation
…iagnostics/DiagnosticSourceEventSource.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
…iagnostics/DiagnosticSourceEventSource.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
…acing/EventSource.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
…into EventSrcWrnFix2
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Suppressions.Shared.xml
Show resolved
Hide resolved
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.
LGTM - This is great!
@@ -145,6 +145,7 @@ public enum EventOpcode | |||
Send = 9, | |||
Receive = 240, | |||
} | |||
[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.All)] |
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.
Do we really need All
- is that because of the NestedTypes issue?
(we should fix that then - it's been agreed that we should change the behavior of that already).
Opened dotnet/linker#1969 for the likely linker bug. Will not merge until this is understood |
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.
👍 : )
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Show resolved
Hide resolved
This fix is necessary for dotnet#51237 to work properly. Otherwise, the linker may crash when analyzing certain patterns.
* eh fix * test change that inadvertently got checked in earlier * Suppresses the trimmer warning on TypeAnalysis ctor * Incorporating FB * Update src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com> * Update src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com> * Update src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com> * Fix DynamicDependency as per PR feedback * an earlier change got reverted * fixed proj file netcore app condition check * fixed NETCORE_ENGINEERING_TELEMETRY build failures * fixeing another NETCORE_ENGINEERING_TELEMETRY build failures * Adding RequiresUnreferencedCode to TypeAnalysis ctor instead of suppressing the warning to get FB, not fully fixed * PR FB and suppressing warnings for safe calls * propagated the warning all the way up * CI build break fix for one file * excluding NativeRTEventSrc from being build in a project * Missed couple of supppressions on NativeRTEventSrc * build break fixes * Trimmer warning fix related to EventSource manifest creation * incorporate fb * fix build break in some configs * comment feedback * build break Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com> (cherry picked from commit ddaa1c3)
* Include linker fix from dotnet/linker#1972 This fix is necessary for #51237 to work properly. Otherwise, the linker may crash when analyzing certain patterns. * Fix EventSource trimmer warning (#51237) * eh fix * test change that inadvertently got checked in earlier * Suppresses the trimmer warning on TypeAnalysis ctor * Incorporating FB * Update src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com> * Update src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com> * Update src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com> * Fix DynamicDependency as per PR feedback * an earlier change got reverted * fixed proj file netcore app condition check * fixed NETCORE_ENGINEERING_TELEMETRY build failures * fixeing another NETCORE_ENGINEERING_TELEMETRY build failures * Adding RequiresUnreferencedCode to TypeAnalysis ctor instead of suppressing the warning to get FB, not fully fixed * PR FB and suppressing warnings for safe calls * propagated the warning all the way up * CI build break fix for one file * excluding NativeRTEventSrc from being build in a project * Missed couple of supppressions on NativeRTEventSrc * build break fixes * Trimmer warning fix related to EventSource manifest creation * incorporate fb * fix build break in some configs * comment feedback * build break Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com> (cherry picked from commit ddaa1c3) Co-authored-by: Lakshan Fernando <lakshanf@hotmail.com>
Now that the trimmer has capability to preserve derived type members, fixed the warning related to manifest generation by preserving all members of EventSource and its derived types