Skip to content

Remove debug only attributes when Debugger.IsSupported is false.#39237

Merged
eerhardt merged 2 commits intodotnet:masterfrom
eerhardt:TrimDebuggerAttributes
Jul 15, 2020
Merged

Remove debug only attributes when Debugger.IsSupported is false.#39237
eerhardt merged 2 commits intodotnet:masterfrom
eerhardt:TrimDebuggerAttributes

Conversation

@eerhardt
Copy link
Copy Markdown
Member

Contributes to dotnet/linker#1093

Locally, I'm seeing roughly ~100 KB total reduction when enabling Debugger.IsSupported = false after this change. That does include roughly ~8KB other Debugger-only trimming coming from #37288, so the net win with this change is about 90 KB.

@Dotnet-GitSync-Bot
Copy link
Copy Markdown
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link
Copy Markdown

ghost commented Jul 13, 2020

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

</assembly>

<!-- The following attributes are only necessary when debugging is supported -->
<assembly fullname="System.Private.CoreLib" feature="System.Diagnostics.Debugger.IsSupported" featurevalue="false">
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"featurevalue=false" means these can be removed when IsSupported is set to false, right? Just validating it's not actually setting IsSupported to false :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct. Feature and featurevalue in the linker xml files are conditions. “Do this when this feature has this value”.

Copy link
Copy Markdown
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Nice reduction.

</assembly>

<!-- The following attributes are only necessary when debugging is supported -->
<assembly fullname="System.Private.CoreLib" feature="System.Diagnostics.Debugger.IsSupported" featurevalue="false">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As all these are feature-switch covered settings which are not wasm specific and we should move them to shared SPC ILLink.LinkAttributes.xml

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand what makes the feature-switch covered attribute different from the rest of the attributes in this file. Why aren't the rest of them in the shared SPC?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The default SDK setup is trim with copy so it feels odd to include the list of always removed attribute for everyone but I'm not against ;-)

@eerhardt eerhardt closed this Jul 15, 2020
@eerhardt eerhardt reopened this Jul 15, 2020
@eerhardt
Copy link
Copy Markdown
Member Author

CoreCLR Pri0 Runtime Tests Run legs are failing in other PRs as well, so it isn't caused by this change. Merging.

@eerhardt eerhardt merged commit 78fb02b into dotnet:master Jul 15, 2020
@eerhardt eerhardt deleted the TrimDebuggerAttributes branch July 15, 2020 15:46
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@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.

8 participants