Skip to content

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

Merged
vitek-karas merged 5 commits intodotnet:mainfrom
vitek-karas:FixBrokenFinally
Aug 12, 2021
Merged

Fix broken try/catch/filter offsets after isinst optimization#2205
vitek-karas merged 5 commits intodotnet:mainfrom
vitek-karas:FixBrokenFinally

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).

@vitek-karas vitek-karas added this to the .NET 6.0 milestone Aug 11, 2021
@vitek-karas vitek-karas requested a review from sbomer August 11, 2021 16:07
@vitek-karas vitek-karas self-assigned this Aug 11, 2021
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!

@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