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

[stephentoub]

See dotnet/runtime#56316 (comment).

[vitek-karas]

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]

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

[vitek-karas]

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]

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