From 82a6fe02e3772e6f09097a221fbb5756b622318e Mon Sep 17 00:00:00 2001 From: Rokas Kupstys Date: Thu, 6 Sep 2018 14:21:54 +0300 Subject: [PATCH 1/2] Improve correctness of SwigDerivedClassHasMethod() by making sure only methods that have `override` are used connected by director. C# does not treat `virtual` methods of child classes as overriding (ass opposed to c++). In order to override a method it must have `override` specified. Previous version of this method treated `virtual void foo()` or `void foo()` in a subclass as methods that override virtual method of parent class. This resulted in `SwigDirectorConnect()` creating delegates and connecting them to a native class. Presence of such methods caused needless roundtrip through managed layer while in the end correct native function was called. This is the old code flow: ```cpp void SwigDirector_MyClass::nonOverride() { if (!swig_callbacknonOverride) { // 0. swig_callbacknonOverride is set MyClass::nonOverride(); return; } else { swig_callbacknonOverride(); // 1. this is called because wrapper mistakenly assumes overriding } } SWIGEXPORT void SWIGSTDCALL CSharp_director_basicNamespace_MyClass_nonOverrideSwigExplicitMyClass(void * jarg1) { MyClass *arg1 = (MyClass *) 0 ; arg1 = (MyClass *)jarg1; (arg1)->MyClass::nonOverride(); // 5. Correct method called in the end } ``` ```cs private void SwigDirectornonOverride() { nonOverride(); // 2. } public virtual void nonOverride() { if (SwigDerivedClassHasMethod("nonOverride", swigMethodTypes4)) // 3. This returns `true` director_basicPINVOKE.MyClass_nonOverrideSwigExplicitMyClass(swigCPtr); // 4. Native method of director class called explicitly else director_basicPINVOKE.MyClass_nonOverride(swigCPtr); } ``` This is the new code flow: ```cpp void SwigDirector_MyClass::nonOverride() { if (!swig_callbacknonOverride) { // 0. swig_callbacknonOverride is not set MyClass::nonOverride(); // 1. Calls correct method immediately return; } else { swig_callbacknonOverride(); } } ``` --- Source/Modules/csharp.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Modules/csharp.cxx b/Source/Modules/csharp.cxx index bd00ffaf3..75ee34023 100644 --- a/Source/Modules/csharp.cxx +++ b/Source/Modules/csharp.cxx @@ -1953,7 +1953,7 @@ public: Printf(proxy_class_code, " private bool SwigDerivedClassHasMethod(string methodName, global::System.Type[] methodTypes) {\n"); Printf(proxy_class_code, " global::System.Reflection.MethodInfo methodInfo = this.GetType().GetMethod(methodName, global::System.Reflection.BindingFlags.Public | global::System.Reflection.BindingFlags.NonPublic | global::System.Reflection.BindingFlags.Instance, null, methodTypes, null);\n"); - Printf(proxy_class_code, " bool hasDerivedMethod = methodInfo.DeclaringType.IsSubclassOf(typeof(%s));\n", proxy_class_name); + Printf(proxy_class_code, " bool hasDerivedMethod = methodInfo.IsVirtual && methodInfo.DeclaringType.IsSubclassOf(typeof(%s)) && methodInfo.DeclaringType != methodInfo.GetBaseDefinition().DeclaringType;\n", proxy_class_name); /* Could add this code to cover corner case where the GetMethod() returns a method which allows type * promotion, eg it will return foo(double), if looking for foo(int). if (hasDerivedMethod) { From 15f9403e06989cd1f6f592bbe52c1a735f94372b Mon Sep 17 00:00:00 2001 From: Rokas Kupstys Date: Wed, 26 Sep 2018 12:20:38 +0300 Subject: [PATCH 2/2] Fix expanded director_basic test. Since we are mixing `new` and `override` a class instance may have multiple methods with the same name. in case of director_basic test one method comes from `MyClassMiddle` (one that participates in dynamic dispatch) and one comes from `MyClassEnd` (one with `new` keyword). Therefore all methods have to be checked. --- Source/Modules/csharp.cxx | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/Source/Modules/csharp.cxx b/Source/Modules/csharp.cxx index 75ee34023..2fa3eff25 100644 --- a/Source/Modules/csharp.cxx +++ b/Source/Modules/csharp.cxx @@ -1951,9 +1951,32 @@ public: // Only emit if there is at least one director method Printf(proxy_class_code, "\n"); Printf(proxy_class_code, " private bool SwigDerivedClassHasMethod(string methodName, global::System.Type[] methodTypes) {\n"); - Printf(proxy_class_code, - " global::System.Reflection.MethodInfo methodInfo = this.GetType().GetMethod(methodName, global::System.Reflection.BindingFlags.Public | global::System.Reflection.BindingFlags.NonPublic | global::System.Reflection.BindingFlags.Instance, null, methodTypes, null);\n"); - Printf(proxy_class_code, " bool hasDerivedMethod = methodInfo.IsVirtual && methodInfo.DeclaringType.IsSubclassOf(typeof(%s)) && methodInfo.DeclaringType != methodInfo.GetBaseDefinition().DeclaringType;\n", proxy_class_name); + Printf(proxy_class_code, " global::System.Reflection.MethodInfo[] methodInfos = this.GetType().GetMethods(\n"); + Printf(proxy_class_code, " global::System.Reflection.BindingFlags.Public | global::System.Reflection.BindingFlags.NonPublic | global::System.Reflection.BindingFlags.Instance);\n"); + Printf(proxy_class_code, " foreach (global::System.Reflection.MethodInfo methodInfo in methodInfos) {\n"); + Printf(proxy_class_code, " if (methodInfo.DeclaringType == null)\n"); + Printf(proxy_class_code, " continue;\n\n"); + Printf(proxy_class_code, " if (methodInfo.Name != methodName)\n"); + Printf(proxy_class_code, " continue;\n\n"); + Printf(proxy_class_code, " var parameters = methodInfo.GetParameters();\n"); + Printf(proxy_class_code, " if (parameters.Length != methodTypes.Length)\n"); + Printf(proxy_class_code, " continue;\n\n"); + Printf(proxy_class_code, " bool parametersMatch = true;\n"); + Printf(proxy_class_code, " for (var i = 0; i < parameters.Length; i++) {\n"); + Printf(proxy_class_code, " if (parameters[i].ParameterType != methodTypes[i]) {\n"); + Printf(proxy_class_code, " parametersMatch = false;\n"); + Printf(proxy_class_code, " break;\n"); + Printf(proxy_class_code, " }\n"); + Printf(proxy_class_code, " }\n\n"); + Printf(proxy_class_code, " if (!parametersMatch)\n"); + Printf(proxy_class_code, " continue;\n\n"); + Printf(proxy_class_code, " if (methodInfo.IsVirtual && (methodInfo.DeclaringType.IsSubclassOf(typeof(%s))) &&\n", proxy_class_name); + Printf(proxy_class_code, " methodInfo.DeclaringType != methodInfo.GetBaseDefinition().DeclaringType) {\n"); + Printf(proxy_class_code, " return true;\n"); + Printf(proxy_class_code, " }\n"); + Printf(proxy_class_code, " }\n\n"); + Printf(proxy_class_code, " return false;\n"); + /* Could add this code to cover corner case where the GetMethod() returns a method which allows type * promotion, eg it will return foo(double), if looking for foo(int). if (hasDerivedMethod) { @@ -1973,7 +1996,7 @@ public: } } */ - Printf(proxy_class_code, " return hasDerivedMethod;\n"); + //Printf(proxy_class_code, " return hasDerivedMethod;\n"); Printf(proxy_class_code, " }\n"); }