Implement a custom ILProcessor and make it auto-fix IL corruptions #2213
Conversation
Cecil doesn't handle IL editing well. It doesn't patch scopes and branches. Added a linker-private IL processor which does this.
| throw new ArgumentOutOfRangeException (nameof (instruction)); | ||
|
|
||
| Instruction nextInstruction = Instructions.Count == index + 1 ? null : Instructions[index + 1]; | ||
| Instruction previousInstruction = index == 0 ? (Instructions.Count == 1 ? null : Instructions[index + 1]) : Instructions[index - 1]; |
There was a problem hiding this comment.
Is the index+1 case for previousInstruction intentional?
There was a problem hiding this comment.
Yes - if we're removing the first instruction, we have to re-point to "something", in this case the previous and next will be the same, but there's no other way really.
There was a problem hiding this comment.
I'm a little confused by this - if removing the last instruction, we are redirecting scope starts to null. Shouldn't removing the first instruction be symmetric, and redirect scope ends to null?
There was a problem hiding this comment.
You're right. I think the correct behavior here is actually to remove the exception handler if this happens - basically if we try to redirect the "Start" of the exception handler to null, we should remove it.
That said this will never happen in the linker I think, branch removal handles removing of exception handlers on its own. So I would rather leave this as is - and just point it to null. Again - purposefully getting into invalid state, but so be it.
There was a problem hiding this comment.
Agreed - removing the handler might be reasonable, but I think it's ok not to do too much "magic" here if you are getting into such weird states. Same for the case where we remove the last instruction and it's a branch target - in this case you just need to know what you're doing.
|
We will try the upstream, but we can't wait for upstream (even if we authored the fix, typically it takes a while to get it approved/merged). |
| throw new ArgumentOutOfRangeException (nameof (instruction)); | ||
|
|
||
| Instruction nextInstruction = Instructions.Count == index + 1 ? null : Instructions[index + 1]; | ||
| Instruction previousInstruction = index == 0 ? (Instructions.Count == 1 ? null : Instructions[index + 1]) : Instructions[index - 1]; |
There was a problem hiding this comment.
I'm a little confused by this - if removing the last instruction, we are redirecting scope starts to null. Shouldn't removing the first instruction be symmetric, and redirect scope ends to null?
|
I was able to reproduce the failure from ArrayPool. So I added test cases in the TryFinally and TryCatch tests which repro the failure (they would put the I also removed now unnecessary test attribute, since in my previous change the exception handlers are now validated within the instruction stream, so no need for a second attribute to validate these. There are no thorough tests for the IL processor in the various corner cases, since we don't have a way in the linker to hit these currently. And I didn't want to try to add true unit tests - it didn't seem worth it. I think this is now ready for review - since it provably fixes a real issue. |
|
@agocke We will want to take this for .NET 6 - not sure what you'll need for approval, please let me know. |
Co-authored-by: Sven Boemer <[email protected]>
|
I was able to verify that the repro case works with these changes - no mono AOT compiler failures. Will merge once CI passes. |
…otnet#2213) Cecil doesn't handle IL editing well. It doesn't patch scopes and branches. Added a linker-private IL processor which does this. Added tests for the IL corruption when removing branches around code which has exception handlers. I also removed now unnecessary test attribute, since in my previous change the exception handlers are now validated within the instruction stream, so no need for a second attribute to validate these. Co-authored-by: Sven Boemer <[email protected]> (cherry picked from commit e9403e2)
[release/6.0-rc1] Fix IL corruption (#2213)
…otnet/linker#2213) Cecil doesn't handle IL editing well. It doesn't patch scopes and branches. Added a linker-private IL processor which does this. Added tests for the IL corruption when removing branches around code which has exception handlers. I also removed now unnecessary test attribute, since in my previous change the exception handlers are now validated within the instruction stream, so no need for a second attribute to validate these. Co-authored-by: Sven Boemer <[email protected]> Commit migrated from dotnet/linker@e9403e2
Cecil doesn't handle IL editing well. It doesn't patch scopes and branches. Added a linker-private IL processor which does this.