Crash on ref return

[keith-anders]

ref return parameters were added in C# 7.0. However, weaving a method with such a return type causes a runtime exception when the OnExit handler attempts to access the MethodExecutionArgs's ReturnValue property.

Managed Debugging Assistant 'FatalExecutionEngineError'
Message=Managed Debugging Assistant 'FatalExecutionEngineError' : 'The runtime has encountered a fatal error. The address of the error was at 0x5ff4e838, on thread 0x424c. The error code is 0xc0000005. This error may be a bug in the CLR or in the unsafe or non-verifiable portions of user code. Common sources of this bug include user marshaling errors for COM-interop or PInvoke, which may corrupt the stack.'

That can be reproduced with this simple example:

class Aspect : OnMethodBoundaryAspect
{
    public override void OnExit(MethodExecutionArgs arg)
    {
        Console.WriteLine("Return value was {0}", arg.ReturnValue);
    }
}

class Program
{
    static void Main(string[] args)
    {
        Test.DoThing();
    }
}
    
static class Test
{
    public static int m_field;
        
    [Aspect]
    public static ref int DoThing()
    {
        return ref m_field;
    }
}

In short, MBA does not properly convert the ref return type to a non-ref object before setting it as the ReturnValue property of the MethodExecutionArgs. I went to add a test for this so I could fix it, and I ran into a different but related issue. Specifically, PEVerify considers ref return types to be invalid, even the latest version of PEVerify from the .NET 4.7.2 sdk running against a simple assembly output by the compiler with no code weaver involved. As such, whenever I try to add a method to the unit test assembly that uses a ref return type, the test fails because PEVerify fails, even though the code actually runs just fine.

I was able to find no evidence of short-term plans by Microsoft to update the verifier to account for this new language feature, so the only way I can get it to work at the moment is by telling PEVerify to ignore this particular error. I have a fix ready to PR which would fix the issue above, but the only way I can get the tests to pass is by simply disabling that error. Should I submit it as is (with the error disabled) or do you want to try to find another way to handle these issues?

[Ralf1108]

hi, yes I already discovered that PE Verify doesnt support newer c# language versions very well.

see dotnet/roslyn#26196

So for now mark the test with an "PEVerifyNotSupported" attribute so we find them later more easy.
We currently dont have another issue...so just make your PR with skipped PE verify

[keith-anders]

I can mark the test with an attribute so that it can be easily revisited later, but I still have to ignore this particular PEVerify error for all tests, not just mine. This is because the weaved version of TestAssembly will still have the ref return from the unweaved assembly even though the weaver didn't touch that class. When PEVerify finds that class, it won't care that the weaver didn't produce it; it will just fail. So if I include the code necessary for my test in the TestAssembly, then I have to make PEVerify ignore this error for all tests.

I just wanted to confirm that this is how I should proceed.

[Ralf1108]

can't you just use a flag which will be forwarded to the PEVerify step which will then be skipped?
Maybe a field or base class method will do it, e.g. "SkipPEVerify()".

If it is possible I want to keep the pe verify for the others tests to be sure.

[keith-anders]

I mean, I can skip PEVerify in my test, but the problem is that adding the needed method to the TestAssembly makes the TestAssembly unverifiable for all tests, no matter whether the weaver touched it or not. I could add a second test assembly that's not verified, but that feels contrived to me.

Note that I wasn't proposing not verifying. I was proposing passing a flag in the PEVerify command line arguments that would cause it to ignore the error that it throws because of ref returns.

[keith-anders]

Ok, I added a PR that I think is probably the best way of handling it: leave the existing TestAssembly fully verified in all the tests that use it, but have a separate test assembly that contains unverifiable code, and ignore the errors it's known to produce only when verifying that assembly. Let me know if any problems with this approach.

[Ralf1108]

fixed by #34