ltrzesniewski/InlineIL.Fody

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

public static void Tail()

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!