ltrzesniewski / InlineIL.Fody

Inject arbitrary IL code at compile time.
MIT License
240 stars 17 forks source link

IL.Emit.Tail() produces wrong IL #7

Closed gfoidl closed 5 years ago

gfoidl commented 5 years ago

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

ltrzesniewski commented 5 years ago

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?

gfoidl commented 5 years ago

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.

ltrzesniewski commented 5 years ago

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.

gfoidl commented 5 years ago

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.

gfoidl commented 5 years ago

An easy fix would be to add [Conditional("Release")] to https://github.com/ltrzesniewski/InlineIL.Fody/blob/f6d2bf1ca6de8412231d972704e43e4cbe00ecc4/src/InlineIL/IL.Emit.cs#L1673

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.

ltrzesniewski commented 5 years ago

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.

gfoidl commented 5 years ago

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.

ltrzesniewski commented 5 years ago

No problem, I'll handle it. Thanks for the report!

ltrzesniewski commented 5 years ago

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.

ltrzesniewski commented 5 years ago

And v1.1.2 handles multiple tail calls in the same method. I hope I got that one right 😄

gfoidl commented 5 years ago

Thanks 👍 for the quick fix!