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

Fix broken try/catch/filter offsets after isinst optimization #2205

Merged
merged 5 commits into from
Aug 12, 2021

Conversation

vitek-karas
Copy link
Member

The isinst optimization replaces that instruction with a pop, ldnull when the type is not instantiated. This changes the instruction and also changes the length of the instruction in that position. Cecil unfortunately doesn't update try/catch/filter references and they keep pointing to the old isinst instruction which not part of the method body anymore. When saving the assembly the offsets stores in the try/catch/filter records end up effective random and corrupted.

This is a short-term fix to unblock failures in runtime due to this problem.

Medium term fix would be to carefully handle all IL replacements in the linker with regard to try/catch/filter records.

Ideally the long term fix would be to do this in Cecil in such a way that IL replacements would be correctly handled on their own.

This fixes the Http3RequestStream failures mentioned in #2181, but I was not able to confirm if this fixes the CoreLib ArrayPool issues as well (I think it will not).

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.

LGTM!

test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs Outdated Show resolved Hide resolved
@vitek-karas
Copy link
Member Author

The isinst optimization replaces that instruction with a pop, ldnull when the type is not instantiated. This changes the instruction and also changes the length of the instruction in that position. Cecil unfortunately doesn't update try/catch/filter references and they keep pointing to the old isinst instruction which not part of the method body anymore. When saving the assembly the offsets stores in the try/catch/filter records end up effective random and corrupted.

This is a short-term fix to unblock failures in runtime due to this problem.

Medium term fix would be to carefully handle all IL replacements in the linker with regard to try/catch/filter records.

Ideally the long term fix would be to do this in Cecil in such a way that IL replacements would be correctly handled on their own.

This fixes the Http3RequestStream failures mentioned in #2181, but I was not able to confirm if this fixes the CoreLib ArrayPool issues as well (I think it will not).

@vitek-karas vitek-karas merged commit 4dd506a into dotnet:main Aug 12, 2021
@vitek-karas vitek-karas deleted the FixBrokenFinally branch August 12, 2021 11:48
vitek-karas added a commit to vitek-karas/runtime that referenced this pull request Aug 17, 2021
Part of the linker issue dotnet/linker#2181 has been fixed in dotnet/linker#2205. This part was the one affecting Http3RequestStream.

This change simple reverts the workaround since it's not needed anymore.
stephentoub pushed a commit to dotnet/runtime that referenced this pull request Aug 18, 2021
Part of the linker issue dotnet/linker#2181 has been fixed in dotnet/linker#2205. This part was the one affecting Http3RequestStream.

This change simple reverts the workaround since it's not needed anymore.
agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022
…/linker#2205)

The isinst optimization replaces that instruction with a pop, ldnull when the type is not instantiated. This changes the instruction and also changes the length of the instruction in that position. Cecil unfortunately doesn't update try/catch/filter references and they keep pointing to the old isinst instruction which not part of the method body anymore. When saving the assembly the offsets stores in the try/catch/filter records end up effective random and corrupted.

This is a short-term fix to unblock failures in runtime due to this problem.

Medium term fix would be to carefully handle all IL replacements in the linker with regard to try/catch/filter records.

Ideally the long term fix would be to do this in Cecil in such a way that IL replacements would be correctly handled on their own.

This fixes the Http3RequestStream failures mentioned in dotnet/linker#2181, but I was not able to confirm if this fixes the CoreLib ArrayPool issues as well (I think it will not).

Commit migrated from dotnet/linker@4dd506a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants