VirtualInvokeMap Table by fadimounir · Pull Request #3100 · dotnet/corert

fadimounir · 2017-03-24T21:25:23Z

Implementation of the VirtualInvokeMap table to enable Reflection invocation to virtual and generic virtual methods.

Implementation supports both CoreRT and ProjectX models.
Fixed interface EEType generation to unconditionally produce a dictionary slot for generic interfaces (to support static interface methods, and enable correct Reflection virtual invoke slot computation on ProjectX).

fadimounir · 2017-03-24T21:26:00Z

@davidwrighton @MichalStrehovsky PTAL.

MichalStrehovsky · 2017-03-24T21:40:35Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ReflectionVirtualInvokeMapNode.cs

+    /// </summary>
+    internal sealed class ReflectionVirtualInvokeMapNode : ObjectNode, ISymbolNode
+    {
+        private const uint GenericVirtualMethod = 1;


Would be nice to make this a constant we can share with the framework code. I've started dumping these to Common\src\Internal\Runtime\RuntimeConstants.cs.

It makes it really easy to findstr all the places that use this. #Resolved

Reusing VirtualInvokeTableEntry.GenericVirtualMethod from MappingTableFlags.cs instead.

In reply to: 108004794 [](ancestors = 108004794)

MichalStrehovsky · 2017-03-24T21:42:43Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/EETypeNode.cs

            {
                // Note: Canonical type instantiations always have a generic dictionary vtable slot, but it's empty
-                if (declType.IsCanonicalSubtype(CanonicalFormKind.Any))
+                if (declType.IsInterface || declType.IsCanonicalSubtype(CanonicalFormKind.Any))


Should this actually go down the path that gets the dictionary instead? If it doesn't work right now for whatever reason, there should at least be a comment. #Resolved

I was looking at what the binder did, and it was not filling the dictionary slot with anything useful.
I'll investigate this further to see if we ever fill the dictionary slot.
@davidwrighton, do you know if we ever fill the slot with a dictionary pointer? If so, under what circumstances? #Resolved

I would assume this actually being used if there's a static nongeneric method on a generic interface. I wouldn't be surprised if we have no test coverage for that and it simply AV's (because the dictionary is null) in Project N. #Resolved

I'll put a TODO comment to generate the right dictionary entry for static methods. #Resolved

MichalStrehovsky · 2017-03-24T21:45:04Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/InterfaceDispatchMapNode.cs

                    {
                        builder.EmitShort(checked((short)interfaceIndex));
-                        builder.EmitShort(checked((short)interfaceMethodSlot));
+                        builder.EmitShort(checked((short)(interfaceMethodSlot + (interfaceType.HasInstantiation ? 1 : 0))));


Maybe make it interfaceType.HasGenericDictionarySlot() for clarity? #Resolved

Ah yes... I wanted to change it to HasGenericDictionarySlot too but forgot. #Resolved

MichalStrehovsky · 2017-03-24T21:50:55Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ReflectionVirtualInvokeMapNode.cs

+                else
+                    containingTypeKey = method.OwningType.ConvertToCanonForm(CanonicalFormKind.Specific);
+
+                if (!methodsEmitted.ContainsKey(mappingEntry.MetadataHandle))


Nit: could you TryGetValue to avoid two lookups. #Resolved

fixed

In reply to: 108006180 [](ancestors = 108006180)

MichalStrehovsky · 2017-03-24T21:52:27Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ReflectionVirtualInvokeMapNode.cs

+
+                    if (slot == -1)
+                    {
+                        // This method doesn't have a slot. (At this time, this is only done for the Object.Finalize method)


Should we assert this? #Resolved

MichalStrehovsky · 2017-03-24T22:02:13Z

src/ILCompiler.Compiler/src/Compiler/VirtualMethodCallHelper.cs

                // For types that have a generic dictionary, the introduced virtual method slots are
                // prefixed with a pointer to the generic dictionary.
-                if (baseType.HasGenericDictionarySlot())
+                if (baseType.HasGenericDictionarySlot() && !doNotCountDictionarySlots)


Can you make this countDictionarySlots? Double negation is kind of hard to parse for humans. #Resolved

Sure :)

In reply to: 108007772 [](ancestors = 108007772)

MichalStrehovsky · 2017-03-24T22:05:07Z

src/ILCompiler.Compiler/src/Compiler/VirtualMethodCallHelper.cs

        {
-            return !type.IsInterface && type.HasInstantiation &&
+            // Dictionary slots on generic interfaces are necessary to support static methods on interfaces
+            if (type.IsInterface)


Just remove the type.IsInterface from the return below. Otherwise you're forcing dictionary slots for interfaces even if we're not compiling shared code, or when interface methods wouldn't need a dictionary anyway (e.g. IEnumerable<int>). #WontFix

The dictionary slot is unconditionally created on generic interfaces, regardless of whether they share code or not.
See code/comment in TypeLoaderEnvironment.Metadata.cs:
if (RuntimeAugments.IsGenericType(searchForSharedGenericTypesInParentHierarchy)) { if (RuntimeAugments.IsInterface(searchForSharedGenericTypesInParentHierarchy)) { // Generic interfaces always have a dictionary slot in the vtable (see binder code in MdilModule::SaveMethodTable) // Interfaces do not have base types, so we can just break out of the loop here ... slot++; break; } #Closed

I chatted with @davidwrighton about this. The historical reason behind making this unconditional is simplicity, and keeping method slot indices for methods on IFoo and IFoo identical. That won't change. #Closed

The historical reason behind making this unconditional is simplicity, and keeping method slot indices for methods on IFoo and IFoo identical. That won't change.

Can you add that to the comment? Sounds like something that would be nice to capture. #Resolved

ok #Resolved

davidwrighton

I'm a little disappointed in the multiple ABIs logic in the runtime, but I can live with it. (It might be nice to convert to supporting both formats in the runtime) In any case, if you change the format of ReflectionMapBlob.VirtualInvokeMap, you must update all consumers of that format.

davidwrighton · 2017-03-27T18:57:24Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ConstructedEETypeNode.cs

-                if (!factory.NecessaryTypeSymbol(templateType).RepresentsIndirectionCell)
-                    dependencyList.Add(factory.NativeLayout.TemplateTypeLayout(templateType), "Template Type Layout");
+                if (templateType.IsCanonicalSubtype(CanonicalFormKind.Any))
+                    if (!factory.NecessaryTypeSymbol(templateType).RepresentsIndirectionCell)


Please wrap the inner if statement in {} brackets. Nested ifs without {} are rife for accidental screwup. #Resolved

davidwrighton · 2017-03-27T19:11:41Z

src/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeLoaderEnvironment.Metadata.cs

@@ -823,11 +823,38 @@ public static bool TryGetVirtualResolveData(NativeFormatModuleInfo module,
            ExternalReferencesTable externalReferences = default(ExternalReferencesTable);
            externalReferences.InitializeCommonFixupsTable(module);



A corresponding change to TryGetMethodNameAndSigFromVirtualResolveData is missing from this change.

Thanks for catching this.

Fixed for the declaring type semantics. No changes done to the slot index computation as we discussed, since this code is only used with SUPPORTS_NATIVE_METADATA_TYPE_LOADING

…ocation to virtual and generic virtual methods. Implementation supports both CoreRT and ProjectX models. Fixed interface EEType generation to unconditionally produce a dictionary slot for generic interfaces (to support static interface methods, and enable correct Reflection virtual invoke slot computation on ProjectX).

Generic interface types have a generic dictionary slot like any other type, except sometimes the slot is null. We can now call static methods on generic interfaces with a usable generic context. Got reminded of this while reading about the planned C# language support for static methods on interfaces. I didn't want to push on it when dotnet#3100 added the logic, but it turns out this is really easy.

Generic interface types have a generic dictionary slot like any other type, except sometimes the slot is null. We can now call static methods on generic interfaces with a usable generic context. Got reminded of this while reading about the planned C# language support for static methods on interfaces. I didn't want to push on it when #3100 added the logic, but it turns out this is really easy.

dnfclas added the cla-already-signed label Mar 24, 2017

fadimounir requested review from MichalStrehovsky and davidwrighton March 24, 2017 21:25

MichalStrehovsky reviewed Mar 24, 2017

View reviewed changes

MichalStrehovsky mentioned this pull request Mar 25, 2017

Make invoking abstract methods work #2887

Closed

MichalStrehovsky approved these changes Mar 27, 2017

View reviewed changes

davidwrighton suggested changes Mar 27, 2017

View reviewed changes

fadimounir force-pushed the VirtualInvokeMap branch from 7345fac to 301e7c6 Compare March 27, 2017 20:51

fadimounir merged commit f2d7716 into dotnet:master Mar 27, 2017

MichalStrehovsky mentioned this pull request Mar 28, 2017

Make it possible to use RD.XML for interface methods #3125

Merged

fadimounir deleted the VirtualInvokeMap branch April 6, 2017 17:00

MichalStrehovsky mentioned this pull request May 19, 2017

Emit interface generic dictionaries #3658

Merged

		@@ -823,11 +823,38 @@ public static bool TryGetVirtualResolveData(NativeFormatModuleInfo module,
		ExternalReferencesTable externalReferences = default(ExternalReferencesTable);
		externalReferences.InitializeCommonFixupsTable(module);

Conversation

fadimounir commented Mar 24, 2017

Uh oh!

fadimounir commented Mar 24, 2017

Uh oh!

MichalStrehovsky Mar 24, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fadimounir Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Mar 24, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fadimounir Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Mar 24, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fadimounir Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Mar 24, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fadimounir Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Mar 24, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fadimounir Mar 24, 2017

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Mar 24, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fadimounir Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Mar 24, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fadimounir Mar 24, 2017

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Mar 24, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fadimounir Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fadimounir Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Mar 25, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fadimounir Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwrighton left a comment

Choose a reason for hiding this comment

MichalStrehovsky Mar 24, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Mar 24, 2017 •

edited by fadimounir

Loading

fadimounir Mar 24, 2017 •

edited

Loading

MichalStrehovsky Mar 24, 2017 •

edited by fadimounir

Loading

fadimounir Mar 25, 2017 •

edited

Loading

MichalStrehovsky Mar 24, 2017 •

edited by fadimounir

Loading

fadimounir Mar 24, 2017 •

edited

Loading

MichalStrehovsky Mar 24, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Mar 24, 2017 •

edited by fadimounir

Loading

fadimounir Mar 24, 2017 •

edited

Loading

MichalStrehovsky Mar 24, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Mar 24, 2017 •

edited by fadimounir

Loading

fadimounir Mar 24, 2017 •

edited

Loading

fadimounir Mar 25, 2017 •

edited

Loading

MichalStrehovsky Mar 25, 2017 •

edited by fadimounir

Loading

fadimounir Mar 27, 2017 •

edited

Loading

davidwrighton Mar 27, 2017 •

edited by fadimounir

Loading