ILASM - Fixing ILASM roundtrip issue with vararg calling convention

[TIHan]

Should resolve: #72759

Description

ILASM was failing to emit the correct callsite signature for a method with a vararg calling convention if one of the parameters was generic instantiation of a type.

In ResolveLocalMemberRefs, it seems we lost some information when getting the fixed signature for vararg callconv and it creates a bad signature for the new member reference.

There are two solutions:

1. Remove special handling for vararg callconv in ResolveLocalMemberRefs as we already have the correct signature from m_LocalMethodRefDList.
2. Fix CountBytesOfOneArg to handle ELEMENT_TYPE_GENERICINST as described by @jakobbotsch

The PR is now using solution number 2.

I manually tested this and it resolves the issue reported. I also tried this scenario:
ClassLibrary1.dll

using System.Runtime.Intrinsics;

namespace ClassLibrary1
{
    public class Class1
    {
        public static int paramLength(Vector128<int> arg, __arglist)
        {
            ArgIterator iterator = new ArgIterator(__arglist);
            return iterator.GetRemainingCount();
        }
    }
}

vararg_test.exe

using ClassLibrary1;
using System.Runtime.Intrinsics;

Console.WriteLine(Class1.paramLength(Vector128<int>.AllBitsSet, __arglist(1, 2)));

This scenario works with the PR changes, so this gives me confidence in the change as this is across assembly boundaries.

Acceptance Criteria

• No additional ilasm roundtrip failures in CI

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Should resolve: #72759

Description

ILASM was failing to emit the correct callsite signature for a method with a vararg calling convention if one of the parameters was generic instantiation of a type.

This is what ECMA 335, Section II.15.4.5 vararg methods says:

A MethodRef might be needed even if the method is
defined in the same assembly, because the MethodDef only describes the fixed part of the argument
list. If the call site does not pass any additional arguments, then it can use the MethodDef for vararg
methods defined in the same assembly. end note]

In ILASM method HRESULT Assembler::ResolveLocalMemberRefs(), there was special handling for methods with the vararg calling convention that would get the fixed signature (includes the fixed part of the argument list) of the method, then it would search for a member reference with that signature.
Then, it could not find the member reference in the list because the signature is different from what was actually used on the callsite. ECMA 335 states:

The Signature at a call site differs from that at its definition, only for a method with the VARARG
calling convention.

So I think the reason it couldn't find the member reference is because getting the fixed signature is effectively the member definition and despite being in the same assembly, the member reference and member definition signatures are different.

Now, there is logic to create a member reference if we could not find it in the member reference list, but it seems we lost some information when getting the fixed signature and it creates a bad signature for the new member reference.

The solution seems to be to simply remove this special handling as we already have the correct signature from m_LocalMethodRefDList.

I manually tested this and it resolves the issue reported. I also tried this scenario:
ClassLibrary1.dll

using System.Runtime.Intrinsics;

namespace ClassLibrary1
{
    public class Class1
    {
        public static int paramLength(Vector128<int> arg, __arglist)
        {
            ArgIterator iterator = new ArgIterator(__arglist);
            return iterator.GetRemainingCount();
        }
    }
}

vararg_test.exe

using ClassLibrary1;
using System.Runtime.Intrinsics;

Console.WriteLine(Class1.paramLength(Vector128<int>.AllBitsSet, __arglist(1, 2)));

This scenario works with the PR changes, so this gives me confidence in the change as this is across assembly boundaries.

Acceptance Criteria

• No additional ilasm roundtrip failures in CI

Author:	TIHan
Assignees:	-
Labels:	area-ILTools-coreclr
Milestone:	-

[TIHan]

@dotnet/jit-contrib This is ready, pending CI runs

[jakobbotsch]

it seems we lost some information when getting the fixed signature and it creates a bad signature for the new member reference.

There are other uses of this _GetFixedSigOfVarArg function, does it need to be fixed instead it if is returning a bad signature?

If I understand the fix here right it will cause ilasm to unnecessarily create TypeRef tokens to types defined within the same module, which seems a little wasteful (though given that it is only for varargs, probably not a big deal).

[jakobbotsch]

There are other uses of this _GetFixedSigOfVarArg function, does it need to be fixed instead it if is returning a bad signature?

It looks suspect to me that _CountBytesOfOneArg used by _GetFixedSigOfVarArg does not handle ELEMENT_TYPE_GENERICINST.

[TIHan]

There are other uses of this _GetFixedSigOfVarArg function, does it need to be fixed instead it if is returning a bad signature?

Possibly, I didn't know exactly what might need to be handled in there and I think the runtime uses this as well. To minimize any risk, I went with just the change only in ILASM.

If I understand the fix here right it will cause ilasm to unnecessarily create TypeRef

Is this when the tkMemberDef is 0? When debugging, it actually does not go down that path.

It looks suspect to me that _CountBytesOfOneArg used by _GetFixedSigOfVarArg does not handle ELEMENT_TYPE_GENERICINST.

I think this might be it. I just tried to handle this, and when checking it under ILDASM I can see the parameter type starting to form:

    IL_000b:  call       vararg int32 Runtime_71375::VarArgs(int32,
                                                             int32,
                                                             int32,
                                                             int32,
                                                             int32,
                                                             int32,
                                                             valuetype [System.Runtime.Intrinsics]System.Runtime.Intrinsics.Vector128`1<void,,int32,,,,,>)

I'll see if I can get it working.

[TIHan]

Updated the PR to handle ELEMENT_TYPE_GENERICINST in CountBytesOfOneArg and reverted the other fix in favor of it.

@dotnet/jit-contrib ready for review again pending CI runs.