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.

Sign up for GitHub

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

Jump to bottom

Resolve ILLink warnings in System.Diagnostics.DiagnosticSource #50265

Merged
merged 3 commits into from
Apr 7, 2021
Merged

Resolve ILLink warnings in System.Diagnostics.DiagnosticSource #50265

merged 3 commits into from
Apr 7, 2021

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Mar 25, 2021 •
edited
Loading

Contributes to #45623

Addresses all warnings in System.Diagnostics.DiagnosticSource with the following plan:

  1. Add to the runtime message when a property can't be found that it may have been trimmed away.
  2. Mark DiagnosticSource.Write(string,object) as RequiresUnreferencedCode
  3. Suppress the warnings for any .NET libraries that call the DiagnosticSource.Write() API, and annotate the .NET types being passed in to preserve their important properties.
    • This was done for HttpClient. ASP.NET and EF will need separate changes when those assemblies are made trim compatible
  4. Annotate Activity and its small closure of types (ActivityLink, ActivityEvent, ActivityContext, etc) to ensure none of those properties are trimmed.
  5. Suppress trim warnings inside DiagnosticSourceEventSource since the public Write method is marked with RequiresUnreferencedCode.
  6. Logged Add DiagnosticSource.Write<T> API to assist with trimming #50454 to add a new Write API that will automatically preserve all the top-level properties on the object (but will still be RequiresUnreferencedCode since sub-properties can be selected).

The DynamicDependency attributes on FetcherForProperty are no longer needed, as the trimmer knows about this Reflection pattern intrinsically and preserves the constructors correctly.

@ghost
Copy link

ghost commented Mar 25, 2021

Tagging subscribers to this area: @tommcdon, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #45623

There are 2 warnings in System.Diagnostics.DiagnosticSource. This solves the first one - when a FilterAndPayloadSpecs is selecting properties that are trimmed from the application, write out a message that informs the user that the property may have been trimmed.

Resolving the second warning needs more thinking - maybe introducing a new API, or a new trimmer feature is needed to fix it. See #50130 (comment) for a write up on that warning.

The DynamicDependency attributes are no longer needed, as the trimmer knows about this Reflection pattern intrinsically and preserves the constructors correctly.

Author: eerhardt
Assignees: -
Labels:

area-System.Diagnostics
Milestone: -

@eerhardt eerhardt added the linkable-framework Issues associated with delivering a linker friendly framework label Mar 25, 2021
@ghost
Copy link

ghost commented Mar 25, 2021

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #45623

There are 2 warnings in System.Diagnostics.DiagnosticSource. This solves the first one - when a FilterAndPayloadSpecs is selecting properties that are trimmed from the application, write out a message that informs the user that the property may have been trimmed.

Resolving the second warning needs more thinking - maybe introducing a new API, or a new trimmer feature is needed to fix it. See #50130 (comment) for a write up on that warning.

The DynamicDependency attributes are no longer needed, as the trimmer knows about this Reflection pattern intrinsically and preserves the constructors correctly.

Author: eerhardt
Assignees: -
Labels:

area-System.Diagnostics, linkable-framework
Milestone: -

@eerhardt eerhardt mentioned this pull request Mar 30, 2021
Closed
@eerhardt
Update based on the following plan:
d6ef851 
1. Mark DiagnosticSource.Write(string,object) as RequiresUnreferencedCode
2. Suppress the warnings for any .NET libraries that call the DiagnosticSource.Write() API, and annotate the .NET types being passed in to preserve their important properties.
    - This was done for HttpClient. ASP.NET and EF will need separate changes when those assemblies are made trim compatible
3. Annotate Activity and its small closure of types (ActivityLink, ActivityEvent, ActivityContext, etc) to ensure none of those properties are trimmed.
4. Suppress trim warnings inside DiagnosticSourceEventSource since the public Write method is marked with RequiresUnreferencedCode.
@eerhardt eerhardt force-pushed the DiagnosticSourceILLinkWarnings branch from 63b5ce9 to d6ef851 Compare March 30, 2021 22:56
@eerhardt eerhardt changed the title Resolve ILLink warnings in System.Diagnostics.DiagnosticSource (Round 1) Resolve ILLink warnings in System.Diagnostics.DiagnosticSource Mar 30, 2021
noahfalk
noahfalk approved these changes Mar 31, 2021
View reviewed changes
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

LGTM!

cc @josalem @sywhang @davmason

@eerhardt eerhardt merged commit 6578f25 into dotnet:main Apr 7, 2021
@eerhardt eerhardt deleted the DiagnosticSourceILLinkWarnings branch April 7, 2021 12:30
thaystg added a commit to thaystg/runtime that referenced this pull request Apr 7, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_fix_42327
b5346a6 
* upstream/main: (568 commits)
  [wasm] Set __DistroRid on Windows to browser-wasm (dotnet#50842)
  [wasm] Fix order of include paths, to have the obj dir first (dotnet#50303)
  [wasm] Fix debug build of AOT cross compiler (dotnet#50418)
  Fix outdated comment (dotnet#50834)
  [wasm][tests] Add properties to allow passing args to xharness (dotnet#50678)
  Vectorized common String.Split() paths (dotnet#38001)
  Fix binplacing symbol files. (dotnet#50819)
  Move type check to after the null ref branch in out marshalling of blittable classes. (dotnet#50735)
  Remove extraneous CMake version requirement. (dotnet#50805)
  [wasm] Remove unncessary condition for EMSDK (dotnet#50810)
  Add loop alignment stats to JitLogCsv (dotnet#50624)
  Resolve ILLink warnings in System.Diagnostics.DiagnosticSource (dotnet#50265)
  Avoid unnecessary closures/delegates in Process (dotnet#50496)
  Fix for field layout verification across version bubble boundary (dotnet#50364)
  JIT: Enable CSE for VectorX.Create (dotnet#50644)
  [main] Update dependencies from mono/linker (dotnet#50779)
  [mono] More domain cleanup (dotnet#50771)
  Race condition in Mock reference tracker runtime with GC. (dotnet#50804)
  Remove IAssemblyName (and various fusion remnants) (dotnet#50755)
  Disable failing test for GCStress. (dotnet#50828)
  ...
@antonfirsov antonfirsov mentioned this pull request Apr 12, 2021
Closed
@eerhardt eerhardt mentioned this pull request Apr 21, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators May 7, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vitek-karas vitek-karas vitek-karas left review comments

@jkotas jkotas jkotas left review comments

@noahfalk noahfalk noahfalk approved these changes

@LakshanF LakshanF Awaiting requested review from LakshanF

@tarekgh tarekgh Awaiting requested review from tarekgh

Assignees
No one assigned
Labels
area-System.Diagnostics linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@eerhardt @noahfalk @jkotas @vitek-karas @tarekgh @karelz