IL.Emit.Tail() produces wrong IL
gfoidl opened this issue ยท 11 comments
As tail. is a prefix instruction, the only valid code sequence is tail. call
(or calli or callvirt) (see link).
Tail();
Calli(new StandAloneMethodSig(CallingConvention.Cdecl, typeof(void)));
produces
IL_0007: tail.
IL_0009: calli void()
instead of (the expected)
IL_0007: tail. calli void()
(the space after the .
is sensitive though).
Hmm... are you sure it's not an artifact of your decompiler? I mean; does your code actually fail at JIT time?
InlineIL has a unit test for prefix instructions which passes.
Assuming it's the decompiler: If you compile in debug mode, it could be an issue with the way InlineIL handles sequence points, which could confuse the decompiler. Can you check if you have this issue in release mode?
Debug
Decompiler shows IL as above.
Runtime throws
System.InvalidProgramException
HResult=0x8013153A
Message=Common Language Runtime detected an invalid program.
Release
Decompiler shows IL as above. Decompiler is ILSpy and ildasm. Both show the same.
Runtime executes just fine.
Ok so it looks like it's normal decompiler behavior (I believe the display of the prefix on a separate line is intended) but the discrepancy between debug and release is definitely a bug.
I'll try to reproduce this with a simple tail+calli case but I won't have much time until next week.
Thx for investigating this, and don't worry about the time -- for me it's not urgent though. Just wanted to report that, so it can be fixed (sometime).
Minimal repro is
using System;
using System.Reflection;
using InlineIL;
using static InlineIL.IL.Emit;
namespace ConsoleApp2
{
class Program
{
static void Main()
{
Ldftn(new MethodRef(new TypeRef(typeof(Program)), nameof(Do)));
Tail();
Calli(new StandAloneMethodSig(CallingConventions.Standard, typeof(void)));
}
private static void Do()
{
Console.WriteLine("Hi");
}
}
}
Behavior of netcoreapp2.2 and netcoreapp3.0 is the same.
An easy fix would be to add [Conditional("Release")]
to
InlineIL.Fody/src/InlineIL/IL.Emit.cs
Line 1673 in f6d2bf1
This also requires to add the Release
constant to be defined.
So in Release
the tail-call is emitted, whilst in Debug
it is not. The correctness of the resulting program should not be affected by this.
I can submit a PR (inc. tests) tomorrow if you want.
Thanks, but that wouldn't fix the underlying issue, only hide it. I need to understand what's wrong there.
If you feel like doing a real fix, that would be nice but it won't be the same difficulty ๐
Otherwise don't worry I'll take a look at it myself in a few days.
like doing a real fix, that would be nice
Unfortunately I have nearly no knowledge on how to use Cecil, and have to dig through your code first...
I agree, that a real fix is better than hiding the symptoms.
No problem, I'll handle it. Thanks for the report!
Well that was a pretty stupid oversight: debug builds insert nop
instructions between lines of code. Your example compiles to something similar to the following:
IL_0008: tail.
IL_000A: calli void ()
IL_000F: nop
IL_0010: ret
Which is incorrect because a tail call needs to be immediately followed by a ret
.
Non-void calls are even worse:
IL_000B: tail.
IL_000D: calli int32 (int32)
IL_0012: nop
IL_0013: stloc.0
IL_0014: br.s IL_0016
IL_0016: ldloc.0
IL_0017: ret
The value goes into a local so the debugger can display it before the function returns, but that gets in the way too.
InlineIL will now validate tail calls and remove the boilerplate code, leaving just ret
, but that will fail horribly if you have multiple tail calls in a single method. I'll decide what to do with this case later. In the meantime, v1.1.1 should fix the issue reported here.
And v1.1.2 handles multiple tail calls in the same method. I hope I got that one right ๐
Thanks ๐ for the quick fix!