Assert failure `extraParamArgLocation == INT_MAX` in interpreter for async covariant return tests

[jakobbotsch]

Example test run: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1495258
Example console log: https://helixr1107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-heads-main-e6056bcae0584738bd/async/1/console.f4560e10.log?helixlogtype=result

14:46:06.361 Running test: _covariant_return_covariant_returns::GenericVirtualMethod.Program.TestGenericVirtualMethod()

Assert failure(PID 2116 [0x00000844], Thread: 2156 [0x086c]): extraParamArgLocation == INT_MAX
    File: D:\a\_work\1\s\src\coreclr\interpreter\compiler.cpp:5641
    Image: C:\h\w\B5F40A08\p\corerun.exe

Reproduces consistently under DOTNET_InterpMode=3 when running the async tests.

Looks caused by #129442, cc @VSadov. It looks like the VM is returning CORINFO_VIRTUALCALL_LDVIRTFTN but also that the call has a generic type argument, which is an unexpected combination for the interpreter.

[dotnet-policy-service]

Tagging subscribers to this area: @vitek-karas, @BrzVlad, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

[BrzVlad]

I was just looking into this and thinking about what the best fix would be. So the problem is that, getCallInfo returns a sig that hasTypeArg, but CORINFO_VIRTUALCALL_LDVIRTFTN calls never require to pass the arg, since the target does all the instantiating. JIT doesn't check for hasTypeArg in its path for CORINFO_VIRTUALCALL_LDVIRTFTN so it doesn't get tripped by it. So the least invasive fix would be to make the interpreter do the same. I believe a more rigorous approach would be to make getCallInfo not have hasTypeArg set for these types of calls in the first place.

Looking even deeper than this, normal virtual calls don't get tripped up by this because token resolving ends up calling MethodDesc::FindOrCreateAssociatedMethodDesc with allowInstParam false. This ends up having as the target of a virtual call an InstantiatedMethodDesc with the instantiation being Canon, and the call info gets marked as not requiring a type arg because the method desc is flagged as carrying the insantiation. It seems like this could also be fixed by tweaking the token in EmitReturnDroppingThunk to point similarly to a Canon instantiated method, but I didn't look much into this approach and I'm not familiar enough with the area to tell whether this would be a better solution

[jakobbotsch]

Looking even deeper than this, normal virtual calls don't get tripped up by this because token resolving ends up calling MethodDesc::FindOrCreateAssociatedMethodDesc with allowInstParam false. This ends up having as the target of a virtual call an InstantiatedMethodDesc with the instantiation being Canon, and the call info gets marked as not requiring a type arg because the method desc is flagged as carrying the insantiation. It seems like this could also be fixed by tweaking the token in EmitReturnDroppingThunk to point similarly to a Canon instantiated method, but I didn't look much into this approach and I'm not familiar enough with the area to tell whether this would be a better solution

We should try as much as possible to match what happens with normal user IL. The call we're trying to encode in EmitReturnDroppingThunk is similar to the Bar<T>() call in

class C
{
  virtual void Foo<T>()
  {
    Bar<T>();
  }

  virtual string Bar<T>()
  {
  }
}

So if that's represented internally with a call to Bar<__Canon> with allowInstParam=false, then that's probably the right way to do that here as well. Other places in asyncthunks.cpp also call FindOrCreateAssociatedMethodDesc with allowInstParam=false before they synthesize tokens.