Make sure we tell the same lie to the JIT for default interface methods

[cshung]

This pull request is intended to fix the crossgen2 compilation crash of constrained2.ilproj.

The compilation failure was found by @janvorli by running all the CoreCLR Pri 1 test on Linux under crossgen2 using SuperIlc.

Luckily, the failure reproduces itself on Windows when building a non-optimized build. The compilation crashed with an assertion here:

runtime/src/coreclr/src/jit/importer.cpp

Line 8106 in f1890a7

assert(call->AsCall()->gtCallType == CT_USER_FUNC);

My first attempt was to comment out the assertion and see what happened, the test case passed. That mislead me to think the assertion could be wrong. Indeed, when I read the code around that function, I have no idea why the JIT can assert that fact, it looks like it should be handling whatever input that the getCallInfo() produces.

Turn out, that was wrong. The assert was meant for detecting wrong data from getCallInfo(). The first few comments below were reacting to my wrong attempt to remove the assertion, and they are obsolete by now.

A comparative study with crossgen revealed that the real root cause for the assert is that the VM was lying to the JIT in crossgen but crossgen2 wasn't lying the same way. In particular, we have this gem:

runtime/src/coreclr/src/vm/jitinterface.cpp

Lines 8793 to 8796 in f1890a7

	// If we are making a virtual call to an instance method on an interface, we need to lie to the JIT.
	// The reason being that we already made sure target is always directly callable (through instantiation stubs),
	// JIT should not generate shared generics aware call code and insert the secret argument again at the callsite.
	// Otherwise we would end up with two secret generic dictionary arguments (since the stub also provides one).

Because of the above, the CORINFO_CALLCONV_PARAMTYPE was not specified when crossgen compile the same test case, no wonder the assert is not a problem from crossgen.

It would be nice if we can come up with a little comment in the JIT about the handling of default interface method, the lie is just far from obvious.

@dotnet/jit-contrib
@dotnet/crossgen-contrib

[jakobbotsch]

it could be reached by having an interface dispatch that requires a runtime lookup (i.e. line 7640 of importer.cpp)

Does this line not just deal with the stub itself requiring runtime lookup? Is that not orthogonal to the generic context?

This issue is found by running constaintedcall2 test case on Linux under crossgen2. The test passed after removig the assert.

I wasn't able to find this test, can you link it? Does this test use the generic context for anything?

[MichalStrehovsky]

I wasn't able to find this test, can you link it?

Is it this one? https://github.com/dotnet/runtime/blob/master/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/constrainedcall/constrained2.il

Note that's a default interface method test. Crossgen2 lacks a bunch of DIM fixes still. Why is the assert not a problem for crossgen? Is this just crossgen2 sending bad input to RyuJIT because of missing DIM handling?

[jakobbotsch]

Is this just crossgen2 sending bad input to RyuJIT because of missing DIM handling?

I know you are asking @cshung, but this is what I would suspect. I think the following is true but please correct me if I am wrong:

• Generic interface methods are always called through instantiating stub so no generic context is required for those.
• Non-generic interface methods go through VSD, even if the interface itself is generic. Because classes can implement multiple interfaces of different type parameters we need runtime lookup for the VSD stub to use in certain cases, for example:

class C : IEnumerable<string>, IEnumerable<object>
{
  ...
}

...
static void Foo<T>(C c) => ((IEnumerable<T>)c).GetEnumerator();

This is what the code you linked to is handling I believe. However, these never need generic context I believe.

• Nevertheless, for non-generic default interface method on a generic interface a generic context is still needed. However these can be done as direct calls, so this does not make the assert wrong.

[fadimounir]

Why are we just hitting this assert in crossgen2, and not crossgen1, even though they use the same JIT?

[davidwrighton]

Looks ok