[Runtime Async] Add support for shared generics

[jakobbotsch]

In the end this wasn't that hard (easier than it would have been a year ago -- the IL emitter has had support for generic signatures added in the meantime). Maybe there are some missing cases. I've also left some TODOs around things I am not fully clear on.

cc @agocke @davidwrighton @VSadov

[jakobbotsch]

Any idea about this? I can imagine GC reporting might not be happy about seeing the null generic context in case we suspend before its value has been restored from the continuation inside the target method.

[jakobbotsch]

I think there is also a GC hole with collectible assemblies we'll have to solve in some way: the continuation does not necessarily have anything keeping the loader allocator for the generic context alive. It seems like for shared generics we'll have to add the loader allocator's rooting object for the generic context arg into the array of GC objects in the continuation. Even for unshared code we have to do this when emitting code into a collectible assembly.

This seems like it will probably require a helper call in the suspension case to get the loader allocator from the generic context/target method. The other alternative is that GC scanning could iterate all continuations and report their target methods/generic contexts, but that does not seem like a good solution (we really do not want to have to iterate all continuations outside the standard optimized GC paths).

[VSadov]

Any idea about this? I can imagine GC reporting might not be happy about seeing the null generic context in case we suspend before its value has been restored from the continuation inside the target method.

I think having null temporarily should be ok. As long as continuation is still reachable, which it should be until we restore.
It may hit some asserts and they may need to be tweaked. Like in

                    // Handle the case where the method is a static shared generic method and we need to keep the type
                    // of the generic parameters alive
                    if (paramContextType == GENERIC_PARAM_CONTEXT_METHODDESC)
                    {
                        MethodDesc *pMDReal = dac_cast<PTR_MethodDesc>(pCF->GetParamTypeArg());
                        _ASSERTE((pMDReal != NULL) || !pCF->IsFrameless());           <===
                        if (pMDReal != NULL)
                        {
                            GcReportLoaderAllocator(gcctx->f, gcctx->sc, pMDReal->GetLoaderAllocator());
                        }
                    }
                    else if (paramContextType == GENERIC_PARAM_CONTEXT_METHODTABLE)
                    {
                        MethodTable *pMTReal = dac_cast<PTR_MethodTable>(pCF->GetParamTypeArg());
                        _ASSERTE((pMTReal != NULL) || !pCF->IsFrameless());           <===
                        if (pMTReal != NULL)
                        {
                            GcReportLoaderAllocator(gcctx->f, gcctx->sc, pMTReal->GetLoaderAllocator());
                        }
                    }

[VSadov]

The other alternative is that GC scanning could iterate all continuations and report their target methods/generic contexts, but that does not seem like a good solution (we really do not want to have to iterate all continuations outside the standard optimized GC paths).

Right. It would be very expensive in Gen0 collections. Gen0 must be able to scan only a small (young) subset of continuations.

Adding the exposed object to the continuation seems the right way. Even if we do not use it for restoring anything - just as a keep-alive reference.

[jakobbotsch]

It may hit some asserts and they may need to be tweaked

I think I will switch this over to load the generic context in the caller once we have the JIT reporting back the continuation layout to the EE. For now we can probably keep it like this.

I'm going to look into addressing the GC holes in a follow-up.

[davidwrighton]

Allowing the context arg to be NULL is not a big deal as long as we are safely preserving the exposed LoaderAllocator object somewhere. We would have to remove those asserts, but as you can see the code already handles the param being marked as NULL. In fact, if we can guarantee that the LoaderAllocator exposed object is kept live, the runtime has no need to require that the context as reported by the CrawlFrame ever be set. (Please don't do this, as it reduces the quality of the call stack, but a temporary time as NULL is not a meaningful problem).

[VSadov]

In fact, if we can guarantee that the LoaderAllocator exposed object is kept live, the runtime has no need to require that the context as reported by the CrawlFrame ever be set.

I think the regular stackwalk needs to report the Loader from the context, as there may be no other reference to the exposed object. It seems hard to prove that for a live frame with unloadable context there is always something else that keeps the loader alive.

The suspended frames are not stackwalked though, thus we need to keep the exposed object reachable as long as continuation is reachable. Stashing a reference to the exposed object in the continuation should be sufficient, I think.

[davidwrighton]

Right, that was what I was thinking about. We need to hold onto the continuation until the context is set in the method frame (and the continuation needs to hold the exposed object), but otherwise, we don't need the thing to be non-null until general code execution begins.

[jakobbotsch]

If that sounds ok to you both then I will just keep it this way -- I've updated the assert in GC about the non-null generic context to allow async2 methods, and removed the TODO comment here.

[jakobbotsch]

This is missing some tests for the non-void case, which needs a bunch more generic signatures (for locals and calls to functions like Task.FromResult<T> and RuntimeHelpers.FinalizeTaskReturningThunk<T>). So not quite there yet.

[jakobbotsch]

Shared generic async2 methods with return values work now. What remains is the async2 -> async1 thunks synthesized when awaiting an async1 method from an async2 method. However, I have noticed that the current Roslyn prototype does not seem to generate calls to those at all. In fact, adding an always-failing assert to MethodDesc::EmitAsync2MethodThunk does not cause any of our async 2 tests to fail, so it seems like we have no testing of those thunks currently.

[jakobbotsch]

Seems like the support has also broken some old tests, e.g. Async2Object no longer works. That test calls a Foo<T> instantiated over object, and this PR changes things so that we try to encode some tokens as a MethodSpec instead of what we were doing before (previously we encoded it by using the instantiating stub). The MethodSpec encoding seems more faithful to IL, so there are probably some missing bits in the resolveToken path for dynamic resolvers.

[VSadov]

However, I have noticed that the current Roslyn prototype does not seem to generate calls to those at all. In fact, adding an always-failing assert to MethodDesc::EmitAsync2MethodThunk does not cause any of our async 2 tests to fail, so it seems like we have no testing of those thunks currently.

I think that came from the early misunderstanding that async1 awaits async2 via a thunk (because there is no other way), but async2 awaits async1 in a regular way (which works, but would not be binary-compatible if the callee is changed to be async2)

I will look into fixing this in the Roslyn prototype.

[jakobbotsch]

This passes all async tests now. Can you take a look @VSadov (and @davidwrighton, potentially)?

[jakobbotsch]

(This assert is copied from MethodDesc::GetSigFromMetadata)

[VSadov]

Thanks!!