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

linker producing bad code in finally block that causes AOT compiler to seg fault #2181

Closed
stephentoub opened this issue Jul 30, 2021 · 5 comments
Closed

linker producing bad code in finally block that causes AOT compiler to seg fault #2181

stephentoub opened this issue Jul 30, 2021 · 5 comments
Milestone
6.0.x

Comments

@stephentoub
Copy link
Member

See dotnet/runtime#56316 (comment).
@stephentoub stephentoub mentioned this issue Jul 30, 2021
Merged
@MichalStrehovsky MichalStrehovsky mentioned this issue Aug 7, 2021
Open
@vitek-karas
Copy link
Member

What should be simple repro steps - requires macOS (I wasn't able to validate that it does repro yet):

Latest dotnet/runtime repo clone
Remove these lines: https://github.com/dotnet/runtime/blob/7249ec41e059c3900eaf361007e5725033320453/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs#L130-L135

./build.sh -Subset mono+libs -os iOSSimulator -arch x64
./dotnet.sh build /t:Test src/tests/FunctionalTests/iOS/Simulator/AOT /p:TargetOS=iOSSimulator /p:TargetArchitecture=x64

The last command should fail to AOT compile.

@vitek-karas
Copy link
Member

Just an update. The underlying problem is:

  • Sometimes linker edits method bodies by replacing instructions with new instructions
  • One such place is the "isinst" instruction - if the target type is not instantiated we replace the "isinst" with "pop, ldnull" (basically return false).
  • We use Cecils ILProcessor to edit the method body
  • This component simple replaces the instruction objects in the method body collection, unfortunately it doesn't patch other data structures.
  • On such data structure is the exception handler records - after replace these continue to point to the old instruction which not in the method body anymore
  • When such state is saved to IL, the catch/finally block "pointers/offsets" in the IL will end up with effectively random values (typically these just shift a little bit)

Mono AOT compiler is obviously sensitive to such corruption.

The case of the Http3RequestStream is about the "isinst" case above. We have a preliminary fix for it here: https://github.com/vitek-karas/linker/tree/FixBrokenFinally

Unfortunately there are other places where linker edits the method body which all have similar issues. One such place is constant propagation and branch removal - which is what feature switches rely on. That's probably where the CoreLib/ArrayPool issue is - but we were not able to get a repro of that to validate the theory.

We don't have a candidate fix for the feature switch corruption (nor we have a direct repro - even a constructed one, just yet).

This was referenced Aug 11, 2021
Closed
Merged
@vitek-karas
Copy link
Member

The issue with isinst rewrite which affected Http3RequestStream is now fixed. The fix is targeted to isinst and like won't fix whatever problem is in the ArrayPool around finaly sections (although it's like the same root cause).

vitek-karas added a commit that referenced this issue Aug 12, 2021
@vitek-karas
Fix broken try/catch/filter offsets after isinst optimization (#2205)
4dd506a 
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 added a commit to vitek-karas/runtime that referenced this issue Aug 17, 2021
@vitek-karas
Remove workaroudn for linker issue in Http3RequestStream
067c0ec 
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.
@vitek-karas vitek-karas mentioned this issue Aug 17, 2021
Merged
@vitek-karas vitek-karas added this to the .NET 6.0 milestone Aug 18, 2021
@vitek-karas
Copy link
Member

#2213 fixes all of the problems known in this area (and tries to prevent any similar problem in the future).
Keeping this issue opened to track porting the fixes to 6.0 branches.

stephentoub pushed a commit to dotnet/runtime that referenced this issue Aug 18, 2021
@vitek-karas
Remove workaroudn for linker issue in Http3RequestStream (#57571)
b4d75a0 
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.
vitek-karas added a commit to vitek-karas/runtime that referenced this issue Aug 20, 2021
@vitek-karas
Revert workarounds for dotnet/linker#2181
59da2cc 
Trying to revert both.
The Http3RequestStream is definitely fixed by the linker changes.
The ArrayPool ones probably not, but trying to get a repro.
stephentoub pushed a commit to dotnet/runtime that referenced this issue Aug 20, 2021
@vitek-karas
Revert workarounds for dotnet/linker#2181 (#57451)
f53c8dc 
* Revert workarounds for dotnet/linker#2181

Trying to revert both.
The Http3RequestStream is definitely fixed by the linker changes.
The ArrayPool ones probably not, but trying to get a repro.

* Fully revert ArrayPool workaround
@agocke
Copy link
Member

agocke commented Sep 7, 2021

Closing, as it looks like this is fixed.

@agocke agocke closed this as completed Sep 7, 2021
agocke pushed a commit to dotnet/runtime that referenced this issue Nov 16, 2022
@vitek-karas
Fix broken try/catch/filter offsets after isinst optimization (dotnet…
5346ea8 
…/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
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.0.x
Development

No branches or pull requests
3 participants
@agocke @stephentoub @vitek-karas