Put embedGenericHandle in sync with constraint resolution (hacky)

[trylek]

As a proof of concept I am presenting the hotfix propagating
constraint resolution from getCallInfo to embedGenericHandle
by means of patching the pResolvedToken. It looks like a very
dirty thing to do but it fixes all crashes in the GenericContext
tests:

609 successes reported
891 failures reported
Expected: 100
Actual: 1
END EXECUTION - FAILED
FAILED

Thanks

Tomas

[trylek]

Attaching my findings from the investigation for reference (originally sent offline in an e-mail to David):

Hi David!

This is the summary of my current findings regarding two runtime crashes in GenericContext tests I have recently investigated. It would be great if you could share your thoughts as to how to continue the investigation and / or what is the right way of fixing this. In general, both failures seem to indicate that the embedGenericHandle contract is insufficient in presence of constrained type resolution. I am not yet fully clear on why that doesn’t cause issues even in the absence of SVMs; my current working theory is that for instance virtual methods the presence of the this pointer containing the pointer to the correct method table probably makes these various lookups more robust.

Thanks

Tomas

Test_Call_GenericOverReferenceType_ClassAGenericClass_NonGeneric_NonGeneric_GenericMethodOverInt

This is the test you helped me investigate on Monday. The problem is that the constrained resolution in getCallInfo works fine but is subsequently not honored in embedGenericHandle – this method only receives the primary pResolvedToken without the constraint resolution and we’re passing neither the constrained token nor the result from getCallInfo to the method so that it creates the generic dictionary corresponding to the unconstrained interface type, not the actual type calculated via the constraint resolution. As I mentioned yesterday, just as a proof of concept I verified locally that I can fix this by patching the primary pResolvedToken in getCallInfo but I suspect it’s not the right solution. It seems to me that the right solution would be to either change the JIT API by passing either the pConstrainedResolvedToken or the pResult of getCallInfo to embedGenericHandle so that it can work with the correct type & method after constraint resolution (passing pResult would be better than passing pConstrainedResolvedToken because that would basically require us to duplicate the constraint resolution logic between getCallInfo and embedGenericHandle); alternatively we might be able to somehow pre-calculate the data for embedGenericHandle directly in getCallInfo but that would also require changing the JIT contract.

Test_Call_GenericOverReferenceType_ClassAGenericClass_GenericOverString_NonGeneric_GenericMethodOverString<string>

Here the primary crash happens during population of the generic dictionary entry but I think that the underlying problem is more or less the same as in the first case:

  .method /*06000445*/ public static void 
          Test_Call_GenericOverReferenceType_ClassAGenericClass_GenericOverString_NonGeneric_GenericMethodOverString<T>() cil managed noinlining
  {
    // 
    .maxstack  8
    IL_0000:  constrained. class [GenericContextCommonAndImplementation/*23000005*/]GenericClass`1/*0100000A*/<object>/*1B000191*/
    IL_0006:  call       void class [GenericContextCommonAndImplementation/*23000005*/]IFaceNonGeneric/*01000003*//*1B000002*/::GenericMethod<string>() /* 2B000002 */

When we resolve the mdtMethodSpec token 0x2B000002 in resolveToken, the call to GetDescFromMemberRef from GetMethodDescFromMethodSpec ends up populating pResolvedToken->pTypeSpec / pResolvedToken->cbTypeSpec with the signature representing the type IFaceNonGeneric. When we subsequently call embedGenericHandle, the code at

runtime/src/coreclr/vm/jitinterface.cpp

Line 3347 in 36dce21

// Encode containing type

            // Encode containing type
            if (pResolvedToken->pTypeSpec != NULL)
            {
                SigPointer sigptr(pResolvedToken->pTypeSpec, pResolvedToken->cbTypeSpec);
                sigptr.ConvertToInternalExactlyOne(pModule, NULL, &sigBuilder);
            }
            else
            {
                sigBuilder.AppendElementType(ELEMENT_TYPE_INTERNAL);
                sigBuilder.AppendPointer(pResolvedToken->hClass);
            }

encodes the incorrect type IFaceNonGeneric, not the correct type GenericClass`1<object> (after the constraint resolution), to the slot, so that we subsequently fail in Dictionary::PopulateEntry because FindOrCreateAssociatedMethodDesc fails to resolve pDefMD = “IFaceNonGeneric.GenericMethod” on pExactMT = “IFaceNonGeneric”.

[trylek]

After rebasing to the latest state of your branch, the results are even better:

928 successes reported
572 failures reported

[trylek]

1272 successes reported
264 failures reported

[trylek]

David has figured out a cleaner way of fixing this, closing.