From ae7c4cd829234d9095bf1f2c94fb8cd0b8d8ddb7 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 20 Mar 2024 16:18:38 -0700 Subject: [PATCH 01/12] Add repro test case --- .../CopyConstructorMarshaler.cs | 11 +++ .../IjwCopyConstructorMarshaler.cpp | 90 +++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs b/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs index 376b64e623c82..6db8fbcf3378f 100644 --- a/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs +++ b/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs @@ -54,6 +54,17 @@ public static int TestEntryPoint() return 100; } + [Fact] + public static void CopyConstructorsInArgumentStackSlots() + { + Assembly ijwNativeDll = Assembly.Load("IjwCopyConstructorMarshaler"); + Type testType = ijwNativeDll.GetType("TestClass"); + object testInstance = Activator.CreateInstance(testType); + MethodInfo testMethod = testType.GetMethod("ExposedThisCopyConstructorScenario"); + + Assert.Equal(0, (int)testMethod.Invoke(testInstance, null)); + } + [DllImport("kernel32.dll")] static extern IntPtr LoadLibraryEx(string lpFileName, IntPtr hReservedNull, int dwFlags); diff --git a/src/tests/Interop/IJW/CopyConstructorMarshaler/IjwCopyConstructorMarshaler.cpp b/src/tests/Interop/IJW/CopyConstructorMarshaler/IjwCopyConstructorMarshaler.cpp index bd1d1b80829d3..c3d50cf77836e 100644 --- a/src/tests/Interop/IJW/CopyConstructorMarshaler/IjwCopyConstructorMarshaler.cpp +++ b/src/tests/Interop/IJW/CopyConstructorMarshaler/IjwCopyConstructorMarshaler.cpp @@ -1,5 +1,90 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +#pragma unmanaged +#include +#include + +namespace ExposedThis +{ + struct Relative; + + std::vector relatives; + + int numMissedCopies = 0; + + struct Relative + { + void* relative; + Relative() + { + std::cout << "Registering " << std::hex << this << "\n"; + relatives.push_back(this); + relative = this - 1; + } + + Relative(const Relative& other) + { + std::cout << "Registering copy of " << std::hex << &other << " at " << this << "\n"; + relatives.push_back(this); + relative = this - 1; + } + + ~Relative() + { + auto location = std::find(relatives.begin(), relatives.end(), this); + if (location != relatives.end()) + { + std::cout << "Unregistering " << std::hex << this << "\n"; + relatives.erase(location); + } + else + { + std::cout << "Error: Relative object " << std::hex << this << " not registered\n"; + numMissedCopies++; + } + + if (relative != this - 1) + { + std::cout << " Error: Relative object " << std::hex << this << " has invalid relative pointer " << std::hex << relative << "\n"; + numMissedCopies++; + } + } + }; + + void UseRelative(Relative rel) + { + std::cout << "Unmanaged: Using relative at address " << std::hex << &rel << "\n"; + } + + void UseRelativeManaged(Relative rel); + + void CallRelative() + { + Relative rel; + UseRelativeManaged(rel); + } + +#pragma managed + + int RunScenario() + { + // Managed to unmanaged + { + Relative rel; + UseRelative(rel); + } + + // Unmanaged to managed + CallRelative(); + + return numMissedCopies; + } + + void UseRelativeManaged(Relative rel) + { + std::cout << "Managed: Using relative at address " << std::hex << &rel << "\n"; + } +} #pragma managed class A @@ -102,4 +187,9 @@ public ref class TestClass B b; return GetCopyCount_ViaManaged(b); } + + int ExposedThisCopyConstructorScenario() + { + return ExposedThis::RunScenario(); + } }; From 0ea31dc33a6f8c2b1219c024b1a2a20a2a62b8fd Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 20 Mar 2024 16:19:13 -0700 Subject: [PATCH 02/12] Directly load the argument address using ldarga to avoid making a copy --- src/coreclr/vm/ilmarshalers.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/coreclr/vm/ilmarshalers.cpp b/src/coreclr/vm/ilmarshalers.cpp index a06cb57e1df0b..84ecfd3fad2cc 100644 --- a/src/coreclr/vm/ilmarshalers.cpp +++ b/src/coreclr/vm/ilmarshalers.cpp @@ -3477,9 +3477,7 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver DWORD dwNewValueTypeLocal; dwNewValueTypeLocal = pslIL->NewLocal(locDesc); - pslILDispatch->EmitLDARG(argidx); - pslILDispatch->EmitSTLOC(dwNewValueTypeLocal); - pslILDispatch->EmitLDLOCA(dwNewValueTypeLocal); + pslILDispatch->EmitLDARGA(argidx); #else LocalDesc locDesc(pargs->mm.m_pMT); locDesc.MakePointer(); From a8d425fe96be17e9f5193a59b357128d45b3bf5f Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 20 Mar 2024 18:05:56 -0700 Subject: [PATCH 03/12] Reimplement the "Copy Constructor Cookie" logic in a more modern and maintainable style to get the test passing again --- .../src/System/StubHelpers.cs | 69 +++++++++++++++++++ src/coreclr/vm/corelib.h | 11 +++ src/coreclr/vm/dllimport.cpp | 57 +++++++++++++++ src/coreclr/vm/dllimport.h | 7 ++ src/coreclr/vm/i386/asmhelpers.asm | 24 +++++++ src/coreclr/vm/ilmarshalers.cpp | 30 ++++++++ src/coreclr/vm/metasig.h | 5 ++ .../CopyConstructorMarshaler.cs | 15 ++-- 8 files changed, 214 insertions(+), 4 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs b/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs index 81c0dd8e1afec..e772613855c76 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs @@ -1315,6 +1315,75 @@ public IntPtr AddRef() } } // class CleanupWorkListElement + internal unsafe struct CopyConstructorCookie + { + private void* m_source; + + private nuint m_destinationOffset; + + public delegate* m_copyConstructor; + + public delegate* m_destructor; + + public CopyConstructorCookie* m_next; + + [StackTraceHidden] + public void ExecuteCopy(void* destinationBase) + { + if (m_copyConstructor != null) + { + m_copyConstructor((byte*)destinationBase + m_destinationOffset, m_source); + } + + if (m_destructor != null) + { + m_destructor(m_source); + } + } + } + + internal unsafe struct CopyConstructorChain + { + public void* m_realTarget; + public CopyConstructorCookie* m_head; + + public void Add(CopyConstructorCookie* cookie) + { + cookie->m_next = m_head; + m_head = cookie; + } + + [ThreadStatic] + private static CopyConstructorChain s_copyConstructorChain; + + public void Install(void* realTarget) + { + m_realTarget = realTarget; + s_copyConstructorChain = this; + } + + [StackTraceHidden] + private void ExecuteCopies(void* destinationBase) + { + for (CopyConstructorCookie* current = m_head; current != null; current = current->m_next) + { + current->ExecuteCopy(destinationBase); + } + } + + [UnmanagedCallersOnly] + [StackTraceHidden] + public static void* ExecuteCurrentCopiesAndGetTarget(void* destinationBase) + { + void* target = s_copyConstructorChain.m_realTarget; + s_copyConstructorChain.ExecuteCopies(destinationBase); + // Reset this instance to ensure we don't accidentally execute the copies again. + // All of the pointers point to the stack, so we don't need to free any memory. + s_copyConstructorChain = default; + return target; + } + } + internal static partial class StubHelpers { [MethodImpl(MethodImplOptions.InternalCall)] diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index c52c58954165a..5411cdd9c8cf3 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -1040,6 +1040,17 @@ DEFINE_METHOD(HANDLE_MARSHALER, CONVERT_SAFEHANDLE_TO_NATIVE,ConvertSaf DEFINE_METHOD(HANDLE_MARSHALER, THROW_SAFEHANDLE_FIELD_CHANGED, ThrowSafeHandleFieldChanged, SM_RetVoid) DEFINE_METHOD(HANDLE_MARSHALER, THROW_CRITICALHANDLE_FIELD_CHANGED, ThrowCriticalHandleFieldChanged, SM_RetVoid) +DEFINE_CLASS(COPY_CONSTRUCTOR_CHAIN, StubHelpers, CopyConstructorChain) +DEFINE_METHOD(COPY_CONSTRUCTOR_CHAIN, EXECUTE_CURRENT_COPIES_AND_GET_TARGET, ExecuteCurrentCopiesAndGetTarget, SM_PtrVoid_RetPtrVoid) +DEFINE_METHOD(COPY_CONSTRUCTOR_CHAIN, INSTALL, Install, IM_PtrVoid_RetVoid) +DEFINE_METHOD(COPY_CONSTRUCTOR_CHAIN, ADD, Add, IM_PtrCopyConstructorCookie_RetVoid) + +DEFINE_CLASS(COPY_CONSTRUCTOR_COOKIE, StubHelpers, CopyConstructorCookie) +DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, SOURCE, m_source) +DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, DESTINATION_OFFSET, m_destinationOffset) +DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, COPY_CONSTRUCTOR, m_copyConstructor) +DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, DESTRUCTOR, m_destructor) + DEFINE_CLASS(COMVARIANT, Marshalling, ComVariant) DEFINE_CLASS(SZARRAYHELPER, System, SZArrayHelper) diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp index 12ca187ecbeac..e5942a356f58d 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -1630,6 +1630,10 @@ NDirectStubLinker::NDirectStubLinker( m_pcsSetup->EmitSTLOC(m_dwTargetInterfacePointerLocalNum); } #endif // FEATURE_COMINTEROP + +#if defined(TARGET_WINDOWS) && defined(TARGET_X86) + m_dwCopyCtorChainLocalNum = -1; +#endif } void NDirectStubLinker::SetCallingConvention(CorInfoCallConvExtension unmngCallConv, BOOL fIsVarArg) @@ -1842,6 +1846,23 @@ DWORD NDirectStubLinker::GetReturnValueLocalNum() return m_dwRetValLocalNum; } +#if defined(TARGET_WINDOWS) && defined(TARGET_X86) +DWORD NDirectStubLinker::GetCopyCtorChainLocalNum() +{ + STANDARD_VM_CONTRACT; + + if (m_dwCopyCtorChainLocalNum == (DWORD)-1) + { + // The local is created and initialized lazily when first asked. + m_dwCopyCtorChainLocalNum = NewLocal(CoreLibBinder::GetClass(CLASS__COPY_CONSTRUCTOR_CHAIN)); + m_pcsSetup->EmitLDLOCA(m_dwCopyCtorChainLocalNum); + m_pcsSetup->EmitINITOBJ(m_pcsSetup->GetToken(CoreLibBinder::GetClass(CLASS__COPY_CONSTRUCTOR_CHAIN))); + } + + return m_dwCopyCtorChainLocalNum; +} +#endif + BOOL NDirectStubLinker::IsCleanupNeeded() { LIMITED_METHOD_CONTRACT; @@ -2071,6 +2092,8 @@ void NDirectStubLinker::End(DWORD dwStubFlags) } } +EXTERN_C void STDCALL CopyConstructorCallStub(void); + void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, MethodDesc * pStubMD) { STANDARD_VM_CONTRACT; @@ -2154,6 +2177,21 @@ void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, Meth } } +#if defined(TARGET_WINDOWS) && defined(TARGET_X86) + if (m_dwCopyCtorChainLocalNum != -1) + { + // If we have a copy constructor chain local, we need to call the copy constructor stub + // to ensure that the chain is called correctly. + // Let's install the stub chain here and redirect the call to the stub. + DWORD targetLoc = NewLocal(ELEMENT_TYPE_I); + pcsEmit->EmitSTLOC(targetLoc); + pcsEmit->EmitLDLOCA(m_dwCopyCtorChainLocalNum); + pcsEmit->EmitLDLOC(targetLoc); + pcsEmit->EmitCALL(METHOD__COPY_CONSTRUCTOR_CHAIN__INSTALL, 2, 0); + pcsEmit->EmitLDC((DWORD_PTR)&CopyConstructorCallStub); + } +#endif // defined(TARGET_WINDOWS) && defined(TARGET_X86) + // For managed-to-native calls, the rest of the work is done by the JIT. It will // erect InlinedCallFrame, flip GC mode, and use the specified calling convention // to call the target. For native-to-managed calls, this is an ordinary managed @@ -6101,5 +6139,24 @@ PCODE GetILStubForCalli(VASigCookie *pVASigCookie, MethodDesc *pMD) RETURN pVASigCookie->pNDirectILStub; } +#if defined(TARGET_X86) && defined(TARGET_WINDOWS) +// Copy constructor support for C++/CLI +EXTERN_C void* STDCALL CallCopyConstructorsWorker(void* esp) +{ + STATIC_CONTRACT_THROWS; + STATIC_CONTRACT_GC_TRIGGERS; + STATIC_CONTRACT_MODE_PREEMPTIVE; // we've already switched to preemptive + + using ExecuteCallback = void*(STDMETHODCALLTYPE*)(void*); + ExecuteCallback pExecute; + { + GCX_COOP(); + MethodDesc* pMD = CoreLibBinder::GetMethod(METHOD__COPY_CONSTRUCTOR_CHAIN__EXECUTE_CURRENT_COPIES_AND_GET_TARGET); + pExecute = (ExecuteCallback)pMD->GetMultiCallableAddrOfCode(); + } + + return pExecute(esp); +} +#endif #endif // #ifndef DACCESS_COMPILE diff --git a/src/coreclr/vm/dllimport.h b/src/coreclr/vm/dllimport.h index 111c7436c9473..a75d1333f73c1 100644 --- a/src/coreclr/vm/dllimport.h +++ b/src/coreclr/vm/dllimport.h @@ -496,6 +496,9 @@ class NDirectStubLinker : public ILStubLinker DWORD GetCleanupWorkListLocalNum(); DWORD GetThreadLocalNum(); DWORD GetReturnValueLocalNum(); +#if defined(TARGET_WINDOWS) && defined(TARGET_X86) + DWORD GetCopyCtorChainLocalNum(); +#endif void SetCleanupNeeded(); void SetExceptionCleanupNeeded(); BOOL IsCleanupWorkListSetup(); @@ -565,6 +568,10 @@ class NDirectStubLinker : public ILStubLinker DWORD m_dwTargetEntryPointLocalNum; #endif // FEATURE_COMINTEROP +#if defined(TARGET_WINDOWS) && defined(TARGET_X86) + DWORD m_dwCopyCtorChainLocalNum; +#endif + BOOL m_fHasCleanupCode; BOOL m_fHasExceptionCleanupCode; BOOL m_fCleanupWorkListIsSetup; diff --git a/src/coreclr/vm/i386/asmhelpers.asm b/src/coreclr/vm/i386/asmhelpers.asm index e03ffa9544f2c..1d02fc48f8d8b 100644 --- a/src/coreclr/vm/i386/asmhelpers.asm +++ b/src/coreclr/vm/i386/asmhelpers.asm @@ -41,6 +41,7 @@ EXTERN _NDirectImportWorker@4:PROC EXTERN _VarargPInvokeStubWorker@12:PROC EXTERN _GenericPInvokeCalliStubWorker@12:PROC +EXTERN _CallCopyConstructorsWorker@4:PROC EXTERN _PreStubWorker@8:PROC EXTERN _TheUMEntryPrestubWorker@4:PROC @@ -1062,6 +1063,29 @@ GoCallCalliWorker: _GenericPInvokeCalliHelper@0 endp +;========================================================================== +; This is small stub whose purpose is to record current stack pointer and +; call CallCopyConstructorsWorker to invoke copy constructors and destructors +; as appropriate. This stub operates on arguments already pushed to the +; stack by JITted IL stub and must not create a new frame, i.e. it must tail +; call to the target for it to see the arguments that copy ctors have been +; called on. +; +_CopyConstructorCallStub@0 proc public + ; there may be an argument in ecx - save it + push ecx + + ; push pointer to arguments + lea edx, [esp + 8] + push edx + + call _CallCopyConstructorsWorker@4 + + ; restore ecx and tail call to the target + pop ecx + jmp eax +_CopyConstructorCallStub@0 endp + ifdef FEATURE_COMINTEROP ;========================================================================== diff --git a/src/coreclr/vm/ilmarshalers.cpp b/src/coreclr/vm/ilmarshalers.cpp index 84ecfd3fad2cc..a603cd5d5fe75 100644 --- a/src/coreclr/vm/ilmarshalers.cpp +++ b/src/coreclr/vm/ilmarshalers.cpp @@ -3459,6 +3459,36 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver #ifdef TARGET_X86 pslIL->SetStubTargetArgType(&locDesc); // native type is the value type pslILDispatch->EmitLDLOC(dwNewValueTypeLocal); // we load the local directly + + // Record this argument's stack slot in the copy constructor chain so we can correctly invoke the copy constructor. + DWORD ctorCookie = pslIL->NewLocal(CoreLibBinder::GetClass(CLASS__COPY_CONSTRUCTOR_COOKIE)); + pslIL->EmitLDLOCA(ctorCookie); + pslIL->EmitINITOBJ(pslIL->GetToken(CoreLibBinder::GetClass(CLASS__COPY_CONSTRUCTOR_COOKIE))); + pslIL->EmitLDLOCA(ctorCookie); + pslIL->EmitLDLOCA(dwNewValueTypeLocal); + pslIL->EmitSTFLD(pslIL->GetToken(CoreLibBinder::GetField(FIELD__COPY_CONSTRUCTOR_COOKIE__SOURCE))); + pslIL->EmitLDLOCA(ctorCookie); + pslIL->EmitLDC(nativeStackOffset); + pslIL->EmitSTFLD(pslIL->GetToken(CoreLibBinder::GetField(FIELD__COPY_CONSTRUCTOR_COOKIE__DESTINATION_OFFSET))); + + if (pargs->mm.m_pCopyCtor) + { + pslIL->EmitLDLOCA(ctorCookie); + pslIL->EmitLDFTN(pslIL->GetToken(pargs->mm.m_pCopyCtor)); + pslIL->EmitSTFLD(pslIL->GetToken(CoreLibBinder::GetField(FIELD__COPY_CONSTRUCTOR_COOKIE__COPY_CONSTRUCTOR))); + } + + if (pargs->mm.m_pDtor) + { + pslIL->EmitLDLOCA(ctorCookie); + pslIL->EmitLDFTN(pslIL->GetToken(pargs->mm.m_pDtor)); + pslIL->EmitSTFLD(pslIL->GetToken(CoreLibBinder::GetField(FIELD__COPY_CONSTRUCTOR_COOKIE__DESTRUCTOR))); + } + + pslIL->EmitLDLOCA(psl->GetCopyCtorChainLocalNum()); + pslIL->EmitLDLOCA(ctorCookie); + pslIL->EmitCALL(METHOD__COPY_CONSTRUCTOR_CHAIN__ADD, 2, 0); + #else pslIL->SetStubTargetArgType(ELEMENT_TYPE_I); // native type is a pointer EmitLoadNativeLocalAddrForByRefDispatch(pslILDispatch, dwNewValueTypeLocal); diff --git a/src/coreclr/vm/metasig.h b/src/coreclr/vm/metasig.h index 182acc55e643f..7c45b03a72a05 100644 --- a/src/coreclr/vm/metasig.h +++ b/src/coreclr/vm/metasig.h @@ -588,6 +588,11 @@ DEFINE_METASIG_T(SM(RefCleanupWorkListElement_RetVoid, r(C(CLEANUP_WORK_LIST_ELE DEFINE_METASIG_T(SM(RefCleanupWorkListElement_SafeHandle_RetIntPtr, r(C(CLEANUP_WORK_LIST_ELEMENT)) C(SAFE_HANDLE), I)) DEFINE_METASIG_T(SM(RefCleanupWorkListElement_Obj_RetVoid, r(C(CLEANUP_WORK_LIST_ELEMENT)) j, v)) +DEFINE_METASIG(SM(PtrVoid_RetPtrVoid, P(v), P(v))) +DEFINE_METASIG(IM(PtrVoid_RetVoid, P(v), v)) +DEFINE_METASIG_T(IM(PtrCopyConstructorCookie_RetVoid, P(g(COPY_CONSTRUCTOR_COOKIE)), v)) + + #ifdef FEATURE_ICASTABLE DEFINE_METASIG_T(SM(ICastable_RtType_RefException_RetBool, C(ICASTABLE) C(CLASS) r(C(EXCEPTION)), F)) DEFINE_METASIG_T(SM(ICastable_RtType_RetRtType, C(ICASTABLE) C(CLASS), C(CLASS))) diff --git a/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs b/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs index 6db8fbcf3378f..5e60d0712d9ce 100644 --- a/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs +++ b/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs @@ -26,25 +26,32 @@ public static int TestEntryPoint() object testInstance = Activator.CreateInstance(testType); MethodInfo testMethod = testType.GetMethod("PInvokeNumCopies"); + // On x86, we have an additional copy on every P/Invoke from the "native" parameter to the actual location on the stack. + int platformExtra = 0; + if (RuntimeInformation.ProcessArchitecture == Architecture.X86) + { + platformExtra = 1; + } + // PInvoke will copy twice. Once from argument to parameter, and once from the managed to native parameter. - Assert.Equal(2, (int)testMethod.Invoke(testInstance, null)); + Assert.Equal(2 + platformExtra, (int)testMethod.Invoke(testInstance, null)); testMethod = testType.GetMethod("ReversePInvokeNumCopies"); // Reverse PInvoke will copy 3 times. Two are from the same paths as the PInvoke, // and the third is from the reverse P/Invoke call. - Assert.Equal(3, (int)testMethod.Invoke(testInstance, null)); + Assert.Equal(3 + platformExtra, (int)testMethod.Invoke(testInstance, null)); testMethod = testType.GetMethod("PInvokeNumCopiesDerivedType"); // PInvoke will copy twice. Once from argument to parameter, and once from the managed to native parameter. - Assert.Equal(2, (int)testMethod.Invoke(testInstance, null)); + Assert.Equal(2 + platformExtra, (int)testMethod.Invoke(testInstance, null)); testMethod = testType.GetMethod("ReversePInvokeNumCopiesDerivedType"); // Reverse PInvoke will copy 3 times. Two are from the same paths as the PInvoke, // and the third is from the reverse P/Invoke call. - Assert.Equal(3, (int)testMethod.Invoke(testInstance, null)); + Assert.Equal(3 + platformExtra, (int)testMethod.Invoke(testInstance, null)); } catch (Exception ex) { From b7c043d0d863fd66ca11dbbc191fd96eaab98320 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 21 Mar 2024 11:35:19 -0700 Subject: [PATCH 04/12] PR feedback. Keep a little more info around --- src/coreclr/vm/dllimport.cpp | 27 +++++++++++++++------------ src/coreclr/vm/dllimport.h | 6 +++--- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp index e5942a356f58d..ec78987da03db 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -1631,8 +1631,8 @@ NDirectStubLinker::NDirectStubLinker( } #endif // FEATURE_COMINTEROP -#if defined(TARGET_WINDOWS) && defined(TARGET_X86) - m_dwCopyCtorChainLocalNum = -1; +#if defined(TARGET_X86) + m_dwCopyCtorChainLocalNum = (DWORD)-1; #endif } @@ -1846,7 +1846,7 @@ DWORD NDirectStubLinker::GetReturnValueLocalNum() return m_dwRetValLocalNum; } -#if defined(TARGET_WINDOWS) && defined(TARGET_X86) +#ifdef TARGET_X86 DWORD NDirectStubLinker::GetCopyCtorChainLocalNum() { STANDARD_VM_CONTRACT; @@ -2092,7 +2092,9 @@ void NDirectStubLinker::End(DWORD dwStubFlags) } } +#if defined(TARGET_WINDOWS) && defined(TARGET_X86) EXTERN_C void STDCALL CopyConstructorCallStub(void); +#endif // defined(TARGET_WINDOWS) && defined(TARGET_X86) void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, MethodDesc * pStubMD) { @@ -2177,8 +2179,9 @@ void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, Meth } } -#if defined(TARGET_WINDOWS) && defined(TARGET_X86) - if (m_dwCopyCtorChainLocalNum != -1) +#if defined(TARGET_X86) +#if defined(TARGET_WINDOWS) + if (m_dwCopyCtorChainLocalNum != (DWORD)-1) { // If we have a copy constructor chain local, we need to call the copy constructor stub // to ensure that the chain is called correctly. @@ -2190,7 +2193,10 @@ void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, Meth pcsEmit->EmitCALL(METHOD__COPY_CONSTRUCTOR_CHAIN__INSTALL, 2, 0); pcsEmit->EmitLDC((DWORD_PTR)&CopyConstructorCallStub); } -#endif // defined(TARGET_WINDOWS) && defined(TARGET_X86) +#else + _ASSERTE(m_dwCopyCtorChainLocalNum == (DWORD)-1); +#endif // defined(TARGET_WINDOWS) +#endif // defined(TARGET_X86) // For managed-to-native calls, the rest of the work is done by the JIT. It will // erect InlinedCallFrame, flip GC mode, and use the specified calling convention @@ -6148,12 +6154,9 @@ EXTERN_C void* STDCALL CallCopyConstructorsWorker(void* esp) STATIC_CONTRACT_MODE_PREEMPTIVE; // we've already switched to preemptive using ExecuteCallback = void*(STDMETHODCALLTYPE*)(void*); - ExecuteCallback pExecute; - { - GCX_COOP(); - MethodDesc* pMD = CoreLibBinder::GetMethod(METHOD__COPY_CONSTRUCTOR_CHAIN__EXECUTE_CURRENT_COPIES_AND_GET_TARGET); - pExecute = (ExecuteCallback)pMD->GetMultiCallableAddrOfCode(); - } + + MethodDesc* pMD = CoreLibBinder::GetMethod(METHOD__COPY_CONSTRUCTOR_CHAIN__EXECUTE_CURRENT_COPIES_AND_GET_TARGET); + ExecuteCallback pExecute = (ExecuteCallback)pMD->GetMultiCallableAddrOfCode(); return pExecute(esp); } diff --git a/src/coreclr/vm/dllimport.h b/src/coreclr/vm/dllimport.h index a75d1333f73c1..bfd331c2bce40 100644 --- a/src/coreclr/vm/dllimport.h +++ b/src/coreclr/vm/dllimport.h @@ -56,7 +56,7 @@ struct StubSigDesc } } #endif // _DEBUG - + #ifndef DACCESS_COMPILE void InitTypeContext(Instantiation classInst, Instantiation methodInst) { @@ -496,7 +496,7 @@ class NDirectStubLinker : public ILStubLinker DWORD GetCleanupWorkListLocalNum(); DWORD GetThreadLocalNum(); DWORD GetReturnValueLocalNum(); -#if defined(TARGET_WINDOWS) && defined(TARGET_X86) +#if defined(TARGET_X86) DWORD GetCopyCtorChainLocalNum(); #endif void SetCleanupNeeded(); @@ -568,7 +568,7 @@ class NDirectStubLinker : public ILStubLinker DWORD m_dwTargetEntryPointLocalNum; #endif // FEATURE_COMINTEROP -#if defined(TARGET_WINDOWS) && defined(TARGET_X86) +#if defined(TARGET_X86) DWORD m_dwCopyCtorChainLocalNum; #endif From fab948360368d462f0d2e24bbe1a952e7d47d371 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 21 Mar 2024 13:56:13 -0700 Subject: [PATCH 05/12] Fix CopyCtorTest --- .../PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs b/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs index 8a3c0b7ba0f98..f8161e77f2b53 100644 --- a/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs +++ b/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs @@ -19,9 +19,15 @@ public static unsafe int StructWithCtorTest(StructWithCtor* ptrStruct, ref Struc if (refStruct._instanceField != 2) return 2; - if (StructWithCtor.CopyCtorCallCount != 2) + int expectedCallCount = 2; + if (RuntimeInformation.ProcessArchitecture == Architecture.X86) + { + expectedCallCount = 3; + } + + if (StructWithCtor.CopyCtorCallCount != expectedCallCount) return 3; - if (StructWithCtor.DtorCallCount != 2) + if (StructWithCtor.DtorCallCount != expectedCallCount) return 4; From 3be291127cebc01cce6a2cc0ff3cc77fc6bc3ffb Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 21 Mar 2024 13:58:50 -0700 Subject: [PATCH 06/12] ifdef out the types in corelib.h for non-x86 --- src/coreclr/vm/corelib.h | 2 ++ src/coreclr/vm/metasig.h | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index 5411cdd9c8cf3..5b579480de8a7 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -1040,6 +1040,7 @@ DEFINE_METHOD(HANDLE_MARSHALER, CONVERT_SAFEHANDLE_TO_NATIVE,ConvertSaf DEFINE_METHOD(HANDLE_MARSHALER, THROW_SAFEHANDLE_FIELD_CHANGED, ThrowSafeHandleFieldChanged, SM_RetVoid) DEFINE_METHOD(HANDLE_MARSHALER, THROW_CRITICALHANDLE_FIELD_CHANGED, ThrowCriticalHandleFieldChanged, SM_RetVoid) +#ifdef TARGET_X86 DEFINE_CLASS(COPY_CONSTRUCTOR_CHAIN, StubHelpers, CopyConstructorChain) DEFINE_METHOD(COPY_CONSTRUCTOR_CHAIN, EXECUTE_CURRENT_COPIES_AND_GET_TARGET, ExecuteCurrentCopiesAndGetTarget, SM_PtrVoid_RetPtrVoid) DEFINE_METHOD(COPY_CONSTRUCTOR_CHAIN, INSTALL, Install, IM_PtrVoid_RetVoid) @@ -1050,6 +1051,7 @@ DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, SOURCE, m_source) DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, DESTINATION_OFFSET, m_destinationOffset) DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, COPY_CONSTRUCTOR, m_copyConstructor) DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, DESTRUCTOR, m_destructor) +#endif DEFINE_CLASS(COMVARIANT, Marshalling, ComVariant) diff --git a/src/coreclr/vm/metasig.h b/src/coreclr/vm/metasig.h index 7c45b03a72a05..35ea26e614d48 100644 --- a/src/coreclr/vm/metasig.h +++ b/src/coreclr/vm/metasig.h @@ -590,7 +590,9 @@ DEFINE_METASIG_T(SM(RefCleanupWorkListElement_Obj_RetVoid, r(C(CLEANUP_WORK_LIST DEFINE_METASIG(SM(PtrVoid_RetPtrVoid, P(v), P(v))) DEFINE_METASIG(IM(PtrVoid_RetVoid, P(v), v)) +#ifdef TARGET_X86 DEFINE_METASIG_T(IM(PtrCopyConstructorCookie_RetVoid, P(g(COPY_CONSTRUCTOR_COOKIE)), v)) +#endif // TARGET_X86 #ifdef FEATURE_ICASTABLE From db4cbe2346813b54fcde9af7bb6dfd70c5885198 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 21 Mar 2024 14:27:53 -0700 Subject: [PATCH 07/12] Copy the stub into the linux x86 assembly and remove TARGET_WINDOWS --- src/coreclr/vm/dllimport.cpp | 10 +++------- src/coreclr/vm/i386/asmhelpers.S | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp index ec78987da03db..05a509d8f0676 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -2092,9 +2092,9 @@ void NDirectStubLinker::End(DWORD dwStubFlags) } } -#if defined(TARGET_WINDOWS) && defined(TARGET_X86) +#if defined(TARGET_X86) EXTERN_C void STDCALL CopyConstructorCallStub(void); -#endif // defined(TARGET_WINDOWS) && defined(TARGET_X86) +#endif void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, MethodDesc * pStubMD) { @@ -2180,7 +2180,6 @@ void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, Meth } #if defined(TARGET_X86) -#if defined(TARGET_WINDOWS) if (m_dwCopyCtorChainLocalNum != (DWORD)-1) { // If we have a copy constructor chain local, we need to call the copy constructor stub @@ -2193,9 +2192,6 @@ void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, Meth pcsEmit->EmitCALL(METHOD__COPY_CONSTRUCTOR_CHAIN__INSTALL, 2, 0); pcsEmit->EmitLDC((DWORD_PTR)&CopyConstructorCallStub); } -#else - _ASSERTE(m_dwCopyCtorChainLocalNum == (DWORD)-1); -#endif // defined(TARGET_WINDOWS) #endif // defined(TARGET_X86) // For managed-to-native calls, the rest of the work is done by the JIT. It will @@ -6145,7 +6141,7 @@ PCODE GetILStubForCalli(VASigCookie *pVASigCookie, MethodDesc *pMD) RETURN pVASigCookie->pNDirectILStub; } -#if defined(TARGET_X86) && defined(TARGET_WINDOWS) +#if defined(TARGET_X86) // Copy constructor support for C++/CLI EXTERN_C void* STDCALL CallCopyConstructorsWorker(void* esp) { diff --git a/src/coreclr/vm/i386/asmhelpers.S b/src/coreclr/vm/i386/asmhelpers.S index ff777550bfab5..1a477bfadf292 100644 --- a/src/coreclr/vm/i386/asmhelpers.S +++ b/src/coreclr/vm/i386/asmhelpers.S @@ -562,6 +562,29 @@ LOCAL_LABEL(GoCallCalliWorker): jmp C_FUNC(GenericPInvokeCalliHelper) LEAF_END GenericPInvokeCalliHelper, _TEXT +//========================================================================== +// This is small stub whose purpose is to record current stack pointer and +// call CallCopyConstructorsWorker to invoke copy constructors and destructors +// as appropriate. This stub operates on arguments already pushed to the +// stack by JITted IL stub and must not create a new frame, i.e. it must tail +// call to the target for it to see the arguments that copy ctors have been +// called on. +// +LEAF_ENTRY CopyConstructorCallStub, _TEXT + // there may be an argument in ecx - save it + push ecx + + // push pointer to arguments + lea edx, [esp + 8] + push edx + + call _CallCopyConstructorsWorker@4 + + // restore ecx and tail call to the target + pop ecx + jmp eax +LEAF_END + #ifdef FEATURE_READYTORUN NESTED_ENTRY DynamicHelperArgsStub, _TEXT, NoHandler .cfi_def_cfa_offset 16 From 8aba7ffc90f62b1496d9e8d475e31889468a6320 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 21 Mar 2024 14:55:03 -0700 Subject: [PATCH 08/12] Just try calling the worker directly (no stdcall name mangling) --- src/coreclr/vm/i386/asmhelpers.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/i386/asmhelpers.S b/src/coreclr/vm/i386/asmhelpers.S index 1a477bfadf292..f27e1bc217bea 100644 --- a/src/coreclr/vm/i386/asmhelpers.S +++ b/src/coreclr/vm/i386/asmhelpers.S @@ -578,7 +578,7 @@ LEAF_ENTRY CopyConstructorCallStub, _TEXT lea edx, [esp + 8] push edx - call _CallCopyConstructorsWorker@4 + call C_FUNC(CallCopyConstructorsWorker) // restore ecx and tail call to the target pop ecx From 89312805b5fcbefd62d2ed22fccaa8a504ed0672 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 21 Mar 2024 16:00:46 -0700 Subject: [PATCH 09/12] Fix macro --- src/coreclr/vm/i386/asmhelpers.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/i386/asmhelpers.S b/src/coreclr/vm/i386/asmhelpers.S index f27e1bc217bea..96859a70fe21f 100644 --- a/src/coreclr/vm/i386/asmhelpers.S +++ b/src/coreclr/vm/i386/asmhelpers.S @@ -583,7 +583,7 @@ LEAF_ENTRY CopyConstructorCallStub, _TEXT // restore ecx and tail call to the target pop ecx jmp eax -LEAF_END +LEAF_END CopyConstructorCallStub, _TEXT #ifdef FEATURE_READYTORUN NESTED_ENTRY DynamicHelperArgsStub, _TEXT, NoHandler From cb49dc02979441a2c3a90af0e527fffd121023c9 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Sat, 23 Mar 2024 15:08:35 -0700 Subject: [PATCH 10/12] Narrow support to Windows only --- src/coreclr/vm/corelib.h | 4 ++-- src/coreclr/vm/dllimport.cpp | 20 ++++++++-------- src/coreclr/vm/dllimport.h | 8 +++---- src/coreclr/vm/i386/asmhelpers.S | 23 ------------------- src/coreclr/vm/ilmarshalers.cpp | 2 ++ src/coreclr/vm/ilmarshalers.h | 2 ++ src/coreclr/vm/metasig.h | 4 ++-- src/coreclr/vm/mlinfo.cpp | 9 ++++++++ src/coreclr/vm/mtypes.h | 3 +++ .../Miscellaneous/CopyCtor/CopyCtorTest.cs | 2 +- 10 files changed, 35 insertions(+), 42 deletions(-) diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index 5b579480de8a7..c42b9277defa2 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -1040,7 +1040,7 @@ DEFINE_METHOD(HANDLE_MARSHALER, CONVERT_SAFEHANDLE_TO_NATIVE,ConvertSaf DEFINE_METHOD(HANDLE_MARSHALER, THROW_SAFEHANDLE_FIELD_CHANGED, ThrowSafeHandleFieldChanged, SM_RetVoid) DEFINE_METHOD(HANDLE_MARSHALER, THROW_CRITICALHANDLE_FIELD_CHANGED, ThrowCriticalHandleFieldChanged, SM_RetVoid) -#ifdef TARGET_X86 +#if defined(TARGET_X86) && defined(TARGET_WINDOWS) DEFINE_CLASS(COPY_CONSTRUCTOR_CHAIN, StubHelpers, CopyConstructorChain) DEFINE_METHOD(COPY_CONSTRUCTOR_CHAIN, EXECUTE_CURRENT_COPIES_AND_GET_TARGET, ExecuteCurrentCopiesAndGetTarget, SM_PtrVoid_RetPtrVoid) DEFINE_METHOD(COPY_CONSTRUCTOR_CHAIN, INSTALL, Install, IM_PtrVoid_RetVoid) @@ -1051,7 +1051,7 @@ DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, SOURCE, m_source) DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, DESTINATION_OFFSET, m_destinationOffset) DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, COPY_CONSTRUCTOR, m_copyConstructor) DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, DESTRUCTOR, m_destructor) -#endif +#endif // defined(TARGET_X86) && defined(TARGET_WINDOWS) DEFINE_CLASS(COMVARIANT, Marshalling, ComVariant) diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp index 05a509d8f0676..281da3cda0499 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -1631,9 +1631,9 @@ NDirectStubLinker::NDirectStubLinker( } #endif // FEATURE_COMINTEROP -#if defined(TARGET_X86) +#if defined(TARGET_X86) && defined(TARGET_WINDOWS) m_dwCopyCtorChainLocalNum = (DWORD)-1; -#endif +#endif // defined(TARGET_X86) && defined(TARGET_WINDOWS) } void NDirectStubLinker::SetCallingConvention(CorInfoCallConvExtension unmngCallConv, BOOL fIsVarArg) @@ -1846,7 +1846,7 @@ DWORD NDirectStubLinker::GetReturnValueLocalNum() return m_dwRetValLocalNum; } -#ifdef TARGET_X86 +#if defined(TARGET_X86) && defined(TARGET_WINDOWS) DWORD NDirectStubLinker::GetCopyCtorChainLocalNum() { STANDARD_VM_CONTRACT; @@ -1861,7 +1861,7 @@ DWORD NDirectStubLinker::GetCopyCtorChainLocalNum() return m_dwCopyCtorChainLocalNum; } -#endif +#endif // defined(TARGET_X86) && defined(TARGET_WINDOWS) BOOL NDirectStubLinker::IsCleanupNeeded() { @@ -2092,9 +2092,9 @@ void NDirectStubLinker::End(DWORD dwStubFlags) } } -#if defined(TARGET_X86) +#if defined(TARGET_X86) && defined(TARGET_WINDOWS) EXTERN_C void STDCALL CopyConstructorCallStub(void); -#endif +#endif // defined(TARGET_X86) && defined(TARGET_WINDOWS) void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, MethodDesc * pStubMD) { @@ -2179,7 +2179,7 @@ void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, Meth } } -#if defined(TARGET_X86) +#if defined(TARGET_X86) && defined(TARGET_WINDOWS) if (m_dwCopyCtorChainLocalNum != (DWORD)-1) { // If we have a copy constructor chain local, we need to call the copy constructor stub @@ -2192,7 +2192,7 @@ void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, Meth pcsEmit->EmitCALL(METHOD__COPY_CONSTRUCTOR_CHAIN__INSTALL, 2, 0); pcsEmit->EmitLDC((DWORD_PTR)&CopyConstructorCallStub); } -#endif // defined(TARGET_X86) +#endif // defined(TARGET_X86) && defined(TARGET_WINDOWS) // For managed-to-native calls, the rest of the work is done by the JIT. It will // erect InlinedCallFrame, flip GC mode, and use the specified calling convention @@ -6141,7 +6141,7 @@ PCODE GetILStubForCalli(VASigCookie *pVASigCookie, MethodDesc *pMD) RETURN pVASigCookie->pNDirectILStub; } -#if defined(TARGET_X86) +#if defined(TARGET_X86) && defined(TARGET_WINDOWS) // Copy constructor support for C++/CLI EXTERN_C void* STDCALL CallCopyConstructorsWorker(void* esp) { @@ -6156,6 +6156,6 @@ EXTERN_C void* STDCALL CallCopyConstructorsWorker(void* esp) return pExecute(esp); } -#endif +#endif // defined(TARGET_X86) && defined(TARGET_WINDOWS) #endif // #ifndef DACCESS_COMPILE diff --git a/src/coreclr/vm/dllimport.h b/src/coreclr/vm/dllimport.h index bfd331c2bce40..da124927eeca2 100644 --- a/src/coreclr/vm/dllimport.h +++ b/src/coreclr/vm/dllimport.h @@ -496,9 +496,9 @@ class NDirectStubLinker : public ILStubLinker DWORD GetCleanupWorkListLocalNum(); DWORD GetThreadLocalNum(); DWORD GetReturnValueLocalNum(); -#if defined(TARGET_X86) +#if defined(TARGET_X86) && defined(TARGET_WINDOWS) DWORD GetCopyCtorChainLocalNum(); -#endif +#endif // defined(TARGET_X86) && defined(TARGET_WINDOWS) void SetCleanupNeeded(); void SetExceptionCleanupNeeded(); BOOL IsCleanupWorkListSetup(); @@ -568,9 +568,9 @@ class NDirectStubLinker : public ILStubLinker DWORD m_dwTargetEntryPointLocalNum; #endif // FEATURE_COMINTEROP -#if defined(TARGET_X86) +#if defined(TARGET_X86) && defined(TARGET_WINDOWS) DWORD m_dwCopyCtorChainLocalNum; -#endif +#endif // defined(TARGET_X86) && defined(TARGET_WINDOWS) BOOL m_fHasCleanupCode; BOOL m_fHasExceptionCleanupCode; diff --git a/src/coreclr/vm/i386/asmhelpers.S b/src/coreclr/vm/i386/asmhelpers.S index 96859a70fe21f..ff777550bfab5 100644 --- a/src/coreclr/vm/i386/asmhelpers.S +++ b/src/coreclr/vm/i386/asmhelpers.S @@ -562,29 +562,6 @@ LOCAL_LABEL(GoCallCalliWorker): jmp C_FUNC(GenericPInvokeCalliHelper) LEAF_END GenericPInvokeCalliHelper, _TEXT -//========================================================================== -// This is small stub whose purpose is to record current stack pointer and -// call CallCopyConstructorsWorker to invoke copy constructors and destructors -// as appropriate. This stub operates on arguments already pushed to the -// stack by JITted IL stub and must not create a new frame, i.e. it must tail -// call to the target for it to see the arguments that copy ctors have been -// called on. -// -LEAF_ENTRY CopyConstructorCallStub, _TEXT - // there may be an argument in ecx - save it - push ecx - - // push pointer to arguments - lea edx, [esp + 8] - push edx - - call C_FUNC(CallCopyConstructorsWorker) - - // restore ecx and tail call to the target - pop ecx - jmp eax -LEAF_END CopyConstructorCallStub, _TEXT - #ifdef FEATURE_READYTORUN NESTED_ENTRY DynamicHelperArgsStub, _TEXT, NoHandler .cfi_def_cfa_offset 16 diff --git a/src/coreclr/vm/ilmarshalers.cpp b/src/coreclr/vm/ilmarshalers.cpp index a603cd5d5fe75..e5520a22c5e53 100644 --- a/src/coreclr/vm/ilmarshalers.cpp +++ b/src/coreclr/vm/ilmarshalers.cpp @@ -3394,6 +3394,7 @@ ILCriticalHandleMarshaler::ReturnOverride( return OVERRIDDEN; } // ILCriticalHandleMarshaler::ReturnOverride +#if defined(TARGET_WINDOWS) MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOverride(NDirectStubLinker* psl, BOOL byref, BOOL fin, @@ -3519,6 +3520,7 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver return OVERRIDDEN; } } +#endif // defined(TARGET_WINDOWS) LocalDesc ILArgIteratorMarshaler::GetNativeType() { diff --git a/src/coreclr/vm/ilmarshalers.h b/src/coreclr/vm/ilmarshalers.h index 757307b500634..275783b6d504b 100644 --- a/src/coreclr/vm/ilmarshalers.h +++ b/src/coreclr/vm/ilmarshalers.h @@ -2923,6 +2923,7 @@ class ILBlittableLayoutClassMarshaler : public ILMarshaler void EmitConvertContentsNativeToCLR(ILCodeStream* pslILEmit) override; }; +#if defined(TARGET_WINDOWS) class ILBlittableValueClassWithCopyCtorMarshaler : public ILMarshaler { public: @@ -2956,6 +2957,7 @@ class ILBlittableValueClassWithCopyCtorMarshaler : public ILMarshaler }; +#endif // defined(TARGET_WINDOWS) class ILArgIteratorMarshaler : public ILMarshaler { diff --git a/src/coreclr/vm/metasig.h b/src/coreclr/vm/metasig.h index 35ea26e614d48..a17066d1f669f 100644 --- a/src/coreclr/vm/metasig.h +++ b/src/coreclr/vm/metasig.h @@ -590,9 +590,9 @@ DEFINE_METASIG_T(SM(RefCleanupWorkListElement_Obj_RetVoid, r(C(CLEANUP_WORK_LIST DEFINE_METASIG(SM(PtrVoid_RetPtrVoid, P(v), P(v))) DEFINE_METASIG(IM(PtrVoid_RetVoid, P(v), v)) -#ifdef TARGET_X86 +#if defined(TARGET_X86) && defined(TARGET_WINDOWS) DEFINE_METASIG_T(IM(PtrCopyConstructorCookie_RetVoid, P(g(COPY_CONSTRUCTOR_COOKIE)), v)) -#endif // TARGET_X86 +#endif // defined(TARGET_X86) && defined(TARGET_WINDOWS) #ifdef FEATURE_ICASTABLE diff --git a/src/coreclr/vm/mlinfo.cpp b/src/coreclr/vm/mlinfo.cpp index 39eff93cc6c24..c36f0bf58bd38 100644 --- a/src/coreclr/vm/mlinfo.cpp +++ b/src/coreclr/vm/mlinfo.cpp @@ -2383,6 +2383,7 @@ MarshalInfo::MarshalInfo(Module* pModule, { if (fNeedsCopyCtor && !IsFieldScenario()) // We don't support automatically discovering copy constructors for fields. { +#if defined(TARGET_WINDOWS) MethodDesc *pCopyCtor; MethodDesc *pDtor; FindCopyCtor(pModule, m_pMT, &pCopyCtor); @@ -2392,6 +2393,10 @@ MarshalInfo::MarshalInfo(Module* pModule, m_args.mm.m_pCopyCtor = pCopyCtor; m_args.mm.m_pDtor = pDtor; m_type = MARSHAL_TYPE_BLITTABLEVALUECLASSWITHCOPYCTOR; +#else // !defined(TARGET_WINDOWS) + m_resID = IDS_EE_BADMARSHAL_BADMANAGED; + IfFailGoto(E_FAIL, lFail); +#endif // defined(TARGET_WINDOWS) } else { @@ -3121,7 +3126,9 @@ bool MarshalInfo::IsValueClass(MarshalType mtype) { case MARSHAL_TYPE_BLITTABLEVALUECLASS: case MARSHAL_TYPE_VALUECLASS: +#if defined(TARGET_WINDOWS) case MARSHAL_TYPE_BLITTABLEVALUECLASSWITHCOPYCTOR: +#endif // defined(TARGET_WINDOWS) return true; default: @@ -3605,7 +3612,9 @@ DispParamMarshaler *MarshalInfo::GenerateDispParamMarshaler() case MARSHAL_TYPE_BLITTABLEVALUECLASS: case MARSHAL_TYPE_BLITTABLEPTR: case MARSHAL_TYPE_LAYOUTCLASSPTR: +#if defined(TARGET_WINDOWS) case MARSHAL_TYPE_BLITTABLEVALUECLASSWITHCOPYCTOR: +#endif // defined(TARGET_WINDOWS) pDispParamMarshaler = new DispParamRecordMarshaler(m_pMT); break; diff --git a/src/coreclr/vm/mtypes.h b/src/coreclr/vm/mtypes.h index e25b9ca83ab35..796487f13ec37 100644 --- a/src/coreclr/vm/mtypes.h +++ b/src/coreclr/vm/mtypes.h @@ -79,7 +79,10 @@ DEFINE_MARSHALER_TYPE(MARSHAL_TYPE_VALUECLASS, ValueClassMa DEFINE_MARSHALER_TYPE(MARSHAL_TYPE_REFERENCECUSTOMMARSHALER, ReferenceCustomMarshaler) DEFINE_MARSHALER_TYPE(MARSHAL_TYPE_ARGITERATOR, ArgIteratorMarshaler) + +#if defined(TARGET_WINDOWS) DEFINE_MARSHALER_TYPE(MARSHAL_TYPE_BLITTABLEVALUECLASSWITHCOPYCTOR, BlittableValueClassWithCopyCtorMarshaler) +#endif // defined(TARGET_WINDOWS) #ifdef FEATURE_COMINTEROP DEFINE_MARSHALER_TYPE(MARSHAL_TYPE_OBJECT, ObjectMarshaler) diff --git a/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs b/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs index f8161e77f2b53..0bc84164a1a4c 100644 --- a/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs +++ b/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs @@ -34,7 +34,7 @@ public static unsafe int StructWithCtorTest(StructWithCtor* ptrStruct, ref Struc return 100; } - [Fact] + [ConditionalFact(typeof(TestLibrary.PlatformDetection), nameof(TestLibrary.PlatformDetection.IsWindows))] [SkipOnMono("Not supported on Mono")] [ActiveIssue("https://github.com/dotnet/runtimelab/issues/155", typeof(TestLibrary.Utilities), nameof(TestLibrary.Utilities.IsNativeAot))] public static unsafe void ValidateCopyConstructorAndDestructorCalled() From d949a45a8d3906bb73cc3c642a1b504d52e6aac7 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Sat, 23 Mar 2024 17:24:48 -0700 Subject: [PATCH 11/12] Fix trimming SPCL for CopyConstructorCookie --- src/coreclr/vm/corelib.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index c42b9277defa2..c37dc5bf251e9 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -1040,7 +1040,8 @@ DEFINE_METHOD(HANDLE_MARSHALER, CONVERT_SAFEHANDLE_TO_NATIVE,ConvertSaf DEFINE_METHOD(HANDLE_MARSHALER, THROW_SAFEHANDLE_FIELD_CHANGED, ThrowSafeHandleFieldChanged, SM_RetVoid) DEFINE_METHOD(HANDLE_MARSHALER, THROW_CRITICALHANDLE_FIELD_CHANGED, ThrowCriticalHandleFieldChanged, SM_RetVoid) -#if defined(TARGET_X86) && defined(TARGET_WINDOWS) +#ifdef TARGET_WINDOWS +#ifdef TARGET_X86 DEFINE_CLASS(COPY_CONSTRUCTOR_CHAIN, StubHelpers, CopyConstructorChain) DEFINE_METHOD(COPY_CONSTRUCTOR_CHAIN, EXECUTE_CURRENT_COPIES_AND_GET_TARGET, ExecuteCurrentCopiesAndGetTarget, SM_PtrVoid_RetPtrVoid) DEFINE_METHOD(COPY_CONSTRUCTOR_CHAIN, INSTALL, Install, IM_PtrVoid_RetVoid) @@ -1051,7 +1052,8 @@ DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, SOURCE, m_source) DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, DESTINATION_OFFSET, m_destinationOffset) DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, COPY_CONSTRUCTOR, m_copyConstructor) DEFINE_FIELD(COPY_CONSTRUCTOR_COOKIE, DESTRUCTOR, m_destructor) -#endif // defined(TARGET_X86) && defined(TARGET_WINDOWS) +#endif // TARGET_X86 +#endif // TARGET_WINDOWS DEFINE_CLASS(COMVARIANT, Marshalling, ComVariant) From 4fba3136c70e48c5735313d027215434ffdf5dd1 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Sun, 24 Mar 2024 08:25:14 -0700 Subject: [PATCH 12/12] Fix test --- .../Miscellaneous/CopyCtor/CopyCtorTest.cs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs b/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs index 0bc84164a1a4c..57ec4d9632426 100644 --- a/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs +++ b/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs @@ -15,21 +15,32 @@ public static unsafe class CopyCtor public static unsafe int StructWithCtorTest(StructWithCtor* ptrStruct, ref StructWithCtor refStruct) { if (ptrStruct->_instanceField != 1) + { + Console.WriteLine($"Fail: {ptrStruct->_instanceField} != {1}"); return 1; + } if (refStruct._instanceField != 2) + { + Console.WriteLine($"Fail: {refStruct._instanceField} != {2}"); return 2; + } int expectedCallCount = 2; if (RuntimeInformation.ProcessArchitecture == Architecture.X86) { - expectedCallCount = 3; + expectedCallCount = 4; } if (StructWithCtor.CopyCtorCallCount != expectedCallCount) + { + Console.WriteLine($"Fail: {StructWithCtor.CopyCtorCallCount} != {expectedCallCount}"); return 3; + } if (StructWithCtor.DtorCallCount != expectedCallCount) + { + Console.WriteLine($"Fail: {StructWithCtor.DtorCallCount} != {expectedCallCount}"); return 4; - + } return 100; }