Fixes around virtual method hierarchy validation

[vitek-karas]

Adds the test from linker and fixes to make it work.

[ghost]

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Adds the test from linker and fixes to make it work.

Author:	vitek-karas
Assignees:	vitek-karas
Labels:	area-NativeAOT-coreclr
Milestone:	-

[vitek-karas]

Looking for feedback - the test is still not fully passing.

This fixes the GVM bug - as per Michal's suggestion

Fixes how we detect virtual method overrides for data flow validation - I'm not sure about this one as I don't know if the code will work for GVM's or not.

[agocke]

Not sure I understand -- do static interface methods not have slots? And can't they enter the exact diamond problem that's presented below?

[MichalStrehovsky]

This matches what we do for non-default methods here:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeGVMEntriesNode.cs

Lines 106 to 107 in 7db5a57

	if (!method.HasInstantiation || method.Signature.IsStatic)
	continue;

This code is used to generate a data structure that can be used to resolve generic virtual methods at runtime. We'll stop filtering them here eventually to fix #77070 but a lot more needs to happen to fix that.

[MichalStrehovsky]

Fixes how we detect virtual method overrides for data flow validation - I'm not sure about this one as I don't know if the code will work for GVM's or not.

We should move that detection to where generic virtual methods are resolved. The reason why we do things this way is that virtual method resolution is really expensive (it used to be like 10% of compiler wallclock time) and we don't want to re-resolve too many times.

I think somewhere around here would be a good spot to call the NoteOverridingMethod callback:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs

Line 157 in 7db5a57

TypeDesc[] openInstantiation = new TypeDesc[_method.Instantiation.Length];

(and one more spot for non-interface methods in the same file).

[vitek-karas]

Re calling NoteOverridingMethod. There's one scenario which is not covered by the GVMDependenciesNode. Static abstract interface methods.

interface IFace
{
     static abstract void StaticGVM<[DAM(PublicMethod)] T>();
}

class FaceImpl : IFace
{
    // This should warn - since the annotation on T doesn't match
    public static void StaticGVM<T>() {};
}

For this there's no GVM nodes generated - I guess we rely on generic specialization and thus JIT will end up generating a direct call to the override. So there's no need to have any data structures for something like "GVM". But it also means that we need a place to still detect the method override for analysis purposes.

[vitek-karas]

Looking into this some more I think for static abstract/virtual GVMs we should handle this in the EETypeNode - but as a special case probably. Currently we only go over the logic if the type will have a vtable - which may not be always true for this case (I assume we only create VTables for instantiated types - but that doesn't seem like a requirement for static abstract methods to work).

That said the logic is to go over all interfaces implemented by the given type and if it has a static abstract interface method, find the implementing method on the current type. This all is already there, but it's intentionally skipped for GVMs. The part I don't know is if ResolveInterfaceMethodToStaticVirtualMethodOnType will work for GVMs. It seems to purely rely on method-impl tables, which should probably work OK for GVMs, but this is far outside of my understand of the virtual override resolution algorithm.

Basically the idea is - in EETypeNode handle everything which either has a vtable slot or doesn't need one as it's going to be called directly (that is the static abstract method case). The GVMDependencyNode only handles cases where a GVM actually needs a special data representation at runtime to facilitate GVM resolution - which is not needed for static abstract GVMs as they're called directly.

[MichalStrehovsky]

Looking into this some more I think for static abstract/virtual GVMs we should handle this in the EETypeNode - but as a special case probably. Currently we only go over the logic if the type will have a vtable - which may not be always true for this case (I assume we only create VTables for instantiated types - but that doesn't seem like a requirement for static abstract methods to work).

Can we do the statics when we do the codegen? I.e. add a callout to NoteOverridingMethod somewhere around where TryResolveConstraintMethodApprox is called in CorInfoImpl.RyuJit.cs and ILImporter.Scanner.cs? The idea is that NoteOverridingMethod should be called when we use the fact that something overrides something else.

This will make NoteOverridingMethod be newly called during multithreaded phase - should be fine, but something to note if you see odd behaviors.

[vitek-karas]

I think I'll need more help on this. Looking into the proposed solution there are 2 problems I ran into:

1. In the ILScanner the most likely case is that the constrained call is for a __Cannon in which case the resolution intentionally bails out (since it doesn't know the target type). So for this we need a different mechanism somewhere. In the ILScanner we don't do generic instantiations, so when the method/type which has the constrained call in it is instantiated, we will not rescan the method body with a known type substituted for the constrained type. The work needs to happen somewhere we decide to keep the implementation method - then we detect that it's an override of a static abstract/virtual and call the NoteOverridingMethod.
2. For data flow this is also broken because there's no callsite scanned by ILScanner - if the implementing method is "marked" due to some DAM annotation somewhere, the callsite is "reflection". So that would again lead to the case of doing this processing where we decide to keep the implementation method.

The JIT case is probably the only one where we might be able to rely on the constrained call site, the question is if it's worth doing it there and then special casing something else for the two cases above, or if it would make more sense to just do it the same way for all cases.

[MichalStrehovsky]

if it would make more sense to just do it the same way for all cases.

We don't have such spot in the compiler, unless we make it. It would be something along the lines of "go over all types in the assembly, resolve their virtuals, and look for mismatches". It would be nice and central, but it would also warn for things that are not used, and it would have us re-resolve everything one more time (including things that we wouldn't even look at otherwise). I like that it's centralized, but I don't particularly like the rest of what it implies.

So we're left with this being spread around all the places that resolve virtuals.

The last spot to cover scenario 1 in your comment is in ConstrainedMethodUseLookupResult::GetTarget.

I think scenario 2 (dataflow) will be covered by the existing check in EEType node for instance virtual methods. For static virtual methods, there's no way to reflection-call them "virtually". As such, I don't think it warrants a warning. (I know that illink might generate a warning for the static virtual method case, but if we're not using the method virtually, a mismatching annotation doesn't actually present a trim safety issue).

[vitek-karas]

Thanks - some more questions:

For example:

interface IFace
{
    void MyGVM<T>();
}

class Impl : IFace
{
   public void MyGVM<T>() { }
}

If there's reflection access to Impl.MyGVM we don't generate a GVM node for it, but we don't produce any dynamic dependencies because of code in HasDynamicDependencies which states that we don't need to do anything for final methods. I assume this is because if there's access to the implementation (non-virtual) method directly, there's no need to figure out anything "virtual" - so we just call the method and avoid the GVM stuff which goes with it.
If I add a reference to the interface method then we do go into the override resolution code.

Given the description above (that reflection to static virtual methods doesn't really make sense on the interface - I assume it's because reflection doesn't have API to express constrained call invocation) - should we remove this todo:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs

Lines 280 to 282 in 1a476d7

	// TODO: Reflection invoking static virtual methods
	if (method.IsVirtual && method.Signature.IsStatic)
	return false;

[vitek-karas]

@MichalStrehovsky could you please take another look - I think this is mostly working - at least I could not find a case where it didn't work.
Sometimes it's not reporting warnings which linker will, but it's around the cases where it's a direct access to the implementation method, so there's no need for the compiler to look for its base. (See my comment above for some questions around that).

I'm not 100% sure about the condition in the JIT helpers where it ran into __Canon types which can't be inspected by the data flow engine (it's a RuntimeGeneratedType and we don't want to look at those, we want the real stuff behind it) - so I tried to avoid those. I think it's OK since for those it should eventually go through generic dictionary creation and the overrides for actual instantiations should be used.

This is harder to test since linker based tests don't run JIT (only ILScanner), so I'm kind of relying on your expertise to tell if it looks correct.

The only thing missing I know of is to try to run the same test file in the linker and tweak the attributes and such so that we don't have resync it next time we sync linker to AOT.

[MichalStrehovsky]

Looks great modulo the two comments!

[MichalStrehovsky]

This part shouldn't be needed - delegate creation goes through getCallInfo below so we would catch it there.

We need to re-resolve here for... reasons... but we already resolved in getCallInfo like the comment on line 427 says below.

(The reason is that we shall never give RyuJIT a method in runtime determined form because RyuJIT only understands canonical form.)

[MichalStrehovsky]

The condition in the if is always true. I'd get rid of it. I think you wanted to filter out canonical forms, but I don't see them filtered out in the scanner either and it should be okay that way. We could potentially change the implementation of NoteOverridingMethod to grab the typical method definitions of things and report that (that way it really doesn't matter what we report).

The difference between canonical forms and runtime determined forms is this (this took me a while to really understand this so maybe I won't be able to explain but I'll still try):

• Canonical form of a method (in principle) is a form where every reference type in the instantiation got replaced with __Canon. There can be multiple canonical forms for a single method definition (for example, Foo<__Canon>, Foo<SomeGenericValueType<__Canon>>). But each canonical form has its own physical method body with processor instructions.
• A runtime determined form has a close relationship to canonical form. It's a canonical method with extra information about what should be in the __Canon.

For example:

static void Foo<T>() { }
static void Bar<T, U>()
{
    Foo<T>();
    Foo<U>();
}

If we're compiling the method body of Bar<__Canon, __Canon>() (canonical form) we need to somehow represent the destination of each of the call. This is where runtime detemined forms come into play: the first call calls Foo<T___Canon>, the second calls Foo<U___Canon> (the runtime determined form). These forms basically capture the physical method body, with extra information about where to grab the generic context from. As far as RyuJIT is concerned, it's calling Foo<__Canon> with some magic cookie (generic context) we made from somewhere. But the runtime determined form lets us figure out what the magic cookie is.