Switch multicast delegate stub on Windows x64 to use stubs-as-il

Fixes #11611. The old hand generated assembly path did not work well for structs passed by reference.
dotnet · May 15, 2017 · 96f1571 · 96f1571
1 parent fd7b8ca
commit 96f1571
Show file tree

Hide file tree

Showing 17 changed files with 135 additions and 34 deletions.
diff --git a/clr.coreclr.props b/clr.coreclr.props
@@ -66,6 +66,7 @@
     <FeatureXplatEventSource>true</FeatureXplatEventSource>
 
     <FeatureArrayStubAsIL>true</FeatureArrayStubAsIL>
+    <FeatureMulticastStubAsIL>true</FeatureMulticastStubAsIL>
     <FeatureStubsAsIL>true</FeatureStubsAsIL>
 
     <!-- Windows specific features -->
@@ -81,7 +82,8 @@
   </PropertyGroup>
 
   <PropertyGroup Condition="'$(TargetsWindows)' == 'true'">
-    <FeatureArrayStubAsIL Condition="('$(TargetArch)' == 'arm') or ('$(TargetArch)' == 'amd64') or ('$(TargetArch)' == 'arm64')">true</FeatureArrayStubAsIL>
+    <FeatureArrayStubAsIL Condition="'$(TargetArch)' != 'i386'">true</FeatureArrayStubAsIL>
+    <FeatureMulticastStubAsIL Condition="'$(TargetArch)' != 'i386'">true</FeatureMulticastStubAsIL>
     <FeatureManagedEtw>true</FeatureManagedEtw>
     <FeatureStubsAsIL Condition="'$(TargetArch)' == 'arm64'">true</FeatureStubsAsIL>
     <FeatureUseLcid>true</FeatureUseLcid>

diff --git a/clr.defines.targets b/clr.defines.targets
@@ -3,6 +3,7 @@
         <DefineConstants Condition="'$(BuildTypeRet)' == 'true'">$(DefineConstants);BUILDTYPE_RET</DefineConstants>
         <DefineConstants Condition="'$(FeatureAppX)' == 'true'">$(DefineConstants);FEATURE_APPX</DefineConstants>
         <DefineConstants Condition="'$(FeatureArrayStubAsIL)' == 'true'">$(DefineConstants);FEATURE_ARRAYSTUB_AS_IL</DefineConstants>
+        <DefineConstants Condition="'$(FeatureMulticastStubAsIL)' == 'true'">$(DefineConstants);FEATURE_MULTICASTSTUB_AS_IL</DefineConstants>
         <DefineConstants Condition="'$(FeatureStubsAsIL)' == 'true'">$(DefineConstants);FEATURE_STUBS_AS_IL</DefineConstants>
         <DefineConstants Condition="'$(FeatureClassicCominterop)' == 'true'">$(DefineConstants);FEATURE_CLASSIC_COMINTEROP</DefineConstants>
         <DefineConstants Condition="'$(FeatureCominterop)' == 'true'">$(DefineConstants);FEATURE_COMINTEROP</DefineConstants>

diff --git a/clrdefinitions.cmake b/clrdefinitions.cmake
@@ -81,11 +81,13 @@ endif(WIN32)
 add_definitions(-DFEATURE_APPDOMAIN_RESOURCE_MONITORING)
 if(WIN32)
   add_definitions(-DFEATURE_APPX)
-  if(CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_ARM OR CLR_CMAKE_TARGET_ARCH_ARM64)
+  if(NOT CLR_CMAKE_TARGET_ARCH_I386)
     add_definitions(-DFEATURE_ARRAYSTUB_AS_IL)
+    add_definitions(-DFEATURE_MULTICASTSTUB_AS_IL)
   endif()
 else(WIN32)
   add_definitions(-DFEATURE_ARRAYSTUB_AS_IL)
+  add_definitions(-DFEATURE_MULTICASTSTUB_AS_IL)
 endif(WIN32)
 
 add_definitions(-DFEATURE_COLLECTIBLE_TYPES)

diff --git a/src/mscorlib/src/System/StubHelpers.cs b/src/mscorlib/src/System/StubHelpers.cs
@@ -1798,7 +1798,7 @@ static internal void CheckStringLength(uint length)
         internal static extern void ArrayTypeCheck(object o, Object[] arr);
 #endif
 
-#if FEATURE_STUBS_AS_IL
+#if FEATURE_MULTICASTSTUB_AS_IL
         [MethodImplAttribute(MethodImplOptions.InternalCall)]
         internal static extern void MulticastDebuggerTraceHelper(object o, Int32 count);
 #endif

diff --git a/src/vm/comdelegate.cpp b/src/vm/comdelegate.cpp
@@ -2631,7 +2631,7 @@ FCIMPL1(MethodDesc*, COMDelegate::GetInvokeMethod, Object* refThisIn)
 }
 FCIMPLEND
 
-#ifdef FEATURE_STUBS_AS_IL
+#ifdef FEATURE_MULTICASTSTUB_AS_IL
 FCIMPL1(PCODE, COMDelegate::GetMulticastInvoke, Object* refThisIn)
 {
     FCALL_CONTRACT;
@@ -2754,7 +2754,7 @@ FCIMPL1(PCODE, COMDelegate::GetMulticastInvoke, Object* refThisIn)
 }
 FCIMPLEND
 
-#else // FEATURE_STUBS_AS_IL
+#else // FEATURE_MULTICASTSTUB_AS_IL
 
 FCIMPL1(PCODE, COMDelegate::GetMulticastInvoke, Object* refThisIn)
 {
@@ -2813,7 +2813,7 @@ FCIMPL1(PCODE, COMDelegate::GetMulticastInvoke, Object* refThisIn)
     return pStub->GetEntryPoint();
 }
 FCIMPLEND
-#endif // FEATURE_STUBS_AS_IL
+#endif // FEATURE_MULTICASTSTUB_AS_IL
 
 #ifdef FEATURE_STUBS_AS_IL
 PCODE COMDelegate::GetSecureInvoke(MethodDesc* pMD)

diff --git a/src/vm/dllimport.h b/src/vm/dllimport.h
@@ -198,8 +198,10 @@ enum ILStubTypes
     ILSTUB_ARRAYOP_SET                   = 0x80000002,
     ILSTUB_ARRAYOP_ADDRESS               = 0x80000004,
 #endif
-#ifdef FEATURE_STUBS_AS_IL
+#ifdef FEATURE_MULTICASTSTUB_AS_IL
     ILSTUB_MULTICASTDELEGATE_INVOKE      = 0x80000010,
+#endif
+#ifdef FEATURE_STUBS_AS_IL
     ILSTUB_UNBOXINGILSTUB                = 0x80000020,
     ILSTUB_INSTANTIATINGSTUB             = 0x80000040,
     ILSTUB_SECUREDELEGATE_INVOKE         = 0x80000080,
@@ -232,9 +234,12 @@ inline bool SF_IsArrayOpStub           (DWORD dwStubFlags) { LIMITED_METHOD_CONT
                                                                                               (dwStubFlags == ILSTUB_ARRAYOP_ADDRESS)); }
 #endif
 
+#ifdef FEATURE_MULTICASTSTUB_AS_IL
+inline bool SF_IsMulticastDelegateStub  (DWORD dwStubFlags) { LIMITED_METHOD_CONTRACT; return (dwStubFlags == ILSTUB_MULTICASTDELEGATE_INVOKE); }
+#endif
+
 #ifdef FEATURE_STUBS_AS_IL
 inline bool SF_IsSecureDelegateStub  (DWORD dwStubFlags)    { LIMITED_METHOD_CONTRACT; return (dwStubFlags == ILSTUB_SECUREDELEGATE_INVOKE); }
-inline bool SF_IsMulticastDelegateStub  (DWORD dwStubFlags) { LIMITED_METHOD_CONTRACT; return (dwStubFlags == ILSTUB_MULTICASTDELEGATE_INVOKE); }
 inline bool SF_IsUnboxingILStub         (DWORD dwStubFlags) { LIMITED_METHOD_CONTRACT; return (dwStubFlags == ILSTUB_UNBOXINGILSTUB); }
 inline bool SF_IsInstantiatingStub      (DWORD dwStubFlags) { LIMITED_METHOD_CONTRACT; return (dwStubFlags == ILSTUB_INSTANTIATINGSTUB); }
 #endif

diff --git a/src/vm/ecalllist.h b/src/vm/ecalllist.h
@@ -1231,9 +1231,9 @@ FCFuncStart(gStubHelperFuncs)
 #ifdef FEATURE_ARRAYSTUB_AS_IL
     FCFuncElement("ArrayTypeCheck", StubHelpers::ArrayTypeCheck)
 #endif //FEATURE_ARRAYSTUB_AS_IL
-#ifdef FEATURE_STUBS_AS_IL
+#ifdef FEATURE_MULTICASTSTUB_AS_IL
     FCFuncElement("MulticastDebuggerTraceHelper", StubHelpers::MulticastDebuggerTraceHelper)
-#endif //FEATURE_STUBS_AS_IL
+#endif //FEATURE_MULTICASTSTUB_AS_IL
 FCFuncEnd()
 
 FCFuncStart(gGCHandleFuncs)

diff --git a/src/vm/ilstubcache.cpp b/src/vm/ilstubcache.cpp
@@ -216,29 +216,31 @@ MethodDesc* ILStubCache::CreateNewMethodDesc(LoaderHeap* pCreationHeap, MethodTa
     }
     else
 #endif
+#ifdef FEATURE_MULTICASTSTUB_AS_IL
+    if (SF_IsMulticastDelegateStub(dwStubFlags))
+    {
+        pMD->m_dwExtendedFlags |= DynamicMethodDesc::nomdMulticastStub;
+        pMD->GetILStubResolver()->SetStubType(ILStubResolver::MulticastDelegateStub);
+    }
+    else
+#endif
 #ifdef FEATURE_STUBS_AS_IL
     if (SF_IsSecureDelegateStub(dwStubFlags))
     {
         pMD->m_dwExtendedFlags |= DynamicMethodDesc::nomdSecureDelegateStub;
         pMD->GetILStubResolver()->SetStubType(ILStubResolver::SecureDelegateStub);
     }
     else
-    if (SF_IsMulticastDelegateStub(dwStubFlags))
-    {
-        pMD->m_dwExtendedFlags |= DynamicMethodDesc::nomdMulticastStub;
-        pMD->GetILStubResolver()->SetStubType(ILStubResolver::MulticastDelegateStub);
-    }	
-    else
     if (SF_IsUnboxingILStub(dwStubFlags))
     {
         pMD->m_dwExtendedFlags |= DynamicMethodDesc::nomdUnboxingILStub;
         pMD->GetILStubResolver()->SetStubType(ILStubResolver::UnboxingILStub);
-    }	
+    }
     else
     if (SF_IsInstantiatingStub(dwStubFlags))
     {
         pMD->GetILStubResolver()->SetStubType(ILStubResolver::InstantiatingStub);
-    }	
+    }
     else
 #endif
 #ifdef FEATURE_COMINTEROP

diff --git a/src/vm/ilstubresolver.cpp b/src/vm/ilstubresolver.cpp
@@ -87,8 +87,10 @@ LPCUTF8 ILStubResolver::GetStubMethodName()
 #ifdef FEATURE_ARRAYSTUB_AS_IL
         case ArrayOpStub:            return "IL_STUB_Array";
 #endif
-#ifdef FEATURE_STUBS_AS_IL
+#ifdef FEATURE_MULTICASTSTUB_AS_IL
         case MulticastDelegateStub:  return "IL_STUB_MulticastDelegate_Invoke";
+#endif
+#ifdef FEATURE_STUBS_AS_IL
         case UnboxingILStub:         return "IL_STUB_UnboxingStub";
         case InstantiatingStub:      return "IL_STUB_InstantiatingStub";
         case SecureDelegateStub:     return "IL_STUB_SecureDelegate_Invoke";

diff --git a/src/vm/ilstubresolver.h b/src/vm/ilstubresolver.h
@@ -82,9 +82,11 @@ class ILStubResolver : DynamicResolver
 #ifdef FEATURE_ARRAYSTUB_AS_IL 
         ArrayOpStub,
 #endif
+#ifdef FEATURE_MULTICASTSTUB_AS_IL
+        MulticastDelegateStub,
+#endif
 #ifdef FEATURE_STUBS_AS_IL
         SecureDelegateStub,
-        MulticastDelegateStub,
         UnboxingILStub,
         InstantiatingStub,
 #endif

diff --git a/src/vm/method.hpp b/src/vm/method.hpp
@@ -2441,17 +2441,19 @@ class DynamicMethodDesc : public StoredSigMethodDesc
     bool IsDelegateCOMStub() { LIMITED_METHOD_CONTRACT; _ASSERTE(IsILStub()); return (0 != (m_dwExtendedFlags & nomdDelegateCOMStub));  }
     bool IsSignatureNeedsRestore() { LIMITED_METHOD_CONTRACT; _ASSERTE(IsILStub()); return (0 != (m_dwExtendedFlags & nomdSignatureNeedsRestore)); }
     bool IsStubNeedsCOMStarted()   { LIMITED_METHOD_CONTRACT; _ASSERTE(IsILStub()); return (0 != (m_dwExtendedFlags & nomdStubNeedsCOMStarted)); }
+#ifdef FEATURE_MULTICASTSTUB_AS_IL
+    bool IsMulticastStub() {
+        LIMITED_METHOD_DAC_CONTRACT;
+        _ASSERTE(IsILStub());
+        return !!(m_dwExtendedFlags & nomdMulticastStub);
+    }
+#endif
 #ifdef FEATURE_STUBS_AS_IL
     bool IsSecureDelegateStub() {
         LIMITED_METHOD_DAC_CONTRACT;
         _ASSERTE(IsILStub());
         return !!(m_dwExtendedFlags & nomdSecureDelegateStub);
     }
-    bool IsMulticastStub() { 
-        LIMITED_METHOD_DAC_CONTRACT; 
-        _ASSERTE(IsILStub());
-        return !!(m_dwExtendedFlags & nomdMulticastStub);
-    }
     bool IsUnboxingILStub() { 
         LIMITED_METHOD_DAC_CONTRACT; 
         _ASSERTE(IsILStub());

diff --git a/src/vm/mscorlib.h b/src/vm/mscorlib.h
@@ -1099,7 +1099,7 @@ DEFINE_METHOD(STUBHELPERS,          PROFILER_END_TRANSITION_CALLBACK,   Profiler
 DEFINE_METHOD(STUBHELPERS,          ARRAY_TYPE_CHECK,    ArrayTypeCheck,          SM_Obj_ArrObject_RetVoid)
 #endif
 
-#ifdef FEATURE_STUBS_AS_IL
+#ifdef FEATURE_MULTICASTSTUB_AS_IL
 DEFINE_METHOD(STUBHELPERS,          MULTICAST_DEBUGGER_TRACE_HELPER,    MulticastDebuggerTraceHelper,    SM_Obj_Int_RetVoid)
 #endif
 

diff --git a/src/vm/stubhelpers.cpp b/src/vm/stubhelpers.cpp
@@ -1970,11 +1970,11 @@ FCIMPL2(void, StubHelpers::ArrayTypeCheck, Object* element, PtrArray* arr)
 FCIMPLEND
 #endif // FEATURE_ARRAYSTUB_AS_IL
 
-#ifdef FEATURE_STUBS_AS_IL
+#ifdef FEATURE_MULTICASTSTUB_AS_IL
 FCIMPL2(void, StubHelpers::MulticastDebuggerTraceHelper, Object* element, INT32 count)
 {
     FCALL_CONTRACT;
     FCUnique(0xa5);
 }
 FCIMPLEND
-#endif // FEATURE_STUBS_AS_IL
+#endif // FEATURE_MULTICASTSTUB_AS_IL
diff --git a/src/vm/stubhelpers.h b/src/vm/stubhelpers.h
@@ -153,7 +153,7 @@ class StubHelpers
     static FCDECL2(ReflectClassBaseObject *,    WinRTTypeNameConverter__GetTypeFromWinRTTypeName, StringObject *pWinRTTypeNameUNSAFE, CLR_BOOL *pbIsPrimitive);
 #endif // FEATURE_COMINTEROP
 
-#ifdef FEATURE_STUBS_AS_IL
+#ifdef FEATURE_MULTICASTSTUB_AS_IL
     static FCDECL2(void,            MulticastDebuggerTraceHelper, Object*, INT32);
 #endif
 };

diff --git a/src/vm/stubmgr.cpp b/src/vm/stubmgr.cpp
@@ -1706,14 +1706,14 @@ BOOL ILStubManager::DoTraceStub(PCODE stubStartAddress,
 
     PCODE traceDestination = NULL;
 
-#ifdef FEATURE_STUBS_AS_IL
+#ifdef FEATURE_MULTICASTSTUB_AS_IL
     MethodDesc* pStubMD = ExecutionManager::GetCodeMethodDesc(stubStartAddress);
     if (pStubMD != NULL && pStubMD->AsDynamicMethodDesc()->IsMulticastStub())
     {
         traceDestination = GetEEFuncEntryPoint(StubHelpers::MulticastDebuggerTraceHelper);
     }
     else
-#endif // FEATURE_STUBS_AS_IL
+#endif // FEATURE_MULTICASTSTUB_AS_IL
     {
         // This call is going out to unmanaged code, either through pinvoke or COM interop.
         traceDestination = stubStartAddress;
@@ -1813,7 +1813,7 @@ BOOL ILStubManager::TraceManager(Thread *thread,
     PCODE stubIP = GetIP(pContext);
     *pRetAddr = (BYTE *)StubManagerHelpers::GetReturnAddress(pContext);
 
-#ifdef FEATURE_STUBS_AS_IL
+#ifdef FEATURE_MULTICASTSTUB_AS_IL
     if (stubIP == GetEEFuncEntryPoint(StubHelpers::MulticastDebuggerTraceHelper))
     {
         stubIP = (PCODE)*pRetAddr;
@@ -1830,7 +1830,7 @@ BOOL ILStubManager::TraceManager(Thread *thread,
     // See code:ILStubCache.CreateNewMethodDesc for the code that sets flags on stub MDs
     PCODE target;
 
-#ifdef FEATURE_STUBS_AS_IL
+#ifdef FEATURE_MULTICASTSTUB_AS_IL
     if(pStubMD->IsMulticastStub())
     {
         _ASSERTE(GetIP(pContext) == GetEEFuncEntryPoint(StubHelpers::MulticastDebuggerTraceHelper));
@@ -1859,7 +1859,7 @@ BOOL ILStubManager::TraceManager(Thread *thread,
 
     }
     else 
-#endif // FEATURE_STUBS_AS_IL
+#endif // FEATURE_MULTICASTSTUB_AS_IL
     if (pStubMD->IsReverseStub())
     {
         if (pStubMD->IsStatic())

diff --git a/tests/src/Regressions/coreclr/GitHub_11611/Test11611.csproj b/tests/src/Regressions/coreclr/GitHub_11611/Test11611.csproj
@@ -0,0 +1,39 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{E55A6F8B-B9E3-45CE-88F4-22AE70F606CB}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <CLRTestKind>BuildAndRun</CLRTestKind>
+    <CLRTestPriority>1</CLRTestPriority>
+  </PropertyGroup>
+  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
+  </PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
+  </PropertyGroup>
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+  <ItemGroup>
+    <!-- Add Compile Object Here -->
+    <Compile Include="test11611.cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <ItemGroup>
+    <ProjectReference Include="../../../Common/CoreCLRTestLibrary/CoreCLRTestLibrary.csproj" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
+  </PropertyGroup>
+</Project>
diff --git a/tests/src/Regressions/coreclr/GitHub_11611/test11611.cs b/tests/src/Regressions/coreclr/GitHub_11611/test11611.cs
@@ -0,0 +1,44 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+using System;
+
+public struct Data
+{
+    public long Int;
+    public object Obj;
+}
+
+public class Test11611
+{    
+    static bool fail = false;
+
+    public static void handle(Data data, long value)
+    {
+        Console.WriteLine("handle( ( i:{0}, o:{1} ), {2} )", data.Int, data.Obj, value);
+
+        if (data.Int != 0)
+        {
+            fail = true;
+        }
+
+        if (data.Obj != null)
+        {
+            fail = true;
+        }
+
+        data.Int = value;
+        data.Obj = value;
+    }
+
+    public static int Main()
+    {
+        Action<Data, long> handler = handle;
+        handler += handle;
+
+        var data = new Data();
+        handler(data, 123);
+
+        return fail ? -1 : 100;
+    }
+}