Skip to content

Fix DebuggableAttribute in InjectModuleInitializer post-build targets #1744

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

Merged
merged 2 commits into from
Aug 29, 2019

Conversation

rladuca
Copy link
Member

@rladuca rladuca commented Aug 29, 2019

Addresses #1549

In the ReconstituteDll target, we call ILAsm to merge a module constructor into the assembly we previously disassembled. We need to enable JIT optimizations here so that we get an appropriate DebuggableAttribute on the assembly and don't cause perf problems.

@vatsan-madhavan who helped debug and find this fix.

@rladuca rladuca added Bug Product bug (most likely) area-infrastructure Performance Performance related issue labels Aug 29, 2019
@rladuca rladuca added this to the 3.0 milestone Aug 29, 2019
@rladuca rladuca self-assigned this Aug 29, 2019
@ghost ghost requested review from vatsan-madhavan, ryalanms and stevenbrix August 29, 2019 02:25
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Aug 29, 2019
@ghost ghost requested a review from SamBent August 29, 2019 02:25
@vatsan-madhavan vatsan-madhavan added the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 29, 2019
@ericstj
Copy link
Member

ericstj commented Aug 29, 2019

Curious why this needs to be specified and it isn't just persisted from the source. Is that a bug in ILAsm or ILDasm? Have we filed that issue?

@rladuca
Copy link
Member Author

rladuca commented Aug 29, 2019

Curious why this needs to be specified and it isn't just persisted from the source. Is that a bug in ILAsm or ILDasm? Have we filed that issue?

I'll have to check with later versions of .NET IL(D)Asm/ILAsm. We're pegged to .NET 4.6.2 due to an issue with the MSBuild tool locator, but also due to this bug with round-tripping in .NET 4.8.

We only just found out about these in the last couple of days, but we'll be sure to file the appropriate issues on both teams.

@rladuca
Copy link
Member Author

rladuca commented Aug 29, 2019

Curious why this needs to be specified and it isn't just persisted from the source. Is that a bug in ILAsm or ILDasm? Have we filed that issue?

I'll have to check with later versions of .NET IL(D)Asm/ILAsm. We're pegged to .NET 4.6.2 due to an issue with the MSBuild tool locator, but also due to this bug with round-tripping in .NET 4.8.

We only just found out about these in the last couple of days, but we'll be sure to file the appropriate issues on both teams.

In the disassembled IL, this attribute is commented out and it indicates this will be added automatically. When I did a recombination of DirectWriteForwarder and PresentationCore with IL(D)Asm, we saw the perf increase as well. The difference here, I believe, is passing the debug flag to get PDB output. When you do that (as we do in WPF builds) I think ILAsm overrides the attribute, requiring you to manually specify optimizations to be enabled.

@ericstj
Copy link
Member

ericstj commented Aug 29, 2019

I see, makes sense.

@rladuca rladuca removed the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 29, 2019
@rladuca
Copy link
Member Author

rladuca commented Aug 29, 2019

Approved by tactics, merging.

@rladuca rladuca merged commit f0c10e1 into master Aug 29, 2019
@rladuca rladuca deleted the dev/roladuca/ilasmfix branch August 29, 2019 20:34
@ghost ghost locked as resolved and limited conversation to collaborators Apr 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-infrastructure Bug Product bug (most likely) Performance Performance related issue PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants