Fix missing case for function pointer signatures in Crossgen2

[trylek]

As Andy Ayers conjectured in the issue #72822 and I confirmed,
Crossgen2 signature emitter was missing the code path for
emitting function pointer signatures. This change fixes this
deficiency. The change also removes the issues.targets exclusion
of the affected test that Andy originally merged in to mitigate
the issue.

Thanks

Tomas

P.S. Even though MethodSignatureFlags more or less corresponds
to CoreCLR runtime calling convention, I have ignored this fact
and implemented an explicit conversion as I find it cleaner, please
let me know if you think otherwise. I would also appreciate
double-checking of the logic around Static vs. HASTHIS as I'm not
completely sure about the exact interpretation of this runtime flag.

/cc @dotnet/crossgen-contrib

Fixes: #72822

[jkotas]

Is IMAGE_CEE_CS_CALLCONV_EXPLICITTHIS right here?

[trylek]

Hmm, that's exactly what I'm trying to get my head around. I believe that ECMA managed method signatures do include the this pointer so I would assume it's explicit but I may be misunderstanding the purpose of the flag, it's not used very broadly in the CoreCLR native runtime.

[jkotas]

The MethodSignature initialization only uses HasThis to initialize the Static bit:

runtime/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaSignatureParser.cs

Lines 399 to 400 in 83bd401

	if (!header.IsInstance)
	flags |= MethodSignatureFlags.Static;

so the reverse should do the same.

If we need to preserve HasExplicitThis too (I do not think we need to), we would need to store it in MethodSignature.

[trylek]

Based on your clarification I have removed the ExplicitThis flag when emitting the non-static function pointer signatures.

[jkotas]

I am still not sure about the ExplicitThis. I have looked into it some more and I think we should probably store it on MethodSignature when we are initializing it from Ecma metadata, store it here; and also send it to the JIT so that the JIT can do the right thing with it.

[jkotas]

We have number of IL tests that exercise explicit this (look for "explicit instance" in *.il under tests). I am surprised that none of them fails.

[davidwrighton]

Ack, yes, we should handle explicitthis, although fixing all the places that work with that may be beyond the scope of what we should try to fix with this PR. I think it will likely be a fairly large fix to a bunch of places unrelated to the general thrust of the code written here.

[trylek]

I have created a .NET 8 tracking issue for this task, #73407.

[jkotas]

I have ignored this fact
and implemented an explicit conversion as I find it cleaner,

FWIW, the type system takes the casting shortcut:

runtime/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaSignatureParser.cs

Lines 389 to 394 in 83bd401

	// Verify that it is safe to convert CallingConvention to MethodSignatureFlags via a simple cast
	Debug.Assert((int)MethodSignatureFlags.UnmanagedCallingConventionCdecl == (int)SignatureCallingConvention.CDecl);
	Debug.Assert((int)MethodSignatureFlags.UnmanagedCallingConventionStdCall == (int)SignatureCallingConvention.StdCall);
	Debug.Assert((int)MethodSignatureFlags.UnmanagedCallingConventionThisCall == (int)SignatureCallingConvention.ThisCall);
	Debug.Assert((int)MethodSignatureFlags.CallingConventionVarargs == (int)SignatureCallingConvention.VarArgs);
	Debug.Assert((int)MethodSignatureFlags.UnmanagedCallingConvention == (int)SignatureCallingConvention.Unmanaged);

[davidwrighton]

I believe we should probably fix the explicit this problem, but I'd like to see such a fix done as part of fixing explicit this throughout our product, not just in this situation.

[trylek]

/azp run runtime-coreclr crossgen2

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).