[CppCodeGen] Assert during generic virtual method call compilation

[kbaladurin]

ilc fails to compile following sample with cpp codegen backend:

using System;

// Name of namespace matches the name of the assembly on purpose to
// ensure that we can handle this (mostly an issue for C++ code generation).
namespace Hello
{
    internal class Program
    {
        public class A
        {
            public virtual void M<T>(T t)
            {
                System.Console.WriteLine("Virtual A " + t);
            }
        }

        public class B : A
        {
            public override void M<T>(T t)
            {
                System.Console.WriteLine("Virtual B " + t);
            }
        }

        private static void Main(string[] args)
        {
            if (args.Length == 0)
            {
                Console.WriteLine("Usage: hello name");
                return;
            }

            Console.WriteLine("Hello " + args[0]);

            A obj = new B();
            obj.M(1);
        }
    }
}

$ bin/Linux.x64.Debug/tools/ilc --verbose @"Hello.ilc.rsp"
...
Compiling [Hello]Hello.Program.Main(string[])
Assertion Failed

   at ILCompiler.DependencyAnalysis.VirtualMethodUseNode..ctor(MethodDesc decl) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/VirtualMethodUseNode.cs:line 39
   at ILCompiler.DependencyAnalysis.NodeFactory.<CreateNodeCaches>b__36_21(MethodDesc method) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.cs:line 345
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at ILCompiler.DependencyAnalysis.NodeFactory.NodeCache`2.GetOrAdd(TKey key) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.cs:line 150
   at ILCompiler.DependencyAnalysis.NodeFactory.VirtualMethodUse(MethodDesc decl) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.cs:line 873
   at Internal.IL.ILImporter.ImportCall(ILOpcode opcode, Int32 token) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.CppCodeGen/src/CppCodeGen/ILToCppImporter.cs:line 1167
   at Internal.IL.ILImporter.ImportBasicBlock(BasicBlock basicBlock) in /media/kbaladurin/data/dotnet/forked/corert/src/Common/src/TypeSystem/IL/ILImporter.cs:line 604
   at Internal.IL.ILImporter.ImportBasicBlocks() in /media/kbaladurin/data/dotnet/forked/corert/src/Common/src/TypeSystem/IL/ILImporter.cs:line 327
   at Internal.IL.ILImporter.Compile(CppMethodCodeNode methodCodeNodeNeedingCode) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.CppCodeGen/src/CppCodeGen/ILToCppImporter.cs:line 651
   at ILCompiler.CppCodeGen.CppWriter.CompileMethod(CppMethodCodeNode methodCodeNodeNeedingCode) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.CppCodeGen/src/CppCodeGen/CppWriter.cs:line 453
   at ILCompiler.CppCodegenCompilation.ComputeDependencyNodeDependencies(List`1 obj) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.CppCodeGen/src/Compiler/CppCodegenCompilation.cs:line 56
   at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ComputeDependencies(List`1 deferredStaticDependencies) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.DependencyAnalysisFramework/src/DependencyAnalyzer.cs:line 139
   at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ComputeMarkedNodes() in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.DependencyAnalysisFramework/src/DependencyAnalyzer.cs:line 262
   at ILCompiler.CppCodegenCompilation.CompileInternal(String outputFile, ObjectDumper dumper) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.CppCodeGen/src/Compiler/CppCodegenCompilation.cs:line 45
   at ILCompiler.Compilation.ILCompiler.ICompilation.Compile(String outputFile, ObjectDumper dumper) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler.Compiler/src/Compiler/Compilation.cs:line 329
   at ILCompiler.Program.Run(String[] args) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler/src/Program.cs:line 500
   at ILCompiler.Program.Main(String[] args) in /media/kbaladurin/data/dotnet/forked/corert/src/ILCompiler/src/Program.cs:line 656
Aborted (core dumped)

Assert occurs here:

        public VirtualMethodUseNode(MethodDesc decl)
        {
            Debug.Assert(!decl.IsRuntimeDeterminedExactMethod);
            Debug.Assert(decl.IsVirtual);

            // Virtual method use always represents the slot defining method of the virtual.
            // Places that might see virtual methods being used through an override need to normalize
            // to the slot defining method.
            Debug.Assert(MetadataVirtualMethodAlgorithm.FindSlotDefiningMethodForVirtualMethod(decl) == decl);

            // Generic virtual methods are tracked by an orthogonal mechanism.
            Debug.Assert(!decl.HasInstantiation);

            _decl = decl;
        }

Generic virtual methods should be tracked using another mechanism. Is it implemented for cpp codegen?

Thank you!

[kbaladurin]

cc @alpencolt @BredPet

[kbaladurin]

@jkotas

[MichalStrehovsky]

Generic virtual methods are currently not implemented in CppCodegen. The implementation should follow what we do in RyuJIT's case (i.e. tracking it within the dependency analysis system using the GVMDependenciesNode). Besides that, we'll need to implement emission of several supporting data structures into the C++ code (such as GenericVirtualMethodsTableNode).

Note that the existing support for generic virtual methods within the class library assumes shared generic code (i.e. code sharing between generic instantiations over reference types) - this is also currently not implemented in CppCodegen, but is a prerequisite for this.

[kbaladurin]

@MichalStrehovsky thank you! What the status of the CppCodegen is? Will it be used in the future or is it legacy and will be replaced by RyuJIT codegen?

[MichalStrehovsky]

What the status of the CppCodegen is?

Jan's statement here still holds. CppCodegen is not legacy - it has a place, we want to keep it, and we do accept pull requests with fixes and new features. It's just not a priority for the scenarios we're targeting right now.

[kbaladurin]

I got it, thank you!

[kbaladurin]

@MichalStrehovsky should we use System.Runtime.TypeLoaderExports.GVMLookupForSlot for cppcodegen mode too? Seems like It also requires enabling reflection.

[MichalStrehovsky]

@MichalStrehovsky should we use System.Runtime.TypeLoaderExports.GVMLookupForSlot for cppcodegen mode too? Seems like It also requires enabling reflection.

Yes - that way we can share the infrastructure that was built for this. It doesn't require reflection, but it does require emitting several data blobs from the compiler that are similar to the data blobs used by reflection (e.g. the GenericVirtualMethodsTableNode mentioned above).

[kbaladurin]

Thank you!