From e3126aa1f2c389b3ca347ab6e6520aff64ea8ccb Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 1 Aug 2024 12:27:14 -0700 Subject: [PATCH 01/30] Revert "Disable GS checks in Reverse P/Invoke stubs to avoid missing these copies." This reverts commit 79b1cd234fbb866cd34f6bc8ccad20578bf7ddc7. --- src/coreclr/vm/jitinterface.cpp | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 59fe2c563fa66..a241db64cc298 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -3743,18 +3743,8 @@ uint32_t CEEInfo::getClassAttribsInternal (CORINFO_CLASS_HANDLE clsHnd) if (pMT->IsByRefLike()) ret |= CORINFO_FLG_BYREF_LIKE; - // In Reverse P/Invoke stubs, we are generating the code - // and we are not generating the code patterns that the GS checks - // are meant to catch. - // As a result, we can skip setting this flag. - // We do this as the GS checks (emitted when this flag is set) - // can break C++/CLI's copy-constructor semantics by missing copies. - if (pClass->IsUnsafeValueClass() - && !(m_pMethodBeingCompiled->IsILStub() - && dac_cast(m_pMethodBeingCompiled)->GetILStubType() == DynamicMethodDesc::StubNativeToCLRInterop)) - { + if (pClass->IsUnsafeValueClass()) ret |= CORINFO_FLG_UNSAFE_VALUECLASS; - } } if (pClass->HasExplicitFieldOffsetLayout() && pClass->HasOverlaidField()) ret |= CORINFO_FLG_OVERLAPPING_FIELDS; From 0f5c835ea1d97faca36aea921c928019e6c10fbb Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 1 Aug 2024 12:27:40 -0700 Subject: [PATCH 02/30] Revert "Since the fix is in the CoreCLR VM side, we need to disable the tests in JitStress (as they won't be fixed when the JIT introduces these extra checks when not asking the VM)" This reverts commit 35d0de9858ae7e78cbcd9fdecd3f41257b6f2d72. --- .../IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs b/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs index b59dcb74dd521..962545f02a06f 100644 --- a/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs +++ b/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs @@ -62,7 +62,6 @@ public static int TestEntryPoint() } [Fact] - [SkipOnCoreClr("JitStress can introduce extra copies that won't happen in production scenarios.", RuntimeTestModes.JitStress)] public static void CopyConstructorsInArgumentStackSlots() { Assembly ijwNativeDll = Assembly.Load("IjwCopyConstructorMarshaler"); @@ -74,7 +73,6 @@ public static void CopyConstructorsInArgumentStackSlots() } [Fact] - [SkipOnCoreClr("JitStress can introduce extra copies that won't happen in production scenarios.", RuntimeTestModes.JitStress)] public static void CopyConstructorsInArgumentStackSlotsWithUnsafeValueType() { Assembly ijwNativeDll = Assembly.Load("IjwCopyConstructorMarshaler"); From 6ae45a8be668562a7d2a2a97c03a4e202cd1cbde Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 1 Aug 2024 16:11:36 -0700 Subject: [PATCH 03/30] Propagate the modreq to the stub and into the GS cookie logic to avoid creating shadow vars --- src/coreclr/inc/corinfo.h | 1 + src/coreclr/inc/jiteeversionguid.h | 10 +- src/coreclr/jit/compiler.h | 4 + src/coreclr/jit/lclvars.cpp | 30 ++++ .../tools/Common/JitInterface/CorInfoTypes.cs | 1 + src/coreclr/vm/ilmarshalers.cpp | 1 + src/coreclr/vm/jitinterface.cpp | 24 ++- src/coreclr/vm/mlinfo.cpp | 15 +- src/coreclr/vm/mlinfo.h | 1 + src/coreclr/vm/siginfo.cpp | 69 ++++++++- src/coreclr/vm/siginfo.hpp | 3 +- src/coreclr/vm/stubgen.cpp | 139 ++++++++++-------- src/coreclr/vm/stubgen.h | 13 ++ 13 files changed, 236 insertions(+), 75 deletions(-) diff --git a/src/coreclr/inc/corinfo.h b/src/coreclr/inc/corinfo.h index 9ae9489e18e5b..5a597667d2391 100644 --- a/src/coreclr/inc/corinfo.h +++ b/src/coreclr/inc/corinfo.h @@ -644,6 +644,7 @@ enum CorInfoTypeWithMod { CORINFO_TYPE_MASK = 0x3F, // lower 6 bits are type mask CORINFO_TYPE_MOD_PINNED = 0x40, // can be applied to CLASS, or BYREF to indicate pinned + CORINFO_TYPE_MOD_COPY_WITH_HELPER = 0x80 // can be applied to VALUECLASS to indicate 'needs helper to copy' }; inline CorInfoType strip(CorInfoTypeWithMod val) { diff --git a/src/coreclr/inc/jiteeversionguid.h b/src/coreclr/inc/jiteeversionguid.h index 770ed708bcd23..d9b15425682bb 100644 --- a/src/coreclr/inc/jiteeversionguid.h +++ b/src/coreclr/inc/jiteeversionguid.h @@ -43,11 +43,11 @@ typedef const GUID *LPCGUID; #define GUID_DEFINED #endif // !GUID_DEFINED -constexpr GUID JITEEVersionIdentifier = { /* 4f0e8e22-e2e9-4a80-b140-37bbb6e68bca */ - 0x4f0e8e22, - 0xe2e9, - 0x4a80, - {0xb1, 0x40, 0x37, 0xbb, 0xb6, 0xe6, 0x8b, 0xca} +constexpr GUID JITEEVersionIdentifier = { /* c0c10d73-f76c-4b92-8681-60b8157e17f1 */ + 0xc0c10d73, + 0xf76c, + 0x4b92, + {0x86, 0x81, 0x60, 0xb8, 0x15, 0x7e, 0x17, 0xf1} }; ////////////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 4b295f7a6aa0e..7b353e654f553 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -532,6 +532,8 @@ class LclVarDsc #endif unsigned char lvPinned : 1; // is this a pinned variable? + unsigned char lvRequiresSpecialCopy : 1; // Local requires copy helper for stack copies. + unsigned char lvMustInit : 1; // must be initialized private: @@ -11278,6 +11280,8 @@ class Compiler // - Whenever a parameter passed in an argument register needs to be spilled by LSRA, we // create a new spill temp if the method needs GS cookie check. return varDsc->lvIsParam; +#elif defined(TARGET_X86) + return varDsc->lvIsParam && !varDsc->lvIsRegArg && !varDsc->lvRequiresSpecialCopy; #else // !defined(TARGET_AMD64) return varDsc->lvIsParam && !varDsc->lvIsRegArg; #endif diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 5b1003c0b8226..b46198d8c37db 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -326,6 +326,19 @@ void Compiler::lvaInitTypeRef() } } + if ((corInfoTypeWithMod & CORINFO_TYPE_MOD_COPY_WITH_HELPER) != 0) + { + if (corInfoType == CORINFO_TYPE_VALUECLASS) + { + JITDUMP("Setting lvRequiresSpecialCopy for V%02u\n", varNum); + varDsc->lvRequiresSpecialCopy = 1; + } + else + { + JITDUMP("Ignoring special copy for non-struct V%02u\n", varNum); + } + } + varDsc->lvOnFrame = true; // The final home for this local variable might be our local stack frame if (corInfoType == CORINFO_TYPE_CLASS) @@ -652,6 +665,19 @@ void Compiler::lvaInitUserArgs(InitVarDscInfo* varDscInfo, unsigned skipArgs, un CorInfoTypeWithMod corInfoType = info.compCompHnd->getArgType(&info.compMethodInfo->args, argLst, &typeHnd); varDsc->lvIsParam = 1; + + if ((corInfoType & CORINFO_TYPE_MOD_COPY_WITH_HELPER) != 0) + { + if (strip(corInfoType) == CORINFO_TYPE_VALUECLASS) + { + JITDUMP("Setting lvRequiresSpecialCopy for user arg%02u\n", i); + varDsc->lvRequiresSpecialCopy = 1; + } + else + { + JITDUMP("Ignoring special copy for non-struct user arg%02u\n", i); + } + } lvaInitVarDsc(varDsc, varDscInfo->varNum, strip(corInfoType), typeHnd, argLst, &info.compMethodInfo->args); @@ -7592,6 +7618,10 @@ void Compiler::lvaDumpEntry(unsigned lclNum, FrameLayoutState curState, size_t r { printf(" pinned"); } + if (varDsc->lvRequiresSpecialCopy) + { + printf(" special-copy"); + } if (varDsc->lvClassHnd != NO_CLASS_HANDLE) { printf(" class-hnd"); diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs b/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs index 902fa1159ed53..4be84d0c316f8 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs @@ -783,6 +783,7 @@ public enum CorInfoTypeWithMod { CORINFO_TYPE_MASK = 0x3F, // lower 6 bits are type mask CORINFO_TYPE_MOD_PINNED = 0x40, // can be applied to CLASS, or BYREF to indicate pinned + CORINFO_TYPE_MOD_COPY_WITH_HELPER = 0x80, // can be applied to VALUECLASS to indicate 'needs helper to copy' }; public struct CORINFO_HELPER_ARG diff --git a/src/coreclr/vm/ilmarshalers.cpp b/src/coreclr/vm/ilmarshalers.cpp index 75d979076bd3a..070ab6fd91149 100644 --- a/src/coreclr/vm/ilmarshalers.cpp +++ b/src/coreclr/vm/ilmarshalers.cpp @@ -3504,6 +3504,7 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver // but on other platforms it comes by-reference #ifdef TARGET_X86 LocalDesc locDesc(pargs->mm.m_pMT); + locDesc.AddModifier(true, pslIL->GetToken(pargs->mm.m_pSigMod)); pslIL->SetStubTargetArgType(&locDesc); DWORD dwNewValueTypeLocal; diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index a241db64cc298..6cb3e36db8f94 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -9437,12 +9437,32 @@ CorInfoTypeWithMod CEEInfo::getArgType ( IfFailThrow(ptr.PeekElemType(&eType)); } + Module* pModule = GetModule(sig->scope); + + if (!IsDynamicScope(sig->scope)) + { + SigPointer sigtmp = ptr; + if (sigtmp.HasCustomModifier(pModule, "Microsoft.VisualC.NeedsCopyConstructorModifier", ELEMENT_TYPE_CMOD_REQD) || + sigtmp.HasCustomModifier(pModule, "System.Runtime.CompilerServices.IsCopyConstructed", ELEMENT_TYPE_CMOD_REQD)) + { + result = (CorInfoTypeWithMod)((int)result | CORINFO_TYPE_MOD_COPY_WITH_HELPER); + } + } + else + { + SigPointer sigtmp = ptr; + DynamicResolver* pResolver = GetDynamicResolver(sig->scope); + if (sigtmp.HasCustomModifier(pResolver, "Microsoft.VisualC.NeedsCopyConstructorModifier", ELEMENT_TYPE_CMOD_REQD) || + sigtmp.HasCustomModifier(pResolver, "System.Runtime.CompilerServices.IsCopyConstructed", ELEMENT_TYPE_CMOD_REQD)) + { + result = (CorInfoTypeWithMod)((int)result | CORINFO_TYPE_MOD_COPY_WITH_HELPER); + } + } + // Now read off the "real" element type after taking any instantiations into consideration SigTypeContext typeContext; GetTypeContext(&sig->sigInst,&typeContext); - Module* pModule = GetModule(sig->scope); - CorElementType type = ptr.PeekElemTypeClosed(pModule, &typeContext); TypeHandle typeHnd = TypeHandle(); diff --git a/src/coreclr/vm/mlinfo.cpp b/src/coreclr/vm/mlinfo.cpp index 8131e4fd3053a..6f95244f4bbcd 100644 --- a/src/coreclr/vm/mlinfo.cpp +++ b/src/coreclr/vm/mlinfo.cpp @@ -1251,7 +1251,7 @@ MarshalInfo::MarshalInfo(Module* pModule, CorNativeType nativeType = NATIVE_TYPE_DEFAULT; Assembly *pAssembly = pModule->GetAssembly(); - BOOL fNeedsCopyCtor = FALSE; + mdToken pCopyCtorModifier = mdTokenNil; m_BestFit = BestFit; m_ThrowOnUnmappableChar = ThrowOnUnmappableChar; m_ms = ms; @@ -1378,11 +1378,10 @@ MarshalInfo::MarshalInfo(Module* pModule, // Skip ET_BYREF IfFailGoto(sigtmp.GetByte(NULL), lFail); - if (sigtmp.HasCustomModifier(pModule, "Microsoft.VisualC.NeedsCopyConstructorModifier", ELEMENT_TYPE_CMOD_REQD) || - sigtmp.HasCustomModifier(pModule, "System.Runtime.CompilerServices.IsCopyConstructed", ELEMENT_TYPE_CMOD_REQD) ) + if (sigtmp.HasCustomModifier(pModule, "Microsoft.VisualC.NeedsCopyConstructorModifier", ELEMENT_TYPE_CMOD_REQD, &pCopyCtorModifier) || + sigtmp.HasCustomModifier(pModule, "System.Runtime.CompilerServices.IsCopyConstructed", ELEMENT_TYPE_CMOD_REQD, &pCopyCtorModifier) ) { mtype = ELEMENT_TYPE_VALUETYPE; - fNeedsCopyCtor = TRUE; m_byref = FALSE; } } @@ -1411,8 +1410,8 @@ MarshalInfo::MarshalInfo(Module* pModule, if (!th.IsEnum()) { // Check for Copy Constructor Modifier - if (sigtmp.HasCustomModifier(pModule, "Microsoft.VisualC.NeedsCopyConstructorModifier", ELEMENT_TYPE_CMOD_REQD) || - sigtmp.HasCustomModifier(pModule, "System.Runtime.CompilerServices.IsCopyConstructed", ELEMENT_TYPE_CMOD_REQD) ) + if (sigtmp.HasCustomModifier(pModule, "Microsoft.VisualC.NeedsCopyConstructorModifier", ELEMENT_TYPE_CMOD_REQD, &pCopyCtorModifier) || + sigtmp.HasCustomModifier(pModule, "System.Runtime.CompilerServices.IsCopyConstructed", ELEMENT_TYPE_CMOD_REQD, &pCopyCtorModifier) ) { mtype = mtype2; @@ -1420,7 +1419,6 @@ MarshalInfo::MarshalInfo(Module* pModule, // of this method we are pretending that the parameter is a value type passed by-value. IfFailGoto(sig.GetElemType(NULL), lFail); - fNeedsCopyCtor = TRUE; m_byref = FALSE; } } @@ -2381,7 +2379,7 @@ MarshalInfo::MarshalInfo(Module* pModule, } else { - if (fNeedsCopyCtor && !IsFieldScenario()) // We don't support automatically discovering copy constructors for fields. + if (pCopyCtorModifier != mdTokenNil && !IsFieldScenario()) // We don't support automatically discovering copy constructors for fields. { #if defined(FEATURE_IJW) MethodDesc *pCopyCtor; @@ -2389,6 +2387,7 @@ MarshalInfo::MarshalInfo(Module* pModule, FindCopyCtor(pModule, m_pMT, &pCopyCtor); FindDtor(pModule, m_pMT, &pDtor); + m_args.mm.m_pSigMod = ClassLoader::LoadTypeDefOrRefThrowing(pModule, pCopyCtorModifier).AsMethodTable(); m_args.mm.m_pMT = m_pMT; m_args.mm.m_pCopyCtor = pCopyCtor; m_args.mm.m_pDtor = pDtor; diff --git a/src/coreclr/vm/mlinfo.h b/src/coreclr/vm/mlinfo.h index 93a0f554d30af..9f23ba99954bc 100644 --- a/src/coreclr/vm/mlinfo.h +++ b/src/coreclr/vm/mlinfo.h @@ -87,6 +87,7 @@ struct OverrideProcArgs struct { + MethodTable* m_pSigMod; MethodTable* m_pMT; MethodDesc* m_pCopyCtor; MethodDesc* m_pDtor; diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index facb809cd4841..3a7cfadd196fa 100644 --- a/src/coreclr/vm/siginfo.cpp +++ b/src/coreclr/vm/siginfo.cpp @@ -937,6 +937,25 @@ IsTypeRefOrDef( return(true); } // IsTypeRefOrDef +BOOL IsTypeRefOrDef( + LPCSTR szClassName, + DynamicResolver * pResolver, + mdToken token) +{ + STANDARD_VM_CONTRACT; + + ResolvedToken resolved; + pResolver->ResolveToken(token, &resolved); + + if (resolved.TypeHandle.IsNull()) + return false; + + DefineFullyQualifiedNameForClassOnStack(); + LPCUTF8 fullyQualifiedName = GetFullyQualifiedNameForClass(resolved.TypeHandle.GetMethodTable()); + + return (strcmp(szClassName, fullyQualifiedName) == 0); +} + TypeHandle SigPointer::GetTypeHandleNT(Module* pModule, const SigTypeContext *pTypeContext) const { @@ -2282,7 +2301,7 @@ BOOL SigPointer::IsClassHelper(Module* pModule, LPCUTF8 szClassName, const SigTy //------------------------------------------------------------------------ // Tests for the existence of a custom modifier //------------------------------------------------------------------------ -BOOL SigPointer::HasCustomModifier(Module *pModule, LPCSTR szModName, CorElementType cmodtype) const +BOOL SigPointer::HasCustomModifier(Module *pModule, LPCSTR szModName, CorElementType cmodtype, mdToken* pModifierType) const { CONTRACTL { @@ -2318,6 +2337,54 @@ BOOL SigPointer::HasCustomModifier(Module *pModule, LPCSTR szModName, CorElement if (etyp == cmodtype && IsTypeRefOrDef(szModName, pModule, tk)) { + if (pModifierType != nullptr) + { + *pModifierType = tk; + } + return(TRUE); + } + + if (FAILED(sp.GetByte(&data))) + return FALSE; + + etyp = (CorElementType)data; + + + } + return(FALSE); +} + +BOOL SigPointer::HasCustomModifier(DynamicResolver *pResolver, LPCSTR szModName, CorElementType cmodtype, mdToken* pModifierType) const +{ + STANDARD_VM_CONTRACT; + + BAD_FORMAT_NOTHROW_ASSERT(cmodtype == ELEMENT_TYPE_CMOD_OPT || cmodtype == ELEMENT_TYPE_CMOD_REQD); + + SigPointer sp = *this; + CorElementType etyp; + if (sp.AtSentinel()) + sp.m_ptr++; + + BYTE data; + + if (FAILED(sp.GetByte(&data))) + return FALSE; + + etyp = (CorElementType)data; + + + while (etyp == ELEMENT_TYPE_CMOD_OPT || etyp == ELEMENT_TYPE_CMOD_REQD) { + + mdToken tk; + if (FAILED(sp.GetToken(&tk))) + return FALSE; + + if (etyp == cmodtype && IsTypeRefOrDef(szModName, pResolver, tk)) + { + if (pModifierType != nullptr) + { + *pModifierType = tk; + } return(TRUE); } diff --git a/src/coreclr/vm/siginfo.hpp b/src/coreclr/vm/siginfo.hpp index fab9a79260d2d..82d2029c63593 100644 --- a/src/coreclr/vm/siginfo.hpp +++ b/src/coreclr/vm/siginfo.hpp @@ -277,7 +277,8 @@ class SigPointer : public SigParser //------------------------------------------------------------------------ // Tests for the existence of a custom modifier //------------------------------------------------------------------------ - BOOL HasCustomModifier(Module *pModule, LPCSTR szModName, CorElementType cmodtype) const; + BOOL HasCustomModifier(Module *pModule, LPCSTR szModName, CorElementType cmodtype, mdToken* pModifierType = NULL) const; + BOOL HasCustomModifier(DynamicResolver *pResolver, LPCSTR szModName, CorElementType cmodtype, mdToken* pModifierType = NULL) const; //------------------------------------------------------------------------ // Tests for ELEMENT_TYPE_CLASS or ELEMENT_TYPE_VALUETYPE followed by a TypeDef, diff --git a/src/coreclr/vm/stubgen.cpp b/src/coreclr/vm/stubgen.cpp index 283b6d3fa2d88..4164da672a1c6 100644 --- a/src/coreclr/vm/stubgen.cpp +++ b/src/coreclr/vm/stubgen.cpp @@ -2601,78 +2601,101 @@ CorCallingConvention ILStubLinker::GetStubTargetCallingConv() return m_nativeFnSigBuilder.GetCallingConv(); } -void ILStubLinker::TransformArgForJIT(LocalDesc *pLoc) +namespace { - STANDARD_VM_CONTRACT; - // Turn everything into blittable primitives. The reason this method is needed are - // byrefs which are OK only when they ref stack data or are pinned. This condition - // cannot be verified by code:NDirect.MarshalingRequired so we explicitly get rid - // of them here. - switch (pLoc->ElementType[0]) - { - // primitives - case ELEMENT_TYPE_VOID: - case ELEMENT_TYPE_BOOLEAN: - case ELEMENT_TYPE_CHAR: - case ELEMENT_TYPE_I1: - case ELEMENT_TYPE_U1: - case ELEMENT_TYPE_I2: - case ELEMENT_TYPE_U2: - case ELEMENT_TYPE_I4: - case ELEMENT_TYPE_U4: - case ELEMENT_TYPE_I8: - case ELEMENT_TYPE_U8: - case ELEMENT_TYPE_R4: - case ELEMENT_TYPE_R8: - case ELEMENT_TYPE_I: - case ELEMENT_TYPE_U: + void TransformArgSignatureForJIT(BYTE sig[], size_t* cbSig, TypeHandle internalToken) + { + STANDARD_VM_CONTRACT; + _ASSERTE(*cbSig > 0); + // Turn everything into blittable primitives. The reason this method is needed are + // byrefs which are OK only when they ref stack data or are pinned. This condition + // cannot be verified by code:NDirect.MarshalingRequired so we explicitly get rid + // of them here. + switch (sig[0]) { - // no transformation needed - break; - } + // primitives + case ELEMENT_TYPE_VOID: + case ELEMENT_TYPE_BOOLEAN: + case ELEMENT_TYPE_CHAR: + case ELEMENT_TYPE_I1: + case ELEMENT_TYPE_U1: + case ELEMENT_TYPE_I2: + case ELEMENT_TYPE_U2: + case ELEMENT_TYPE_I4: + case ELEMENT_TYPE_U4: + case ELEMENT_TYPE_I8: + case ELEMENT_TYPE_U8: + case ELEMENT_TYPE_R4: + case ELEMENT_TYPE_R8: + case ELEMENT_TYPE_I: + case ELEMENT_TYPE_U: + { + // no transformation needed + break; + } - case ELEMENT_TYPE_VALUETYPE: - { - _ASSERTE(!"Should have been replaced by a native value type!"); - break; - } + case ELEMENT_TYPE_VALUETYPE: + { + _ASSERTE(!"Should have been replaced by a native value type!"); + break; + } - case ELEMENT_TYPE_PTR: - { - // Don't transform pointer types to ELEMENT_TYPE_I. The JIT can handle the correct type information, - // and it's required for some cases (such as SwiftError*). - break; - } + case ELEMENT_TYPE_PTR: + { + // Don't transform pointer types to ELEMENT_TYPE_I. The JIT can handle the correct type information, + // and it's required for some cases (such as SwiftError*). + break; + } - case ELEMENT_TYPE_BYREF: - { - // Transform ELEMENT_TYPE_BYREF to ELEMENT_TYPE_PTR to retain the pointed-to type information - // while making the type blittable. - pLoc->ElementType[0] = ELEMENT_TYPE_PTR; - break; - } + case ELEMENT_TYPE_BYREF: + { + // Transform ELEMENT_TYPE_BYREF to ELEMENT_TYPE_PTR to retain the pointed-to type information + // while making the type blittable. + sig[0] = ELEMENT_TYPE_PTR; + break; + } - case ELEMENT_TYPE_INTERNAL: - { - // JIT will handle structures - if (pLoc->InternalToken.IsValueType()) + case ELEMENT_TYPE_CMOD_OPT: + case ELEMENT_TYPE_CMOD_REQD: { - _ASSERTE(pLoc->InternalToken.IsNativeValueType() || !pLoc->InternalToken.GetMethodTable()->ContainsGCPointers()); + // Keep the custom modifier, + // but scan the rest of the signature after the modifier token. + + size_t cbModOpt = CorSigUncompressedDataSize(&sig[1]); + size_t cbRemainingSig = *cbSig - 1 - cbModOpt; + TransformArgSignatureForJIT(&sig[1 + cbModOpt], &cbRemainingSig, internalToken); + *cbSig = 1 + cbModOpt + cbRemainingSig; break; } - FALLTHROUGH; - } - // ref types -> ELEMENT_TYPE_I - default: - { - pLoc->ElementType[0] = ELEMENT_TYPE_I; - pLoc->cbType = 1; - break; + case ELEMENT_TYPE_INTERNAL: + { + // JIT will handle structures + if (internalToken.IsValueType()) + { + _ASSERTE(internalToken.IsNativeValueType() || !internalToken.GetMethodTable()->ContainsGCPointers()); + break; + } + FALLTHROUGH; + } + + // ref types -> ELEMENT_TYPE_I + default: + { + sig[0] = ELEMENT_TYPE_I; + *cbSig = 1; + break; + } } } } +void ILStubLinker::TransformArgForJIT(LocalDesc *pLoc) +{ + STANDARD_VM_CONTRACT; + return TransformArgSignatureForJIT(pLoc->ElementType, &pLoc->cbType, pLoc->InternalToken); +} + Module *ILStubLinker::GetStubSigModule() { LIMITED_METHOD_CONTRACT; diff --git a/src/coreclr/vm/stubgen.h b/src/coreclr/vm/stubgen.h index 968e5f9b48291..6847c3d72423b 100644 --- a/src/coreclr/vm/stubgen.h +++ b/src/coreclr/vm/stubgen.h @@ -96,6 +96,19 @@ struct LocalDesc ChangeType(ELEMENT_TYPE_PTR); } + void AddModifier(bool required, mdToken token) + { + LIMITED_METHOD_CONTRACT; + BYTE compressed[4]; + ULONG cbCompressed; + cbCompressed = CorSigCompressToken(token, compressed); + _ASSERTE(cbCompressed + cbType + 1 <= MAX_LOCALDESC_ELEMENTS); + memmove(&ElementType[cbCompressed], ElementType, cbType); + memmove(&ElementType, compressed, cbCompressed); + cbType += cbCompressed; + ChangeType(required ? ELEMENT_TYPE_CMOD_REQD : ELEMENT_TYPE_CMOD_OPT); + } + void ChangeType(CorElementType elemType) { LIMITED_METHOD_CONTRACT; From dc15fb2e9f3db332181f7a9e02f9e454547c4471 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 2 Aug 2024 11:49:44 -0700 Subject: [PATCH 04/30] Add RuntimeHelpers copy-constructor helper --- .../RuntimeHelpers.CoreCLR.cs | 7 ++ src/coreclr/inc/corinfo.h | 1 + src/coreclr/inc/jithelpers.h | 2 + .../Common/JitInterface/CorInfoHelpFunc.cs | 1 + src/coreclr/vm/corelib.h | 1 + src/coreclr/vm/jithelpers.cpp | 23 +++++ src/coreclr/vm/jitinterface.cpp | 94 +++++++++++++++++++ src/coreclr/vm/mlinfo.cpp | 10 +- src/coreclr/vm/mlinfo.h | 13 +++ 9 files changed, 147 insertions(+), 5 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs index e155b109886d0..20829c8c77405 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs @@ -344,6 +344,13 @@ internal static int EnumCompareTo(T x, T y) where T : struct, Enum return x.CompareTo(y); } + + // The body of this function will be created by the EE for the specific type. + // See getILIntrinsicImplementation for how this happens. + [Intrinsic] + [MethodImpl(MethodImplOptions.InternalCall)] + internal static extern void CopyConstruct(T* dest, T* src) where T : unmanaged; + internal static ref byte GetRawData(this object obj) => ref Unsafe.As(obj).Data; diff --git a/src/coreclr/inc/corinfo.h b/src/coreclr/inc/corinfo.h index 5a597667d2391..8cfa28ed95380 100644 --- a/src/coreclr/inc/corinfo.h +++ b/src/coreclr/inc/corinfo.h @@ -509,6 +509,7 @@ enum CorInfoHelpFunc CORINFO_HELP_MEMCPY, // Copy block of memory CORINFO_HELP_NATIVE_MEMSET, // Init block of memory using native memset (not safe for pDst being null, // not safe for unbounded size, does not trigger GC) + CORINFO_HELP_GET_SPECIAL_STRUCT_COPY, // special copy helper for structs CORINFO_HELP_RUNTIMEHANDLE_METHOD, // determine a type/field/method handle at run-time CORINFO_HELP_RUNTIMEHANDLE_METHOD_LOG, // determine a type/field/method handle at run-time, with IBC logging diff --git a/src/coreclr/inc/jithelpers.h b/src/coreclr/inc/jithelpers.h index 4401a38098921..604187f8aff02 100644 --- a/src/coreclr/inc/jithelpers.h +++ b/src/coreclr/inc/jithelpers.h @@ -221,6 +221,8 @@ DYNAMICJITHELPER(CORINFO_HELP_MEMZERO, NULL, METHOD__SPAN_HELPERS__MEMZERO) DYNAMICJITHELPER(CORINFO_HELP_MEMCPY, NULL, METHOD__SPAN_HELPERS__MEMCOPY) JITHELPER(CORINFO_HELP_NATIVE_MEMSET, Jit_NativeMemSet, METHOD__NIL) + JITHELPER(CORINFO_HELP_NATIVE_MEMSET, Jit_NativeMemSet, METHOD__NIL) + JITHELPER(CORINFO_HELP_GET_SPECIAL_STRUCT_COPY, JIT_GetSpecialStructCopyHelper, METHOD__NIL) // Generics JITHELPER(CORINFO_HELP_RUNTIMEHANDLE_METHOD, JIT_GenericHandleMethod, METHOD__NIL) diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs b/src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs index 7d2074575e07d..434b24bde4bb5 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs @@ -199,6 +199,7 @@ which is the right helper to use to allocate an object of a given type. */ CORINFO_HELP_MEMCPY, // Copy block of memory CORINFO_HELP_NATIVE_MEMSET, // Init block of memory using native memset (not safe for pDst being null, // not safe for unbounded size, does not trigger GC) + CORINFO_HELP_GET_SPECIAL_STRUCT_COPY, // special copy helper for structs CORINFO_HELP_RUNTIMEHANDLE_METHOD, // determine a type/field/method handle at run-time CORINFO_HELP_RUNTIMEHANDLE_METHOD_LOG, // determine a type/field/method handle at run-time, with IBC logging diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index 057057a0a1d97..845addcb763dc 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -636,6 +636,7 @@ DEFINE_METHOD(RUNTIME_HELPERS, ENUM_COMPARE_TO, EnumCompareTo, NoSig DEFINE_METHOD(RUNTIME_HELPERS, ALLOC_TAILCALL_ARG_BUFFER, AllocTailCallArgBuffer, SM_Int_IntPtr_RetIntPtr) DEFINE_METHOD(RUNTIME_HELPERS, GET_TAILCALL_INFO, GetTailCallInfo, NoSig) DEFINE_METHOD(RUNTIME_HELPERS, DISPATCH_TAILCALLS, DispatchTailCalls, NoSig) +DEFINE_METHOD(RUNTIME_HELPERS, COPY_CONSTRUCT, CopyConstruct, NoSig) DEFINE_CLASS(SPAN_HELPERS, System, SpanHelpers) DEFINE_METHOD(SPAN_HELPERS, MEMSET, Fill, SM_RefByte_Byte_UIntPtr_RetVoid) diff --git a/src/coreclr/vm/jithelpers.cpp b/src/coreclr/vm/jithelpers.cpp index b365309257dbc..3f38e4b159b7a 100644 --- a/src/coreclr/vm/jithelpers.cpp +++ b/src/coreclr/vm/jithelpers.cpp @@ -2438,6 +2438,29 @@ HCIMPL3(void, Jit_NativeMemSet, void* pDest, int value, size_t length) } HCIMPLEND +HCIMPL1(PCODE, JIT_GetSpecialStructCopyHelper, CORINFO_CLASS_HANDLE cls) +{ + FCALL_CONTRACT; + + TypeHandle type{cls}; + + _ASSERTE(type.IsValueType()); + + MethodDesc* pHelperMD = CoreLibBinder::GetMethod(METHOD__RUNTIME_HELPERS__COPY_CONSTRUCT); + + pHelperMD = MethodDesc::FindOrCreateAssociatedMethodDesc( + pHelperMD, + pHelperMD->GetMethodTable(), + FALSE, + Instantiation(&type, 1), + FALSE, + FALSE); + + pHelperMD->CheckRestore(); + return pHelperMD->GetMultiCallableAddrOfCode(); +} +HCIMPLEND + HCIMPL1(Object*, JIT_GetRuntimeFieldStub, CORINFO_FIELD_HANDLE field) { FCALL_CONTRACT; diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 6cb3e36db8f94..a9176d5073d9f 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -7549,6 +7549,80 @@ class MethodInfoHelperContext final } }; +namespace +{ + bool GenerateCopyConstructorHelper(MethodDesc* ftn, TypeHandle type, MethodInfoHelperContext& cxt, CORINFO_METHOD_INFO* methInfo) + { + if (!type.IsValueType()) + return false; + + MethodTable * pMT = type.AsMethodTable(); + + MethodDesc* pCopyCtor = nullptr; + IJWHelpers::FindCopyConstructor(pMT->GetModule(), pMT, &pCopyCtor); + + MethodDesc* pDestructor = nullptr; + IJWHelpers::FindDestructor(pMT->GetModule(), pMT, &pDestructor); + + NewHolder ilResolver = new ILStubResolver(); + ilResolver->SetStubMethodDesc(ftn); + + SigTypeContext genericContext; + SigTypeContext::InitTypeContext(ftn, &genericContext); + + ILStubLinker sl( + ftn->GetModule(), + ftn->GetSignature(), + &genericContext, + ftn, + ILSTUB_LINKER_FLAG_NONE); + + ILCodeStream* pCode = sl.NewCodeStream(ILStubLinker::kDispatch); + + pCode->EmitLDARG(0); + pCode->EmitLDARG(1); + if (pCopyCtor != nullptr) + { + pCode->EmitCALL(pCode->GetToken(pCopyCtor), 2, 0); + } + else + { + pCode->EmitLDC(type.GetSize()); + pCode->EmitCPBLK(); + } + + if (pDestructor != nullptr) + { + pCode->EmitLDARG(1); + pCode->EmitCALL(pCode->GetToken(pDestructor), 1, 0); + } + + // Generate all IL associated data for JIT + { + UINT maxStack; + size_t cbCode = sl.Link(&maxStack); + DWORD cbSig = sl.GetLocalSigSize(); + + COR_ILMETHOD_DECODER* pILHeader = ilResolver->AllocGeneratedIL(cbCode, cbSig, maxStack); + BYTE* pbBuffer = (BYTE*)pILHeader->Code; + BYTE* pbLocalSig = (BYTE*)pILHeader->LocalVarSig; + _ASSERTE(cbSig == pILHeader->cbLocalVarSig); + sl.GenerateCode(pbBuffer, cbCode); + sl.GetLocalSig(pbLocalSig, cbSig); + + // Store the token lookup map + ilResolver->SetTokenLookupMap(sl.GetTokenLookupMap()); + ilResolver->SetJitFlags(CORJIT_FLAGS(CORJIT_FLAGS::CORJIT_FLAG_IL_STUB)); + + cxt.TransientResolver = (DynamicResolver*)ilResolver; + cxt.Header = pILHeader; + } + + ilResolver.SuppressRelease(); + return true; + } +} + static void getMethodInfoHelper( MethodInfoHelperContext& cxt, CORINFO_METHOD_INFO* methInfo, @@ -7623,6 +7697,26 @@ static void getMethodInfoHelper( SigPointer localSig = pResolver->GetLocalSig(); localSig.GetSignature(&pLocalSig, &cbLocalSig); } + else if (ftn->GetMemberDef() == CoreLibBinder::GetMethod(METHOD__RUNTIME_HELPERS__COPY_CONSTRUCT)->GetMemberDef()) + { + _ASSERTE(ftn->HasMethodInstantiation()); + Instantiation inst = ftn->GetMethodInstantiation(); + + _ASSERTE(inst.GetNumArgs() == 1); + TypeHandle type = inst[0]; + + if (!GenerateCopyConstructorHelper(ftn, type, cxt, methInfo)) + { + ThrowHR(COR_E_BADIMAGEFORMAT); + } + + scopeHnd = cxt.CreateScopeHandle(); + + _ASSERTE(cxt.Header != NULL); + getMethodInfoILMethodHeaderHelper(cxt.Header, methInfo); + pLocalSig = cxt.Header->LocalVarSig; + cbLocalSig = cxt.Header->cbLocalVarSig; + } else { if (!ftn->TryGenerateUnsafeAccessor(&cxt.TransientResolver, &cxt.Header)) diff --git a/src/coreclr/vm/mlinfo.cpp b/src/coreclr/vm/mlinfo.cpp index 6f95244f4bbcd..14bb59efee8a9 100644 --- a/src/coreclr/vm/mlinfo.cpp +++ b/src/coreclr/vm/mlinfo.cpp @@ -46,12 +46,12 @@ #define INITIAL_NUM_CMINFO_HASHTABLE_BUCKETS 32 #define DEBUG_CONTEXT_STR_LEN 2000 -namespace +namespace IJWHelpers { //------------------------------------------------------------------------------------- // Return the copy ctor for a VC class (if any exists) //------------------------------------------------------------------------------------- - void FindCopyCtor(Module *pModule, MethodTable *pMT, MethodDesc **pMDOut) + void FindCopyConstructor(Module *pModule, MethodTable *pMT, MethodDesc **pMDOut) { CONTRACTL { @@ -249,7 +249,7 @@ namespace //------------------------------------------------------------------------------------- // Return the destructor for a VC class (if any exists) //------------------------------------------------------------------------------------- - void FindDtor(Module *pModule, MethodTable *pMT, MethodDesc **pMDOut) + void FindDestructor(Module *pModule, MethodTable *pMT, MethodDesc **pMDOut) { CONTRACTL { @@ -2384,8 +2384,8 @@ MarshalInfo::MarshalInfo(Module* pModule, #if defined(FEATURE_IJW) MethodDesc *pCopyCtor; MethodDesc *pDtor; - FindCopyCtor(pModule, m_pMT, &pCopyCtor); - FindDtor(pModule, m_pMT, &pDtor); + IJWHelpers::FindCopyConstructor(pModule, m_pMT, &pCopyCtor); + IJWHelpers::FindDestructor(pModule, m_pMT, &pDtor); m_args.mm.m_pSigMod = ClassLoader::LoadTypeDefOrRefThrowing(pModule, pCopyCtorModifier).AsMethodTable(); m_args.mm.m_pMT = m_pMT; diff --git a/src/coreclr/vm/mlinfo.h b/src/coreclr/vm/mlinfo.h index 9f23ba99954bc..54d8fbc0fa2b1 100644 --- a/src/coreclr/vm/mlinfo.h +++ b/src/coreclr/vm/mlinfo.h @@ -123,6 +123,19 @@ typedef MarshalerOverrideStatus (*RETURNOVERRIDEPROC)(NDirectStubLinker* psl, OverrideProcArgs* pargs, UINT* pResID); +namespace IJWHelpers +{ + //------------------------------------------------------------------------------------- + // Return the copy ctor for a VC class (if any exists) + //------------------------------------------------------------------------------------- + void FindCopyConstructor(Module *pModule, MethodTable *pMT, MethodDesc **pMDOut); + + //------------------------------------------------------------------------------------- + // Return the destructor for a VC class (if any exists) + //------------------------------------------------------------------------------------- + void FindDestructor(Module *pModule, MethodTable *pMT, MethodDesc **pMDOut); +} + //========================================================================== // This structure contains the native type information for a given // parameter. From bf92d9e83016670268ec29c75da0001b2ce65f8e Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 2 Aug 2024 13:52:04 -0700 Subject: [PATCH 05/30] Implement Reverse P/Invoke GS shadow copy for copy constructors --- .../RuntimeHelpers.CoreCLR.cs | 3 +- src/coreclr/jit/compiler.h | 2 - src/coreclr/jit/gschecks.cpp | 42 +++++++++++++++---- src/coreclr/vm/jithelpers.cpp | 10 +++-- src/coreclr/vm/jitinterface.cpp | 2 + 5 files changed, 44 insertions(+), 15 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs index 20829c8c77405..4389d9507a4c4 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs @@ -348,8 +348,7 @@ internal static int EnumCompareTo(T x, T y) where T : struct, Enum // The body of this function will be created by the EE for the specific type. // See getILIntrinsicImplementation for how this happens. [Intrinsic] - [MethodImpl(MethodImplOptions.InternalCall)] - internal static extern void CopyConstruct(T* dest, T* src) where T : unmanaged; + internal static extern unsafe void CopyConstruct(T* dest, T* src) where T : unmanaged; internal static ref byte GetRawData(this object obj) => ref Unsafe.As(obj).Data; diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 7b353e654f553..e12f1232ef0c9 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -11280,8 +11280,6 @@ class Compiler // - Whenever a parameter passed in an argument register needs to be spilled by LSRA, we // create a new spill temp if the method needs GS cookie check. return varDsc->lvIsParam; -#elif defined(TARGET_X86) - return varDsc->lvIsParam && !varDsc->lvIsRegArg && !varDsc->lvRequiresSpecialCopy; #else // !defined(TARGET_AMD64) return varDsc->lvIsParam && !varDsc->lvIsRegArg; #endif diff --git a/src/coreclr/jit/gschecks.cpp b/src/coreclr/jit/gschecks.cpp index 0baa5ad289c24..0d10a974bf155 100644 --- a/src/coreclr/jit/gschecks.cpp +++ b/src/coreclr/jit/gschecks.cpp @@ -516,13 +516,41 @@ void Compiler::gsParamsToShadows() continue; } - GenTree* src = gtNewLclvNode(lclNum, varDsc->TypeGet()); - src->gtFlags |= GTF_DONT_CSE; - GenTree* store = gtNewStoreLclVarNode(shadowVarNum, src); - - fgEnsureFirstBBisScratch(); - compCurBB = fgFirstBB; // Needed by some morphing - (void)fgNewStmtAtBeg(fgFirstBB, fgMorphTree(store)); + if (varDsc->lvRequiresSpecialCopy) + { + GenTree* src = gtNewLclVarAddrNode(lclNum); + GenTree* dst = gtNewLclVarAddrNode(shadowVarNum); + GenTree* type = gtNewIconHandleNode(size_t(varDsc->GetLayout()->GetClassHandle()), GTF_ICON_CLASS_HDL); + GenTree* getHelper = gtNewHelperCallNode(CORINFO_HELP_GET_SPECIAL_STRUCT_COPY, TYP_I_IMPL, type); + GenTreeCall* call = gtNewIndCallNode(getHelper, TYP_VOID); + call->gtArgs.PushBack(this, NewCallArg::Primitive(dst)); + call->gtArgs.PushBack(this, NewCallArg::Primitive(src)); + + fgEnsureFirstBBisScratch(); + compCurBB = fgFirstBB; // Needed by some morphing + if (opts.IsReversePInvoke()) + { + // If we are in a reverse P/Invoke, then we need to insert + // the call at the end of the first block as we need to do the GC transition + // before we can call the helper. + (void)fgNewStmtAtEnd(fgFirstBB, fgMorphTree(call)); + } + else + { + (void)fgNewStmtAtBeg(fgFirstBB, fgMorphTree(call)); + } + } + else + { + GenTree* src = gtNewLclvNode(lclNum, varDsc->TypeGet()); + src->gtFlags |= GTF_DONT_CSE; + + GenTree* store = gtNewStoreLclVarNode(shadowVarNum, src); + + fgEnsureFirstBBisScratch(); + compCurBB = fgFirstBB; // Needed by some morphing + (void)fgNewStmtAtBeg(fgFirstBB, fgMorphTree(store)); + } } compCurBB = nullptr; diff --git a/src/coreclr/vm/jithelpers.cpp b/src/coreclr/vm/jithelpers.cpp index 3f38e4b159b7a..7fbf6ed8cc292 100644 --- a/src/coreclr/vm/jithelpers.cpp +++ b/src/coreclr/vm/jithelpers.cpp @@ -2438,12 +2438,14 @@ HCIMPL3(void, Jit_NativeMemSet, void* pDest, int value, size_t length) } HCIMPLEND -HCIMPL1(PCODE, JIT_GetSpecialStructCopyHelper, CORINFO_CLASS_HANDLE cls) +// This helper may be called in the prolog or after a GC transition, so we cannot make +// assumptions about GC mode. +HCIMPL1_RAW(PCODE, JIT_GetSpecialStructCopyHelper, CORINFO_CLASS_HANDLE cls) { - FCALL_CONTRACT; - TypeHandle type{cls}; + GCX_COOP(); + _ASSERTE(type.IsValueType()); MethodDesc* pHelperMD = CoreLibBinder::GetMethod(METHOD__RUNTIME_HELPERS__COPY_CONSTRUCT); @@ -2459,7 +2461,7 @@ HCIMPL1(PCODE, JIT_GetSpecialStructCopyHelper, CORINFO_CLASS_HANDLE cls) pHelperMD->CheckRestore(); return pHelperMD->GetMultiCallableAddrOfCode(); } -HCIMPLEND +HCIMPLEND_RAW HCIMPL1(Object*, JIT_GetRuntimeFieldStub, CORINFO_FIELD_HANDLE field) { diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index a9176d5073d9f..a4e76baea2f53 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -7597,6 +7597,8 @@ namespace pCode->EmitCALL(pCode->GetToken(pDestructor), 1, 0); } + pCode->EmitRET(); + // Generate all IL associated data for JIT { UINT maxStack; From 110b29b0e492c7a7994d80c2586b97f0c7ac57e2 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 2 Aug 2024 14:28:57 -0700 Subject: [PATCH 06/30] Use a jitinterface function instead of a jit helper so we don't need to do the generic method instantiation on every call (only when jitting) --- src/coreclr/inc/corinfo.h | 3 +- src/coreclr/inc/icorjitinfoimpl_generated.h | 3 ++ src/coreclr/inc/jiteeversionguid.h | 10 +++--- src/coreclr/inc/jithelpers.h | 2 -- src/coreclr/jit/ICorJitInfo_names_generated.h | 1 + .../jit/ICorJitInfo_wrapper_generated.hpp | 9 +++++ src/coreclr/jit/gschecks.cpp | 10 ++++-- .../Common/JitInterface/CorInfoHelpFunc.cs | 1 - .../tools/Common/JitInterface/CorInfoImpl.cs | 5 +++ .../JitInterface/CorInfoImpl_generated.cs | 18 +++++++++- .../ThunkGenerator/ThunkInput.txt | 1 + .../aot/jitinterface/jitinterface_generated.h | 10 ++++++ .../tools/superpmi/superpmi-shared/lwmlist.h | 1 + .../superpmi-shared/methodcontext.cpp | 27 +++++++++++++++ .../superpmi/superpmi-shared/methodcontext.h | 4 +++ .../superpmi-shim-collector/icorjitinfo.cpp | 8 +++++ .../icorjitinfo_generated.cpp | 7 ++++ .../icorjitinfo_generated.cpp | 6 ++++ src/coreclr/vm/jithelpers.cpp | 25 -------------- src/coreclr/vm/jitinterface.cpp | 34 +++++++++++++++++++ 20 files changed, 147 insertions(+), 38 deletions(-) diff --git a/src/coreclr/inc/corinfo.h b/src/coreclr/inc/corinfo.h index 8cfa28ed95380..534d153c807c4 100644 --- a/src/coreclr/inc/corinfo.h +++ b/src/coreclr/inc/corinfo.h @@ -509,7 +509,6 @@ enum CorInfoHelpFunc CORINFO_HELP_MEMCPY, // Copy block of memory CORINFO_HELP_NATIVE_MEMSET, // Init block of memory using native memset (not safe for pDst being null, // not safe for unbounded size, does not trigger GC) - CORINFO_HELP_GET_SPECIAL_STRUCT_COPY, // special copy helper for structs CORINFO_HELP_RUNTIMEHANDLE_METHOD, // determine a type/field/method handle at run-time CORINFO_HELP_RUNTIMEHANDLE_METHOD_LOG, // determine a type/field/method handle at run-time, with IBC logging @@ -3323,6 +3322,8 @@ class ICorDynamicInfo : public ICorStaticInfo // but for tailcalls, the contract is that JIT leaves the indirection cell in // a register during tailcall. virtual void updateEntryPointForTailCall(CORINFO_CONST_LOOKUP* entryPoint) = 0; + + virtual CORINFO_METHOD_HANDLE GetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) = 0; }; /**********************************************************************************/ diff --git a/src/coreclr/inc/icorjitinfoimpl_generated.h b/src/coreclr/inc/icorjitinfoimpl_generated.h index a8d1923f1971d..7ef00a4afb3c5 100644 --- a/src/coreclr/inc/icorjitinfoimpl_generated.h +++ b/src/coreclr/inc/icorjitinfoimpl_generated.h @@ -741,6 +741,9 @@ uint32_t getJitFlags( CORJIT_FLAGS* flags, uint32_t sizeInBytes) override; +CORINFO_METHOD_HANDLE GetSpecialCopyHelper( + CORINFO_CLASS_HANDLE type) override; + /**********************************************************************************/ // clang-format on /**********************************************************************************/ diff --git a/src/coreclr/inc/jiteeversionguid.h b/src/coreclr/inc/jiteeversionguid.h index d9b15425682bb..fc1917037043b 100644 --- a/src/coreclr/inc/jiteeversionguid.h +++ b/src/coreclr/inc/jiteeversionguid.h @@ -43,11 +43,11 @@ typedef const GUID *LPCGUID; #define GUID_DEFINED #endif // !GUID_DEFINED -constexpr GUID JITEEVersionIdentifier = { /* c0c10d73-f76c-4b92-8681-60b8157e17f1 */ - 0xc0c10d73, - 0xf76c, - 0x4b92, - {0x86, 0x81, 0x60, 0xb8, 0x15, 0x7e, 0x17, 0xf1} +constexpr GUID JITEEVersionIdentifier = { /* 62865a69-7c84-4ba5-8636-a7dec55c05a7 */ + 0x62865a69, + 0x7c84, + 0x4ba5, + {0x86, 0x36, 0xa7, 0xde, 0xc5, 0x5c, 0x05, 0xa7} }; ////////////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/src/coreclr/inc/jithelpers.h b/src/coreclr/inc/jithelpers.h index 604187f8aff02..4401a38098921 100644 --- a/src/coreclr/inc/jithelpers.h +++ b/src/coreclr/inc/jithelpers.h @@ -221,8 +221,6 @@ DYNAMICJITHELPER(CORINFO_HELP_MEMZERO, NULL, METHOD__SPAN_HELPERS__MEMZERO) DYNAMICJITHELPER(CORINFO_HELP_MEMCPY, NULL, METHOD__SPAN_HELPERS__MEMCOPY) JITHELPER(CORINFO_HELP_NATIVE_MEMSET, Jit_NativeMemSet, METHOD__NIL) - JITHELPER(CORINFO_HELP_NATIVE_MEMSET, Jit_NativeMemSet, METHOD__NIL) - JITHELPER(CORINFO_HELP_GET_SPECIAL_STRUCT_COPY, JIT_GetSpecialStructCopyHelper, METHOD__NIL) // Generics JITHELPER(CORINFO_HELP_RUNTIMEHANDLE_METHOD, JIT_GenericHandleMethod, METHOD__NIL) diff --git a/src/coreclr/jit/ICorJitInfo_names_generated.h b/src/coreclr/jit/ICorJitInfo_names_generated.h index 4779f13a029b8..07a1b4af5f5de 100644 --- a/src/coreclr/jit/ICorJitInfo_names_generated.h +++ b/src/coreclr/jit/ICorJitInfo_names_generated.h @@ -180,5 +180,6 @@ DEF_CLR_API(recordRelocation) DEF_CLR_API(getRelocTypeHint) DEF_CLR_API(getExpectedTargetArchitecture) DEF_CLR_API(getJitFlags) +DEF_CLR_API(GetSpecialCopyHelper) #undef DEF_CLR_API diff --git a/src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp b/src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp index 9c81be10f41f7..dae8350b553b6 100644 --- a/src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp +++ b/src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp @@ -1741,6 +1741,15 @@ uint32_t WrapICorJitInfo::getJitFlags( return temp; } +CORINFO_METHOD_HANDLE WrapICorJitInfo::GetSpecialCopyHelper( + CORINFO_CLASS_HANDLE type) +{ + API_ENTER(GetSpecialCopyHelper); + CORINFO_METHOD_HANDLE temp = wrapHnd->GetSpecialCopyHelper(type); + API_LEAVE(GetSpecialCopyHelper); + return temp; +} + /**********************************************************************************/ // clang-format on /**********************************************************************************/ diff --git a/src/coreclr/jit/gschecks.cpp b/src/coreclr/jit/gschecks.cpp index 0d10a974bf155..e2aa54721bb33 100644 --- a/src/coreclr/jit/gschecks.cpp +++ b/src/coreclr/jit/gschecks.cpp @@ -518,11 +518,13 @@ void Compiler::gsParamsToShadows() if (varDsc->lvRequiresSpecialCopy) { + JITDUMP("Var V%02u requires special copy, using special copy helper to copy to shadow var V%02u\n", lclNum, shadowVarNum); + CORINFO_METHOD_HANDLE copyHelper = info.compCompHnd->GetSpecialCopyHelper(varDsc->GetLayout()->GetClassHandle()); + GenTreeCall* call = gtNewCallNode(CT_USER_FUNC, copyHelper, TYP_VOID); + GenTree* src = gtNewLclVarAddrNode(lclNum); GenTree* dst = gtNewLclVarAddrNode(shadowVarNum); - GenTree* type = gtNewIconHandleNode(size_t(varDsc->GetLayout()->GetClassHandle()), GTF_ICON_CLASS_HDL); - GenTree* getHelper = gtNewHelperCallNode(CORINFO_HELP_GET_SPECIAL_STRUCT_COPY, TYP_I_IMPL, type); - GenTreeCall* call = gtNewIndCallNode(getHelper, TYP_VOID); + call->gtArgs.PushBack(this, NewCallArg::Primitive(dst)); call->gtArgs.PushBack(this, NewCallArg::Primitive(src)); @@ -530,6 +532,7 @@ void Compiler::gsParamsToShadows() compCurBB = fgFirstBB; // Needed by some morphing if (opts.IsReversePInvoke()) { + JITDUMP("Inserting special copy helper call at the end of the first block after Reverse P/Invoke transition\n"); // If we are in a reverse P/Invoke, then we need to insert // the call at the end of the first block as we need to do the GC transition // before we can call the helper. @@ -537,6 +540,7 @@ void Compiler::gsParamsToShadows() } else { + JITDUMP("Inserting special copy helper call at the beginning of the first block\n"); (void)fgNewStmtAtBeg(fgFirstBB, fgMorphTree(call)); } } diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs b/src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs index 434b24bde4bb5..7d2074575e07d 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs @@ -199,7 +199,6 @@ which is the right helper to use to allocate an object of a given type. */ CORINFO_HELP_MEMCPY, // Copy block of memory CORINFO_HELP_NATIVE_MEMSET, // Init block of memory using native memset (not safe for pDst being null, // not safe for unbounded size, does not trigger GC) - CORINFO_HELP_GET_SPECIAL_STRUCT_COPY, // special copy helper for structs CORINFO_HELP_RUNTIMEHANDLE_METHOD, // determine a type/field/method handle at run-time CORINFO_HELP_RUNTIMEHANDLE_METHOD_LOG, // determine a type/field/method handle at run-time, with IBC logging diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs index d0ff9b5e6450b..48490a22cdd53 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs @@ -4455,5 +4455,10 @@ private static bool TryReadRvaFieldData(FieldDesc field, byte* buffer, int buffe } return false; } + + private CORINFO_METHOD_STRUCT_* GetSpecialCopyHelper(CORINFO_CLASS_STRUCT_* type) + { + throw new NotImplementedException("GetSpecialCopyHelper"); + } } } diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs b/src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs index f81cae4e251e4..2ffc5d666cd84 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs @@ -2605,10 +2605,25 @@ private static uint _getJitFlags(IntPtr thisHandle, IntPtr* ppException, CORJIT_ } } + [UnmanagedCallersOnly] + private static CORINFO_METHOD_STRUCT_* _GetSpecialCopyHelper(IntPtr thisHandle, IntPtr* ppException, CORINFO_CLASS_STRUCT_* type) + { + var _this = GetThis(thisHandle); + try + { + return _this.GetSpecialCopyHelper(type); + } + catch (Exception ex) + { + *ppException = _this.AllocException(ex); + return default; + } + } + private static IntPtr GetUnmanagedCallbacks() { - void** callbacks = (void**)Marshal.AllocCoTaskMem(sizeof(IntPtr) * 176); + void** callbacks = (void**)Marshal.AllocCoTaskMem(sizeof(IntPtr) * 177); callbacks[0] = (delegate* unmanaged)&_isIntrinsic; callbacks[1] = (delegate* unmanaged)&_notifyMethodInfoUsage; @@ -2786,6 +2801,7 @@ private static IntPtr GetUnmanagedCallbacks() callbacks[173] = (delegate* unmanaged)&_getRelocTypeHint; callbacks[174] = (delegate* unmanaged)&_getExpectedTargetArchitecture; callbacks[175] = (delegate* unmanaged)&_getJitFlags; + callbacks[176] = (delegate* unmanaged)&_GetSpecialCopyHelper; return (IntPtr)callbacks; } diff --git a/src/coreclr/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txt b/src/coreclr/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txt index 1b39a55d10d46..3246d7c65b88f 100644 --- a/src/coreclr/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txt +++ b/src/coreclr/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txt @@ -340,3 +340,4 @@ FUNCTIONS uint16_t getRelocTypeHint(void* target) uint32_t getExpectedTargetArchitecture() uint32_t getJitFlags(CORJIT_FLAGS* flags, uint32_t sizeInBytes) + CORINFO_METHOD_HANDLE GetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) = 0; diff --git a/src/coreclr/tools/aot/jitinterface/jitinterface_generated.h b/src/coreclr/tools/aot/jitinterface/jitinterface_generated.h index a79c45df3588e..96e869cb86fd2 100644 --- a/src/coreclr/tools/aot/jitinterface/jitinterface_generated.h +++ b/src/coreclr/tools/aot/jitinterface/jitinterface_generated.h @@ -187,6 +187,7 @@ struct JitInterfaceCallbacks uint16_t (* getRelocTypeHint)(void * thisHandle, CorInfoExceptionClass** ppException, void* target); uint32_t (* getExpectedTargetArchitecture)(void * thisHandle, CorInfoExceptionClass** ppException); uint32_t (* getJitFlags)(void * thisHandle, CorInfoExceptionClass** ppException, CORJIT_FLAGS* flags, uint32_t sizeInBytes); + CORINFO_METHOD_HANDLE (* GetSpecialCopyHelper)(void * thisHandle, CorInfoExceptionClass** ppException, CORINFO_CLASS_HANDLE type); }; @@ -1918,4 +1919,13 @@ class JitInterfaceWrapper : public ICorJitInfo if (pException != nullptr) throw pException; return temp; } + + virtual CORINFO_METHOD_HANDLE GetSpecialCopyHelper( + CORINFO_CLASS_HANDLE type) +{ + CorInfoExceptionClass* pException = nullptr; + CORINFO_METHOD_HANDLE temp = _callbacks->GetSpecialCopyHelper(_thisHandle, &pException, type); + if (pException != nullptr) throw pException; + return temp; +} }; diff --git a/src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h b/src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h index bb2c78da30854..e00f86d1b6458 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h +++ b/src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h @@ -131,6 +131,7 @@ LWM(GetSwiftLowering, DWORDLONG, Agnostic_GetSwiftLowering) LWM(GetFpStructLowering, DWORDLONG, Agnostic_GetFpStructLowering) LWM(GetTailCallHelpers, Agnostic_GetTailCallHelpers, Agnostic_CORINFO_TAILCALL_HELPERS) LWM(UpdateEntryPointForTailCall, Agnostic_CORINFO_CONST_LOOKUP, Agnostic_CORINFO_CONST_LOOKUP) +LWM(GetSpecialCopyHelper, DWORDLONG, DWORDLONG) LWM(GetThreadTLSIndex, DWORD, DLD) LWM(GetTokenTypeAsHandle, GetTokenTypeAsHandleValue, DWORDLONG) LWM(GetTypeForBox, DWORDLONG, DWORDLONG) diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp index 4901a4535de5f..9f9497f166feb 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp @@ -7187,6 +7187,33 @@ const WCHAR* MethodContext::repGetStringConfigValue(const WCHAR* name) return value; } +void MethodContext::recGetSpecialCopyHelper(CORINFO_CLASS_HANDLE type, CORINFO_METHOD_HANDLE helper) +{ + if (GetSpecialCopyHelper == nullptr) + GetSpecialCopyHelper = new LightWeightMap(); + + DWORDLONG key; + ZeroMemory(&key, sizeof(key)); // Zero key including any struct padding + key = CastHandle(type); + + DWORDLONG value = CastHandle(helper); + GetSpecialCopyHelper->Add(key, value); + DEBUG_REC(dmpGetSpecialCopyHelper(key, value)); +} + +void MethodContext::dmpGetSpecialCopyHelper(DWORDLONG key, DWORDLONG value) +{ + printf("GetSpecialCopyHelper key %016" PRIX64 ", value %016" PRIX64 "", key, value); +} + +CORINFO_METHOD_HANDLE MethodContext::repGetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) +{ + DWORDLONG key = CastHandle(type); + DWORDLONG value = LookupByKeyOrMiss(GetSpecialCopyHelper, key, ": key %016" PRIX64 "", key); + DEBUG_REP(dmpGetSpecialCopyHelper(key, value)); + return (CORINFO_METHOD_HANDLE)value; +} + void MethodContext::dmpSigInstHandleMap(DWORD key, DWORDLONG value) { printf("SigInstHandleMap key %u, value %016" PRIX64 "", key, value); diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h index 8088f80298618..7fad15034425f 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h @@ -896,6 +896,10 @@ class MethodContext void dmpGetStringConfigValue(DWORD nameIndex, DWORD result); const WCHAR* repGetStringConfigValue(const WCHAR* name); + void recGetSpecialCopyHelper(CORINFO_CLASS_HANDLE type, CORINFO_METHOD_HANDLE helper); + void dmpGetSpecialCopyHelper(DWORDLONG key, DWORDLONG value); + CORINFO_METHOD_HANDLE repGetSpecialCopyHelper(CORINFO_CLASS_HANDLE type); + void dmpSigInstHandleMap(DWORD key, DWORDLONG value); struct Environment diff --git a/src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp b/src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp index 257d35f3b3faa..d82bf7f853170 100644 --- a/src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp @@ -2027,3 +2027,11 @@ bool interceptor_ICJI::notifyInstructionSetUsage(CORINFO_InstructionSet instruct { return original_ICorJitInfo->notifyInstructionSetUsage(instructionSet, supported); } + +CORINFO_METHOD_HANDLE interceptor_ICJI::GetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) +{ + mc->cr->AddCall("GetSpecialCopyHelper"); + CORINFO_METHOD_HANDLE temp = original_ICorJitInfo->GetSpecialCopyHelper(type); + mc->recGetSpecialCopyHelper(type, temp); + return temp; +} \ No newline at end of file diff --git a/src/coreclr/tools/superpmi/superpmi-shim-counter/icorjitinfo_generated.cpp b/src/coreclr/tools/superpmi/superpmi-shim-counter/icorjitinfo_generated.cpp index 8ee5f6783f937..33147cb85fef7 100644 --- a/src/coreclr/tools/superpmi/superpmi-shim-counter/icorjitinfo_generated.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shim-counter/icorjitinfo_generated.cpp @@ -1436,3 +1436,10 @@ uint32_t interceptor_ICJI::getJitFlags( return original_ICorJitInfo->getJitFlags(flags, sizeInBytes); } +CORINFO_METHOD_HANDLE interceptor_ICJI::GetSpecialCopyHelper( + CORINFO_CLASS_HANDLE type) +{ + mcs->AddCall("GetSpecialCopyHelper"); + return original_ICorJitInfo->GetSpecialCopyHelper(type); +} + diff --git a/src/coreclr/tools/superpmi/superpmi-shim-simple/icorjitinfo_generated.cpp b/src/coreclr/tools/superpmi/superpmi-shim-simple/icorjitinfo_generated.cpp index adb90f842194a..4269f5d5b6e5b 100644 --- a/src/coreclr/tools/superpmi/superpmi-shim-simple/icorjitinfo_generated.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shim-simple/icorjitinfo_generated.cpp @@ -1260,3 +1260,9 @@ uint32_t interceptor_ICJI::getJitFlags( return original_ICorJitInfo->getJitFlags(flags, sizeInBytes); } +CORINFO_METHOD_HANDLE interceptor_ICJI::GetSpecialCopyHelper( + CORINFO_CLASS_HANDLE type) +{ + return original_ICorJitInfo->GetSpecialCopyHelper(type); +} + diff --git a/src/coreclr/vm/jithelpers.cpp b/src/coreclr/vm/jithelpers.cpp index 7fbf6ed8cc292..b365309257dbc 100644 --- a/src/coreclr/vm/jithelpers.cpp +++ b/src/coreclr/vm/jithelpers.cpp @@ -2438,31 +2438,6 @@ HCIMPL3(void, Jit_NativeMemSet, void* pDest, int value, size_t length) } HCIMPLEND -// This helper may be called in the prolog or after a GC transition, so we cannot make -// assumptions about GC mode. -HCIMPL1_RAW(PCODE, JIT_GetSpecialStructCopyHelper, CORINFO_CLASS_HANDLE cls) -{ - TypeHandle type{cls}; - - GCX_COOP(); - - _ASSERTE(type.IsValueType()); - - MethodDesc* pHelperMD = CoreLibBinder::GetMethod(METHOD__RUNTIME_HELPERS__COPY_CONSTRUCT); - - pHelperMD = MethodDesc::FindOrCreateAssociatedMethodDesc( - pHelperMD, - pHelperMD->GetMethodTable(), - FALSE, - Instantiation(&type, 1), - FALSE, - FALSE); - - pHelperMD->CheckRestore(); - return pHelperMD->GetMultiCallableAddrOfCode(); -} -HCIMPLEND_RAW - HCIMPL1(Object*, JIT_GetRuntimeFieldStub, CORINFO_FIELD_HANDLE field) { FCALL_CONTRACT; diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index a4e76baea2f53..57373f3a43537 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -10048,6 +10048,40 @@ void CEEInfo::getAddressOfPInvokeTarget(CORINFO_METHOD_HANDLE method, EE_TO_JIT_TRANSITION(); } +CORINFO_METHOD_HANDLE CEEInfo::GetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) +{ + CONTRACTL { + THROWS; + GC_TRIGGERS; + MODE_PREEMPTIVE; + } CONTRACTL_END; + + CORINFO_METHOD_HANDLE result = NULL; + + JIT_TO_EE_TRANSITION(); + + TypeHandle th = TypeHandle(type); + + _ASSERTE(th.IsValueType()); + + MethodDesc* pHelperMD = CoreLibBinder::GetMethod(METHOD__RUNTIME_HELPERS__COPY_CONSTRUCT); + + pHelperMD = MethodDesc::FindOrCreateAssociatedMethodDesc( + pHelperMD, + pHelperMD->GetMethodTable(), + FALSE, + Instantiation(&th, 1), + FALSE, + FALSE); + + pHelperMD->CheckRestore(); + result = (CORINFO_METHOD_HANDLE)pHelperMD; + + EE_TO_JIT_TRANSITION(); + + return result; +} + /*********************************************************************/ CORINFO_JUST_MY_CODE_HANDLE CEEInfo::getJustMyCodeHandle( CORINFO_METHOD_HANDLE method, From b2bb8fc0249a4cd39d55101b7c20ec1d36b9da0a Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 2 Aug 2024 15:09:09 -0700 Subject: [PATCH 07/30] Remove extra P/Invoke copy constructor call --- src/coreclr/vm/ilmarshalers.cpp | 52 ++++++------------- .../CopyConstructorMarshaler.cs | 8 +-- 2 files changed, 20 insertions(+), 40 deletions(-) diff --git a/src/coreclr/vm/ilmarshalers.cpp b/src/coreclr/vm/ilmarshalers.cpp index 070ab6fd91149..bf34b87f09d13 100644 --- a/src/coreclr/vm/ilmarshalers.cpp +++ b/src/coreclr/vm/ilmarshalers.cpp @@ -3424,49 +3424,31 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver if (fManagedToNative) { - // 1) create new native value type local - // 2) run new->CopyCtor(old) - // 3) run old->Dtor() +#ifdef TARGET_X86 + LocalDesc locDesc(pargs->mm.m_pMT); + pslIL->SetStubTargetArgType(&locDesc); // native type is the value type - LocalDesc locDesc(pargs->mm.m_pMT); + locDesc.MakeByRef(); + locDesc.MakePinned(); - DWORD dwNewValueTypeLocal; + DWORD dwPinnedArgLocal; // Step 1 - dwNewValueTypeLocal = pslIL->NewLocal(locDesc); + dwPinnedArgLocal = pslIL->NewLocal(locDesc); - // Step 2 - if (pargs->mm.m_pCopyCtor) - { - // Managed copy constructor has signature of CopyCtor(T* new, T old); - pslIL->EmitLDLOCA(dwNewValueTypeLocal); - pslIL->EmitLDARG(argidx); - pslIL->EmitCALL(pslIL->GetToken(pargs->mm.m_pCopyCtor), 2, 0); - } - else - { - pslIL->EmitLDARG(argidx); - pslIL->EmitLDOBJ(pslIL->GetToken(pargs->mm.m_pMT)); - pslIL->EmitSTLOC(dwNewValueTypeLocal); - } + pslIL->EmitLDARG(argidx); + pslIL->EmitSTLOC(dwPinnedArgLocal); - // Step 3 - if (pargs->mm.m_pDtor) - { - // Managed destructor has signature of Destructor(T old); - pslIL->EmitLDARG(argidx); - pslIL->EmitCALL(pslIL->GetToken(pargs->mm.m_pDtor), 1, 0); - } -#ifdef TARGET_X86 - pslIL->SetStubTargetArgType(&locDesc); // native type is the value type - pslILDispatch->EmitLDLOC(dwNewValueTypeLocal); // we load the local directly + pslILDispatch->EmitLDLOC(dwPinnedArgLocal); + pslILDispatch->EmitLDOBJ(pslIL->GetToken(pargs->mm.m_pMT)); // 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->EmitLDLOC(dwPinnedArgLocal); + pslIL->EmitCONV_U(); pslIL->EmitSTFLD(pslIL->GetToken(CoreLibBinder::GetField(FIELD__COPY_CONSTRUCTOR_COOKIE__SOURCE))); pslIL->EmitLDLOCA(ctorCookie); pslIL->EmitLDC(nativeStackOffset); @@ -3489,10 +3471,10 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver 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); + pslIL->SetStubTargetArgType(ELEMENT_TYPE_U); // native type is a pointer + pslILDispatch->EmitLDARG(argidx); + pslILEmit->EmitCONV_U(); #endif return OVERRIDDEN; @@ -3507,8 +3489,6 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver locDesc.AddModifier(true, pslIL->GetToken(pargs->mm.m_pSigMod)); pslIL->SetStubTargetArgType(&locDesc); - DWORD dwNewValueTypeLocal; - dwNewValueTypeLocal = pslIL->NewLocal(locDesc); pslILDispatch->EmitLDARGA(argidx); #else LocalDesc locDesc(pargs->mm.m_pMT); diff --git a/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs b/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs index 962545f02a06f..d7ae3c0bd3445 100644 --- a/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs +++ b/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs @@ -34,24 +34,24 @@ public static int TestEntryPoint() } // PInvoke will copy twice. Once from argument to parameter, and once from the managed to native parameter. - Assert.Equal(2 + platformExtra, (int)testMethod.Invoke(testInstance, null)); + Assert.Equal(1 + 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 + platformExtra, (int)testMethod.Invoke(testInstance, null)); + Assert.Equal(2 + 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 + platformExtra, (int)testMethod.Invoke(testInstance, null)); + Assert.Equal(1 + 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 + platformExtra, (int)testMethod.Invoke(testInstance, null)); + Assert.Equal(2 + platformExtra, (int)testMethod.Invoke(testInstance, null)); } catch (Exception ex) { From 037ad6aad97e631f10c6c52937e0e29572abdacc Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 2 Aug 2024 16:09:03 -0700 Subject: [PATCH 08/30] Propagate the "special-copy" modifier into the JIT for byref locals (which the P/Invoke scenario will use) --- src/coreclr/jit/lclvars.cpp | 4 ++-- src/coreclr/vm/ilmarshalers.cpp | 20 ++++++++++++++++++-- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index b46198d8c37db..701617906bc18 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -328,14 +328,14 @@ void Compiler::lvaInitTypeRef() if ((corInfoTypeWithMod & CORINFO_TYPE_MOD_COPY_WITH_HELPER) != 0) { - if (corInfoType == CORINFO_TYPE_VALUECLASS) + if (corInfoType == CORINFO_TYPE_VALUECLASS || corInfoType == CORINFO_TYPE_BYREF) { JITDUMP("Setting lvRequiresSpecialCopy for V%02u\n", varNum); varDsc->lvRequiresSpecialCopy = 1; } else { - JITDUMP("Ignoring special copy for non-struct V%02u\n", varNum); + JITDUMP("Ignoring special copy for non-struct, non-byref V%02u\n", varNum); } } diff --git a/src/coreclr/vm/ilmarshalers.cpp b/src/coreclr/vm/ilmarshalers.cpp index bf34b87f09d13..66deb428babcc 100644 --- a/src/coreclr/vm/ilmarshalers.cpp +++ b/src/coreclr/vm/ilmarshalers.cpp @@ -3425,10 +3425,12 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver if (fManagedToNative) { #ifdef TARGET_X86 - LocalDesc locDesc(pargs->mm.m_pMT); + LocalDesc locDesc(pargs->mm.m_pMT); pslIL->SetStubTargetArgType(&locDesc); // native type is the value type locDesc.MakeByRef(); + locDesc.AddModifier(true, pslIL->GetToken(pargs->mm.m_pSigMod)); + locDesc.MakePinned(); DWORD dwPinnedArgLocal; @@ -3472,8 +3474,22 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver pslIL->EmitLDLOCA(ctorCookie); pslIL->EmitCALL(METHOD__COPY_CONSTRUCTOR_CHAIN__ADD, 2, 0); #else + LocalDesc locDesc(pargs->mm.m_pMT); + locDesc.AddModifier(true, pslIL->GetToken(pargs->mm.m_pSigMod)); + + locDesc.MakeByRef(); + locDesc.MakePinned(); + + DWORD dwPinnedArgLocal; + + // Step 1 + dwPinnedArgLocal = pslIL->NewLocal(locDesc); + + pslIL->EmitLDARG(argidx); + pslIL->EmitSTLOC(dwPinnedArgLocal); + pslIL->SetStubTargetArgType(ELEMENT_TYPE_U); // native type is a pointer - pslILDispatch->EmitLDARG(argidx); + pslILDispatch->EmitLDLOC(dwPinnedArgLocal); pslILEmit->EmitCONV_U(); #endif From c8fa45d6d86f33536a65c4547338099a7dcf8391 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 5 Aug 2024 15:06:20 -0700 Subject: [PATCH 09/30] Move copy constructor handling into the JIT for arguments --- src/coreclr/jit/gentree.cpp | 41 +++++++++++-- src/coreclr/jit/lower.cpp | 58 +++++++++++++++++++ src/coreclr/jit/lower.h | 1 + src/coreclr/vm/ilmarshalers.cpp | 30 ---------- .../CopyConstructorMarshaler.cs | 17 +++--- 5 files changed, 104 insertions(+), 43 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index dee719745f498..0beca9ef9e87a 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16604,6 +16604,16 @@ GenTree* Compiler::gtNewTempStore( valTyp = lvaGetRealType(val->AsLclVar()->GetLclNum()); val->gtType = valTyp; } + + if (val->OperGet() == GT_BLK && val->AsBlk()->Addr()->OperGet() == GT_LCL_VAR) + { + LclVarDsc* srcVarDsc = &lvaTable[val->AsBlk()->Addr()->AsLclVar()->GetLclNum()]; + if (srcVarDsc->lvRequiresSpecialCopy) + { + varDsc->lvRequiresSpecialCopy = true; + } + } + var_types dstTyp = varDsc->TypeGet(); /* If the variable's lvType is not yet set then set it here */ @@ -16668,14 +16678,35 @@ GenTree* Compiler::gtNewTempStore( compFloatingPointUsed = true; } - GenTree* store = gtNewStoreLclVarNode(tmp, val); + GenTree* store; + if (varDsc->lvRequiresSpecialCopy) + { + JITDUMP("Var V%02u requires special copy\n", tmp); + CORINFO_METHOD_HANDLE copyHelper = info.compCompHnd->GetSpecialCopyHelper(varDsc->GetLayout()->GetClassHandle()); + GenTreeCall* call = gtNewCallNode(CT_USER_FUNC, copyHelper, TYP_VOID); + + GenTree* src; - // TODO-ASG: delete this zero-diff quirk. Requires some forward substitution work. - store->gtType = dstTyp; + assert(val->OperIs(GT_BLK)); + src = val->AsBlk()->Addr(); - if (varTypeIsStruct(varDsc) && !val->IsInitVal()) + GenTree* dst = gtNewLclVarAddrNode(tmp); + + call->gtArgs.PushBack(this, NewCallArg::Primitive(dst)); + call->gtArgs.PushBack(this, NewCallArg::Primitive(src)); + store = call; + } + else { - store = impStoreStruct(store, curLevel, pAfterStmt, di, block); + store = gtNewStoreLclVarNode(tmp, val); + + // TODO-ASG: delete this zero-diff quirk. Requires some forward substitution work. + store->gtType = dstTyp; + + if (varTypeIsStruct(varDsc) && !val->IsInitVal()) + { + store = impStoreStruct(store, curLevel, pAfterStmt, di, block); + } } return store; diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 3fd18d3e8ae38..102082f9fbe27 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -1523,6 +1523,58 @@ void Lowering::ReplaceArgWithPutArgOrBitcast(GenTree** argSlot, GenTree* putArgO BlockRange().InsertAfter(arg, putArgOrBitcast); } +void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, GenTreeLclVar* lclVar) +{ + assert(putArgStk != nullptr); + assert(lclVar != nullptr); + + GenTree* dest; + +#if FEATURE_FIXED_OUT_ARGS + dest = comp->gtNewOperNode(GT_ADD, TYP_I_IMPL, comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL), comp->gtNewIconNode(putArgStk->getArgOffset())); +#else + dest = comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL); +#endif + + GenTree* src = comp->gtNewLclAddrNode(lclVar->GetLclNum(), lclVar->GetLclOffs(), TYP_I_IMPL); + + GenTree* destPlaceholder = comp->gtNewZeroConNode(dest->TypeGet()); + GenTree* srcPlaceholder = comp->gtNewZeroConNode(genActualType(src)); + + GenTreeCall* call = comp->gtNewCallNode(CT_USER_FUNC, comp->info.compCompHnd->GetSpecialCopyHelper(lclVar->GetLayout(comp)->GetClassHandle()), TYP_VOID); + + call->gtArgs.PushBack(comp, NewCallArg::Primitive(destPlaceholder)); + call->gtArgs.PushBack(comp, NewCallArg::Primitive(srcPlaceholder)); + + comp->fgMorphArgs(call); + + LIR::Range callRange = LIR::SeqTree(comp, call); + GenTree* callRangeStart = callRange.FirstNode(); + GenTree* callRangeEnd = callRange.LastNode(); + + BlockRange().InsertAfter(putArgStk, std::move(callRange)); + BlockRange().InsertAfter(putArgStk, dest); + BlockRange().InsertAfter(putArgStk, src); + + LIR::Use destUse; + LIR::Use srcUse; + BlockRange().TryGetUse(destPlaceholder, &destUse); + BlockRange().TryGetUse(srcPlaceholder, &srcUse); + destUse.ReplaceWith(dest); + srcUse.ReplaceWith(src); + destPlaceholder->SetUnusedValue(); + srcPlaceholder->SetUnusedValue(); + + LowerRange(callRangeStart, callRangeEnd); + + // Finally move all GT_PUTARG_* nodes + // Re-use the existing logic for CFG call args here + MoveCFGCallArgs(call); + + BlockRange().Remove(destPlaceholder); + BlockRange().Remove(srcPlaceholder); +} + //------------------------------------------------------------------------ // NewPutArg: rewrites the tree to put an arg in a register or on the stack. // @@ -1879,6 +1931,12 @@ void Lowering::LowerArg(GenTreeCall* call, CallArg* callArg, bool late) { ReplaceArgWithPutArgOrBitcast(ppArg, putArg); } + + if (putArg->OperIsPutArgStk() + && arg->OperIs(GT_LCL_VAR) && comp->lvaTable[arg->AsLclVar()->GetLclNum()].lvRequiresSpecialCopy) + { + InsertSpecialCopyArg(putArg->AsPutArgStk(), arg->AsLclVar()); + } } arg = *ppArg; diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index bff33917f28da..5a7214822d53b 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -183,6 +183,7 @@ class Lowering final : public Phase GenTree* LowerVirtualStubCall(GenTreeCall* call); void LowerArgsForCall(GenTreeCall* call); void ReplaceArgWithPutArgOrBitcast(GenTree** ppChild, GenTree* newNode); + void InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, GenTreeLclVar* lclVar); GenTree* NewPutArg(GenTreeCall* call, GenTree* arg, CallArg* callArg, var_types type); void LowerArg(GenTreeCall* call, CallArg* callArg, bool late); #if defined(TARGET_ARMARCH) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) diff --git a/src/coreclr/vm/ilmarshalers.cpp b/src/coreclr/vm/ilmarshalers.cpp index 66deb428babcc..8b7bb8749da4d 100644 --- a/src/coreclr/vm/ilmarshalers.cpp +++ b/src/coreclr/vm/ilmarshalers.cpp @@ -3443,36 +3443,6 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver pslILDispatch->EmitLDLOC(dwPinnedArgLocal); pslILDispatch->EmitLDOBJ(pslIL->GetToken(pargs->mm.m_pMT)); - - // 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->EmitLDLOC(dwPinnedArgLocal); - pslIL->EmitCONV_U(); - 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 LocalDesc locDesc(pargs->mm.m_pMT); locDesc.AddModifier(true, pslIL->GetToken(pargs->mm.m_pSigMod)); diff --git a/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs b/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs index d7ae3c0bd3445..f85128ea0be90 100644 --- a/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs +++ b/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs @@ -26,31 +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. + // On x86, we will copy when we spill before the call and copy into the final arg slot. int platformExtra = 0; if (RuntimeInformation.ProcessArchitecture == Architecture.X86) { - platformExtra = 1; + platformExtra = 2; } - // PInvoke will copy twice. Once from argument to parameter, and once from the managed to native parameter. + // PInvoke will copy once. Once from the managed to native parameter. + // On x86, we will copy when we spill before the call and copy into the final arg slot. Assert.Equal(1 + 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. + // Reverse PInvoke will copy 2 times. One from the same path as the PInvoke, + // and one from the reverse P/Invoke call. Assert.Equal(2 + 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. + // PInvoke will copy once from the managed to native parameter. Assert.Equal(1 + 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. + // Reverse PInvoke will copy 2 times. One from the same path as the PInvoke, + // and one from the reverse P/Invoke call. Assert.Equal(2 + platformExtra, (int)testMethod.Invoke(testInstance, null)); } catch (Exception ex) From 52d4da921a80046990692b1ed03318b586ccfdcc Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 5 Aug 2024 15:31:26 -0700 Subject: [PATCH 10/30] Remove now-dead code --- .../src/System/StubHelpers.cs | 69 --- src/coreclr/vm/corelib.h | 15 - src/coreclr/vm/dllimport.cpp | 481 +++++++++++++++--- src/coreclr/vm/dllimport.h | 8 +- src/coreclr/vm/i386/asmhelpers.asm | 24 - src/coreclr/vm/ilmarshalers.cpp | 12 +- src/coreclr/vm/ilmarshalers.h | 15 +- src/coreclr/vm/jitinterface.cpp | 78 +-- src/coreclr/vm/metasig.h | 3 - src/coreclr/vm/mlinfo.cpp | 349 +------------ src/coreclr/vm/mlinfo.h | 19 +- 11 files changed, 423 insertions(+), 650 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs b/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs index 4d04557565c3c..9f2528b1a8231 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs @@ -1315,75 +1315,6 @@ 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 845addcb763dc..a142ffdcfaa95 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -1051,21 +1051,6 @@ 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_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) -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) -#endif // TARGET_X86 -#endif // TARGET_WINDOWS - 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 d6ebd11a59c37..cce391b42c302 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -211,7 +211,7 @@ class StubState virtual void SetLastError(BOOL fSetLastError) = 0; virtual void BeginEmit(DWORD dwStubFlags) = 0; virtual void MarshalReturn(MarshalInfo* pInfo, int argOffset) = 0; - virtual void MarshalArgument(MarshalInfo* pInfo, int argOffset, UINT nativeStackOffset) = 0; + virtual void MarshalArgument(MarshalInfo* pInfo, int argOffset) = 0; virtual void MarshalLCID(int argIdx) = 0; virtual void MarshalField(MarshalInfo* pInfo, UINT32 managedOffset, UINT32 nativeOffset, FieldDesc* pFieldDesc) = 0; @@ -288,7 +288,7 @@ class ILStubState : public StubState SF_IsHRESULTSwapping(m_dwStubFlags)); } - void MarshalArgument(MarshalInfo* pInfo, int argOffset, UINT nativeStackOffset) + void MarshalArgument(MarshalInfo* pInfo, int argOffset) { CONTRACTL { @@ -297,7 +297,7 @@ class ILStubState : public StubState } CONTRACTL_END; - pInfo->GenerateArgumentIL(&m_slIL, argOffset, nativeStackOffset, SF_IsForwardStub(m_dwStubFlags)); + pInfo->GenerateArgumentIL(&m_slIL, argOffset, SF_IsForwardStub(m_dwStubFlags)); } void MarshalField(MarshalInfo* pInfo, UINT32 managedOffset, UINT32 nativeOffset, FieldDesc* pFieldDesc) @@ -1620,10 +1620,6 @@ NDirectStubLinker::NDirectStubLinker( m_pcsSetup->EmitSTLOC(m_dwTargetInterfacePointerLocalNum); } #endif // FEATURE_COMINTEROP - -#if defined(TARGET_X86) && defined(FEATURE_IJW) - m_dwCopyCtorChainLocalNum = (DWORD)-1; -#endif // defined(TARGET_X86) && defined(FEATURE_IJW) } void NDirectStubLinker::SetCallingConvention(CorInfoCallConvExtension unmngCallConv, BOOL fIsVarArg) @@ -1836,23 +1832,6 @@ DWORD NDirectStubLinker::GetReturnValueLocalNum() return m_dwRetValLocalNum; } -#if defined(TARGET_X86) && defined(FEATURE_IJW) -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 // defined(TARGET_X86) && defined(FEATURE_IJW) - BOOL NDirectStubLinker::IsCleanupNeeded() { LIMITED_METHOD_CONTRACT; @@ -2082,10 +2061,6 @@ void NDirectStubLinker::End(DWORD dwStubFlags) } } -#if defined(TARGET_X86) && defined(TARGET_WINDOWS) -EXTERN_C void STDCALL CopyConstructorCallStub(void); -#endif // defined(TARGET_X86) && defined(TARGET_WINDOWS) - void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, MethodDesc * pStubMD) { STANDARD_VM_CONTRACT; @@ -2169,21 +2144,6 @@ void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, Meth } } -#if defined(TARGET_X86) && defined(FEATURE_IJW) - 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. - // 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_X86) && defined(FEATURE_IJW) - // 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 @@ -2415,7 +2375,7 @@ class DispatchStubState : public StubState // For CLR-to-COM late-bound/eventing CONTRACTL_END; } - void MarshalArgument(MarshalInfo* pInfo, int argOffset, UINT nativeStackOffset) + void MarshalArgument(MarshalInfo* pInfo, int argOffset) { CONTRACTL { @@ -3457,7 +3417,6 @@ static MarshalInfo::MarshalType DoMarshalReturnValue(MetaSig& msig, int argOffset, DWORD dwStubFlags, MethodDesc *pMD, - UINT& nativeStackOffset, bool& fStubNeedsCOM, int nativeArgIndex DEBUG_ARG(LPCUTF8 pDebugName) @@ -3580,19 +3539,6 @@ static MarshalInfo::MarshalType DoMarshalReturnValue(MetaSig& msig, return marshalType; } -static inline UINT GetStackOffsetFromStackSize(UINT stackSize, bool fThisCall) -{ - LIMITED_METHOD_CONTRACT; -#ifdef TARGET_X86 - if (fThisCall) - { - // -1 means that the argument is not on the stack - return (stackSize >= TARGET_POINTER_SIZE ? (stackSize - TARGET_POINTER_SIZE) : (UINT)-1); - } -#endif // TARGET_X86 - return stackSize; -} - //--------------------------------------------------------- // Creates a new stub for a N/Direct call. Return refcount is 1. // Note that this function may now throw if it fails to create @@ -3782,7 +3728,7 @@ static void CreateNDirectStubWorker(StubState* pss, MarshalInfo &info = pParamMarshalInfo[argidx - 1]; - pss->MarshalArgument(&info, argOffset, GetStackOffsetFromStackSize(nativeStackSize, fThisCall)); + pss->MarshalArgument(&info, argOffset); nativeStackSize += info.GetNativeArgSize(); fStubNeedsCOM |= info.MarshalerRequiresCOM(); @@ -3818,7 +3764,6 @@ static void CreateNDirectStubWorker(StubState* pss, argOffset, dwStubFlags, pMD, - nativeStackSize, fStubNeedsCOM, nativeArgIndex DEBUG_ARG(pSigDesc->m_pDebugName) @@ -6113,21 +6058,415 @@ PCODE GetILStubForCalli(VASigCookie *pVASigCookie, MethodDesc *pMD) RETURN pVASigCookie->pNDirectILStub; } -#if defined(TARGET_X86) && defined(FEATURE_IJW) -// Copy constructor support for C++/CLI -EXTERN_C void* STDCALL CallCopyConstructorsWorker(void* esp) +namespace { - STATIC_CONTRACT_THROWS; - STATIC_CONTRACT_GC_TRIGGERS; - STATIC_CONTRACT_MODE_PREEMPTIVE; // we've already switched to preemptive + //------------------------------------------------------------------------------------- + // Return the copy ctor for a VC class (if any exists) + //------------------------------------------------------------------------------------- + void FindCopyConstructor(Module *pModule, MethodTable *pMT, MethodDesc **pMDOut) + { + CONTRACTL + { + THROWS; + GC_TRIGGERS; // CompareTypeTokens may trigger GC + MODE_ANY; + } + CONTRACTL_END; + + *pMDOut = NULL; + + HRESULT hr; + mdMethodDef tk; + mdTypeDef cl = pMT->GetCl(); + TypeHandle th = TypeHandle(pMT); + SigTypeContext typeContext(th); + + IMDInternalImport *pInternalImport = pModule->GetMDImport(); + MDEnumHolder hEnumMethod(pInternalImport); + + // + // First try for the new syntax: + // + IfFailThrow(pInternalImport->EnumInit(mdtMethodDef, cl, &hEnumMethod)); + + while (pInternalImport->EnumNext(&hEnumMethod, &tk)) + { + _ASSERTE(TypeFromToken(tk) == mdtMethodDef); + DWORD dwMemberAttrs; + IfFailThrow(pInternalImport->GetMethodDefProps(tk, &dwMemberAttrs)); + + if (IsMdSpecialName(dwMemberAttrs)) + { + ULONG cSig; + PCCOR_SIGNATURE pSig; + LPCSTR pName; + IfFailThrow(pInternalImport->GetNameAndSigOfMethodDef(tk, &pSig, &cSig, &pName)); + + const char *pBaseName = ""; + int ncBaseName = (int)strlen(pBaseName); + int nc = (int)strlen(pName); + if (nc >= ncBaseName && 0 == strcmp(pName + nc - ncBaseName, pBaseName)) + { + MetaSig msig(pSig, cSig, pModule, &typeContext); + + // Looking for the prototype void (Ptr VC, Ptr VC); + if (msig.NumFixedArgs() == 2) + { + if (msig.GetReturnType() == ELEMENT_TYPE_VOID) + { + if (msig.NextArg() == ELEMENT_TYPE_PTR) + { + SigPointer sp1 = msig.GetArgProps(); + IfFailThrow(sp1.GetElemType(NULL)); + CorElementType eType; + IfFailThrow(sp1.GetElemType(&eType)); + if (eType == ELEMENT_TYPE_VALUETYPE) + { + mdToken tk1; + IfFailThrow(sp1.GetToken(&tk1)); + hr = CompareTypeTokensNT(tk1, cl, pModule, pModule); + if (FAILED(hr)) + { + pInternalImport->EnumClose(&hEnumMethod); + ThrowHR(hr); + } + + if (hr == S_OK) + { + if (msig.NextArg() == ELEMENT_TYPE_PTR) + { + SigPointer sp2 = msig.GetArgProps(); + IfFailThrow(sp2.GetElemType(NULL)); + IfFailThrow(sp2.GetElemType(&eType)); + if (eType == ELEMENT_TYPE_VALUETYPE) + { + mdToken tk2; + IfFailThrow(sp2.GetToken(&tk2)); + + hr = (tk2 == tk1) ? S_OK : CompareTypeTokensNT(tk2, cl, pModule, pModule); + if (hr == S_OK) + { + *pMDOut = pModule->LookupMethodDef(tk); + return; + } + } + } + } + } + } + } + } + } + } + } + + // + // Next try the old syntax: global .__ctor + // + IfFailThrow(pInternalImport->EnumGlobalFunctionsInit(&hEnumMethod)); + + while (pInternalImport->EnumNext(&hEnumMethod, &tk)) + { + _ASSERTE(TypeFromToken(tk) == mdtMethodDef); + DWORD dwMemberAttrs; + IfFailThrow(pInternalImport->GetMethodDefProps(tk, &dwMemberAttrs)); + + if (IsMdSpecialName(dwMemberAttrs)) + { + ULONG cSig; + PCCOR_SIGNATURE pSig; + LPCSTR pName; + IfFailThrow(pInternalImport->GetNameAndSigOfMethodDef(tk, &pSig, &cSig, &pName)); + + const char *pBaseName = ".__ctor"; + int ncBaseName = (int)strlen(pBaseName); + int nc = (int)strlen(pName); + if (nc >= ncBaseName && 0 == strcmp(pName + nc - ncBaseName, pBaseName)) + { + + MetaSig msig(pSig, cSig, pModule, &typeContext); + + // Looking for the prototype Ptr VC __ctor(Ptr VC, ByRef VC); + if (msig.NumFixedArgs() == 2) + { + if (msig.GetReturnType() == ELEMENT_TYPE_PTR) + { + SigPointer spret = msig.GetReturnProps(); + IfFailThrow(spret.GetElemType(NULL)); + CorElementType eType; + IfFailThrow(spret.GetElemType(&eType)); + if (eType == ELEMENT_TYPE_VALUETYPE) + { + mdToken tk0; + IfFailThrow(spret.GetToken(&tk0)); + hr = CompareTypeTokensNT(tk0, cl, pModule, pModule); + if (FAILED(hr)) + { + pInternalImport->EnumClose(&hEnumMethod); + ThrowHR(hr); + } + + if (hr == S_OK) + { + if (msig.NextArg() == ELEMENT_TYPE_PTR) + { + SigPointer sp1 = msig.GetArgProps(); + IfFailThrow(sp1.GetElemType(NULL)); + IfFailThrow(sp1.GetElemType(&eType)); + if (eType == ELEMENT_TYPE_VALUETYPE) + { + mdToken tk1; + IfFailThrow(sp1.GetToken(&tk1)); + hr = (tk1 == tk0) ? S_OK : CompareTypeTokensNT(tk1, cl, pModule, pModule); + if (FAILED(hr)) + { + pInternalImport->EnumClose(&hEnumMethod); + ThrowHR(hr); + } + + if (hr == S_OK) + { + if (msig.NextArg() == ELEMENT_TYPE_PTR && + msig.GetArgProps().HasCustomModifier(pModule, "Microsoft.VisualC.IsCXXReferenceModifier", ELEMENT_TYPE_CMOD_OPT)) + { + SigPointer sp2 = msig.GetArgProps(); + IfFailThrow(sp2.GetElemType(NULL)); + IfFailThrow(sp2.GetElemType(&eType)); + if (eType == ELEMENT_TYPE_VALUETYPE) + { + mdToken tk2; + IfFailThrow(sp2.GetToken(&tk2)); + + hr = (tk2 == tk0) ? S_OK : CompareTypeTokensNT(tk2, cl, pModule, pModule); + if (hr == S_OK) + { + *pMDOut = pModule->LookupMethodDef(tk); + return; + } + } + } + } + } + } + } + } + } + } + } + } + } + } + + + //------------------------------------------------------------------------------------- + // Return the destructor for a VC class (if any exists) + //------------------------------------------------------------------------------------- + void FindDestructor(Module *pModule, MethodTable *pMT, MethodDesc **pMDOut) + { + CONTRACTL + { + THROWS; + GC_TRIGGERS; // CompareTypeTokens may trigger GC + MODE_ANY; + } + CONTRACTL_END; + + *pMDOut = NULL; + + HRESULT hr; + mdMethodDef tk; + mdTypeDef cl = pMT->GetCl(); + TypeHandle th = TypeHandle(pMT); + SigTypeContext typeContext(th); + + IMDInternalImport *pInternalImport = pModule->GetMDImport(); + MDEnumHolder hEnumMethod(pInternalImport); + + // + // First try for the new syntax: + // + IfFailThrow(pInternalImport->EnumInit(mdtMethodDef, cl, &hEnumMethod)); + + while (pInternalImport->EnumNext(&hEnumMethod, &tk)) + { + _ASSERTE(TypeFromToken(tk) == mdtMethodDef); + DWORD dwMemberAttrs; + IfFailThrow(pInternalImport->GetMethodDefProps(tk, &dwMemberAttrs)); + + if (IsMdSpecialName(dwMemberAttrs)) + { + ULONG cSig; + PCCOR_SIGNATURE pSig; + LPCSTR pName; + IfFailThrow(pInternalImport->GetNameAndSigOfMethodDef(tk, &pSig, &cSig, &pName)); + + const char *pBaseName = ""; + int ncBaseName = (int)strlen(pBaseName); + int nc = (int)strlen(pName); + if (nc >= ncBaseName && 0 == strcmp(pName + nc - ncBaseName, pBaseName)) + { + MetaSig msig(pSig, cSig, pModule, &typeContext); + + // Looking for the prototype void (Ptr VC); + if (msig.NumFixedArgs() == 1) + { + if (msig.GetReturnType() == ELEMENT_TYPE_VOID) + { + if (msig.NextArg() == ELEMENT_TYPE_PTR) + { + SigPointer sp1 = msig.GetArgProps(); + IfFailThrow(sp1.GetElemType(NULL)); + CorElementType eType; + IfFailThrow(sp1.GetElemType(&eType)); + if (eType == ELEMENT_TYPE_VALUETYPE) + { + mdToken tk1; + IfFailThrow(sp1.GetToken(&tk1)); + + hr = CompareTypeTokensNT(tk1, cl, pModule, pModule); + IfFailThrow(hr); + + if (hr == S_OK) + { + *pMDOut = pModule->LookupMethodDef(tk); + return; + } + } + } + } + } + } + } + } + + + // + // Next try the old syntax: global .__dtor + // + IfFailThrow(pInternalImport->EnumGlobalFunctionsInit(&hEnumMethod)); + + while (pInternalImport->EnumNext(&hEnumMethod, &tk)) + { + _ASSERTE(TypeFromToken(tk) == mdtMethodDef); + ULONG cSig; + PCCOR_SIGNATURE pSig; + LPCSTR pName; + IfFailThrow(pInternalImport->GetNameAndSigOfMethodDef(tk, &pSig, &cSig, &pName)); + + const char *pBaseName = ".__dtor"; + int ncBaseName = (int)strlen(pBaseName); + int nc = (int)strlen(pName); + if (nc >= ncBaseName && 0 == strcmp(pName + nc - ncBaseName, pBaseName)) + { + MetaSig msig(pSig, cSig, pModule, &typeContext); + + // Looking for the prototype void __dtor(Ptr VC); + if (msig.NumFixedArgs() == 1) + { + if (msig.GetReturnType() == ELEMENT_TYPE_VOID) + { + if (msig.NextArg() == ELEMENT_TYPE_PTR) + { + SigPointer sp1 = msig.GetArgProps(); + IfFailThrow(sp1.GetElemType(NULL)); + CorElementType eType; + IfFailThrow(sp1.GetElemType(&eType)); + if (eType == ELEMENT_TYPE_VALUETYPE) + { + mdToken tk1; + IfFailThrow(sp1.GetToken(&tk1)); + hr = CompareTypeTokensNT(tk1, cl, pModule, pModule); + if (FAILED(hr)) + { + pInternalImport->EnumClose(&hEnumMethod); + ThrowHR(hr); + } + + if (hr == S_OK) + { + *pMDOut = pModule->LookupMethodDef(tk); + return; + } + } + } + } + } + } + } + } +} + +bool GenerateCopyConstructorHelper(MethodDesc* ftn, TypeHandle type, DynamicResolver** ppResolver, COR_ILMETHOD_DECODER** ppHeader, CORINFO_METHOD_INFO* methInfo) +{ + if (!type.IsValueType()) + return false; + + MethodTable * pMT = type.AsMethodTable(); + + MethodDesc* pCopyCtor = nullptr; + FindCopyConstructor(pMT->GetModule(), pMT, &pCopyCtor); + + MethodDesc* pDestructor = nullptr; + FindDestructor(pMT->GetModule(), pMT, &pDestructor); + + NewHolder ilResolver = new ILStubResolver(); + ilResolver->SetStubMethodDesc(ftn); + + SigTypeContext genericContext; + SigTypeContext::InitTypeContext(ftn, &genericContext); + + ILStubLinker sl( + ftn->GetModule(), + ftn->GetSignature(), + &genericContext, + ftn, + ILSTUB_LINKER_FLAG_NONE); + + ILCodeStream* pCode = sl.NewCodeStream(ILStubLinker::kDispatch); + + pCode->EmitLDARG(0); + pCode->EmitLDARG(1); + if (pCopyCtor != nullptr) + { + pCode->EmitCALL(pCode->GetToken(pCopyCtor), 2, 0); + } + else + { + pCode->EmitLDC(type.GetSize()); + pCode->EmitCPBLK(); + } + + if (pDestructor != nullptr) + { + pCode->EmitLDARG(1); + pCode->EmitCALL(pCode->GetToken(pDestructor), 1, 0); + } - using ExecuteCallback = void*(STDMETHODCALLTYPE*)(void*); + pCode->EmitRET(); - MethodDesc* pMD = CoreLibBinder::GetMethod(METHOD__COPY_CONSTRUCTOR_CHAIN__EXECUTE_CURRENT_COPIES_AND_GET_TARGET); - ExecuteCallback pExecute = (ExecuteCallback)pMD->GetMultiCallableAddrOfCode(); + // Generate all IL associated data for JIT + { + UINT maxStack; + size_t cbCode = sl.Link(&maxStack); + DWORD cbSig = sl.GetLocalSigSize(); + + COR_ILMETHOD_DECODER* pILHeader = ilResolver->AllocGeneratedIL(cbCode, cbSig, maxStack); + BYTE* pbBuffer = (BYTE*)pILHeader->Code; + BYTE* pbLocalSig = (BYTE*)pILHeader->LocalVarSig; + _ASSERTE(cbSig == pILHeader->cbLocalVarSig); + sl.GenerateCode(pbBuffer, cbCode); + sl.GetLocalSig(pbLocalSig, cbSig); + + // Store the token lookup map + ilResolver->SetTokenLookupMap(sl.GetTokenLookupMap()); + ilResolver->SetJitFlags(CORJIT_FLAGS(CORJIT_FLAGS::CORJIT_FLAG_IL_STUB)); + + *ppResolver = (DynamicResolver*)ilResolver; + *ppHeader = pILHeader; + } - return pExecute(esp); + ilResolver.SuppressRelease(); + return true; } -#endif // defined(TARGET_X86) && defined(FEATURE_IJW) #endif // #ifndef DACCESS_COMPILE diff --git a/src/coreclr/vm/dllimport.h b/src/coreclr/vm/dllimport.h index f5e2c058fb1a1..2fdd426391a1c 100644 --- a/src/coreclr/vm/dllimport.h +++ b/src/coreclr/vm/dllimport.h @@ -488,9 +488,6 @@ class NDirectStubLinker : public ILStubLinker DWORD GetCleanupWorkListLocalNum(); DWORD GetThreadLocalNum(); DWORD GetReturnValueLocalNum(); -#if defined(TARGET_X86) && defined(FEATURE_IJW) - DWORD GetCopyCtorChainLocalNum(); -#endif // defined(TARGET_X86) && defined(FEATURE_IJW) void SetCleanupNeeded(); void SetExceptionCleanupNeeded(); BOOL IsCleanupWorkListSetup(); @@ -560,10 +557,6 @@ class NDirectStubLinker : public ILStubLinker DWORD m_dwTargetEntryPointLocalNum; #endif // FEATURE_COMINTEROP -#if defined(TARGET_X86) && defined(FEATURE_IJW) - DWORD m_dwCopyCtorChainLocalNum; -#endif // defined(TARGET_X86) && defined(FEATURE_IJW) - BOOL m_fHasCleanupCode; BOOL m_fHasExceptionCleanupCode; BOOL m_fCleanupWorkListIsSetup; @@ -599,6 +592,7 @@ HRESULT FindPredefinedILStubMethod(MethodDesc *pTargetMD, DWORD dwStubFlags, Met #ifndef DACCESS_COMPILE void MarshalStructViaILStub(MethodDesc* pStubMD, void* pManagedData, void* pNativeData, StructMarshalStubs::MarshalOperation operation, void** ppCleanupWorkList = nullptr); void MarshalStructViaILStubCode(PCODE pStubCode, void* pManagedData, void* pNativeData, StructMarshalStubs::MarshalOperation operation, void** ppCleanupWorkList = nullptr); +bool GenerateCopyConstructorHelper(MethodDesc* ftn, TypeHandle type, DynamicResolver** ppResolver, COR_ILMETHOD_DECODER** ppHeader, CORINFO_METHOD_INFO* methInfo); #endif // DACCESS_COMPILE // diff --git a/src/coreclr/vm/i386/asmhelpers.asm b/src/coreclr/vm/i386/asmhelpers.asm index 07f6e8b955d03..801de37407386 100644 --- a/src/coreclr/vm/i386/asmhelpers.asm +++ b/src/coreclr/vm/i386/asmhelpers.asm @@ -40,7 +40,6 @@ EXTERN _NDirectImportWorker@4:PROC EXTERN _VarargPInvokeStubWorker@12:PROC EXTERN _GenericPInvokeCalliStubWorker@12:PROC -EXTERN _CallCopyConstructorsWorker@4:PROC EXTERN _PreStubWorker@8:PROC EXTERN _TheUMEntryPrestubWorker@4:PROC @@ -1006,29 +1005,6 @@ 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 8b7bb8749da4d..a9e07da0410b6 100644 --- a/src/coreclr/vm/ilmarshalers.cpp +++ b/src/coreclr/vm/ilmarshalers.cpp @@ -2644,8 +2644,7 @@ MarshalerOverrideStatus ILHandleRefMarshaler::ArgumentOverride(NDirectStubLinker BOOL fManagedToNative, OverrideProcArgs* pargs, UINT* pResID, - UINT argidx, - UINT nativeStackOffset) + UINT argidx) { CONTRACTL { @@ -2737,8 +2736,7 @@ MarshalerOverrideStatus ILSafeHandleMarshaler::ArgumentOverride(NDirectStubLinke BOOL fManagedToNative, OverrideProcArgs* pargs, UINT* pResID, - UINT argidx, - UINT nativeStackOffset) + UINT argidx) { CONTRACTL { @@ -3095,8 +3093,7 @@ MarshalerOverrideStatus ILCriticalHandleMarshaler::ArgumentOverride(NDirectStubL BOOL fManagedToNative, OverrideProcArgs* pargs, UINT* pResID, - UINT argidx, - UINT nativeStackOffset) + UINT argidx) { CONTRACTL { @@ -3402,8 +3399,7 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver BOOL fManagedToNative, OverrideProcArgs* pargs, UINT* pResID, - UINT argidx, - UINT nativeStackOffset) + UINT argidx) { CONTRACTL { diff --git a/src/coreclr/vm/ilmarshalers.h b/src/coreclr/vm/ilmarshalers.h index 11c3983fc6549..ca84692561b03 100644 --- a/src/coreclr/vm/ilmarshalers.h +++ b/src/coreclr/vm/ilmarshalers.h @@ -1490,8 +1490,7 @@ class ILMarshaler BOOL fManagedToNative, OverrideProcArgs* pargs, UINT* pResID, - UINT argidx, - UINT nativeStackOffset) + UINT argidx) { LIMITED_METHOD_CONTRACT; return HANDLEASNORMAL; @@ -2207,8 +2206,7 @@ class ILHandleRefMarshaler : public ILMarshaler BOOL fManagedToNative, OverrideProcArgs* pargs, UINT* pResID, - UINT argidx, - UINT nativeStackOffset); + UINT argidx); static MarshalerOverrideStatus ReturnOverride(NDirectStubLinker* psl, BOOL fManagedToNative, @@ -2248,8 +2246,7 @@ class ILSafeHandleMarshaler : public ILMarshaler BOOL fManagedToNative, OverrideProcArgs* pargs, UINT* pResID, - UINT argidx, - UINT nativeStackOffset); + UINT argidx); static MarshalerOverrideStatus ReturnOverride(NDirectStubLinker *psl, BOOL fManagedToNative, @@ -2292,8 +2289,7 @@ class ILCriticalHandleMarshaler : public ILMarshaler BOOL fManagedToNative, OverrideProcArgs* pargs, UINT* pResID, - UINT argidx, - UINT nativeStackOffset); + UINT argidx); static MarshalerOverrideStatus ReturnOverride(NDirectStubLinker *psl, BOOL fManagedToNative, @@ -2952,8 +2948,7 @@ class ILBlittableValueClassWithCopyCtorMarshaler : public ILMarshaler BOOL fManagedToNative, OverrideProcArgs* pargs, UINT* pResID, - UINT argidx, - UINT nativeStackOffset); + UINT argidx); }; diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 57373f3a43537..6e4fa79ff124f 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -7549,82 +7549,6 @@ class MethodInfoHelperContext final } }; -namespace -{ - bool GenerateCopyConstructorHelper(MethodDesc* ftn, TypeHandle type, MethodInfoHelperContext& cxt, CORINFO_METHOD_INFO* methInfo) - { - if (!type.IsValueType()) - return false; - - MethodTable * pMT = type.AsMethodTable(); - - MethodDesc* pCopyCtor = nullptr; - IJWHelpers::FindCopyConstructor(pMT->GetModule(), pMT, &pCopyCtor); - - MethodDesc* pDestructor = nullptr; - IJWHelpers::FindDestructor(pMT->GetModule(), pMT, &pDestructor); - - NewHolder ilResolver = new ILStubResolver(); - ilResolver->SetStubMethodDesc(ftn); - - SigTypeContext genericContext; - SigTypeContext::InitTypeContext(ftn, &genericContext); - - ILStubLinker sl( - ftn->GetModule(), - ftn->GetSignature(), - &genericContext, - ftn, - ILSTUB_LINKER_FLAG_NONE); - - ILCodeStream* pCode = sl.NewCodeStream(ILStubLinker::kDispatch); - - pCode->EmitLDARG(0); - pCode->EmitLDARG(1); - if (pCopyCtor != nullptr) - { - pCode->EmitCALL(pCode->GetToken(pCopyCtor), 2, 0); - } - else - { - pCode->EmitLDC(type.GetSize()); - pCode->EmitCPBLK(); - } - - if (pDestructor != nullptr) - { - pCode->EmitLDARG(1); - pCode->EmitCALL(pCode->GetToken(pDestructor), 1, 0); - } - - pCode->EmitRET(); - - // Generate all IL associated data for JIT - { - UINT maxStack; - size_t cbCode = sl.Link(&maxStack); - DWORD cbSig = sl.GetLocalSigSize(); - - COR_ILMETHOD_DECODER* pILHeader = ilResolver->AllocGeneratedIL(cbCode, cbSig, maxStack); - BYTE* pbBuffer = (BYTE*)pILHeader->Code; - BYTE* pbLocalSig = (BYTE*)pILHeader->LocalVarSig; - _ASSERTE(cbSig == pILHeader->cbLocalVarSig); - sl.GenerateCode(pbBuffer, cbCode); - sl.GetLocalSig(pbLocalSig, cbSig); - - // Store the token lookup map - ilResolver->SetTokenLookupMap(sl.GetTokenLookupMap()); - ilResolver->SetJitFlags(CORJIT_FLAGS(CORJIT_FLAGS::CORJIT_FLAG_IL_STUB)); - - cxt.TransientResolver = (DynamicResolver*)ilResolver; - cxt.Header = pILHeader; - } - - ilResolver.SuppressRelease(); - return true; - } -} - static void getMethodInfoHelper( MethodInfoHelperContext& cxt, CORINFO_METHOD_INFO* methInfo, @@ -7707,7 +7631,7 @@ static void getMethodInfoHelper( _ASSERTE(inst.GetNumArgs() == 1); TypeHandle type = inst[0]; - if (!GenerateCopyConstructorHelper(ftn, type, cxt, methInfo)) + if (!GenerateCopyConstructorHelper(ftn, type, &cxt.TransientResolver, &cxt.Header, methInfo)) { ThrowHR(COR_E_BADIMAGEFORMAT); } diff --git a/src/coreclr/vm/metasig.h b/src/coreclr/vm/metasig.h index 2a74fa3511258..a6fa300353563 100644 --- a/src/coreclr/vm/metasig.h +++ b/src/coreclr/vm/metasig.h @@ -590,9 +590,6 @@ DEFINE_METASIG_T(SM(RefCleanupWorkListElement_Obj_RetVoid, r(C(CLEANUP_WORK_LIST DEFINE_METASIG(SM(PtrVoid_RetPtrVoid, P(v), P(v))) DEFINE_METASIG(SM(PtrVoid_PtrVoid_PtrVoid_RetVoid, P(v) P(v) P(v), v)) DEFINE_METASIG(IM(PtrVoid_RetVoid, P(v), v)) -#if defined(TARGET_X86) && defined(TARGET_WINDOWS) -DEFINE_METASIG_T(IM(PtrCopyConstructorCookie_RetVoid, P(g(COPY_CONSTRUCTOR_COOKIE)), v)) -#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 14bb59efee8a9..53edc30eec3d8 100644 --- a/src/coreclr/vm/mlinfo.cpp +++ b/src/coreclr/vm/mlinfo.cpp @@ -46,344 +46,6 @@ #define INITIAL_NUM_CMINFO_HASHTABLE_BUCKETS 32 #define DEBUG_CONTEXT_STR_LEN 2000 -namespace IJWHelpers -{ - //------------------------------------------------------------------------------------- - // Return the copy ctor for a VC class (if any exists) - //------------------------------------------------------------------------------------- - void FindCopyConstructor(Module *pModule, MethodTable *pMT, MethodDesc **pMDOut) - { - CONTRACTL - { - THROWS; - GC_TRIGGERS; // CompareTypeTokens may trigger GC - MODE_ANY; - } - CONTRACTL_END; - - *pMDOut = NULL; - - HRESULT hr; - mdMethodDef tk; - mdTypeDef cl = pMT->GetCl(); - TypeHandle th = TypeHandle(pMT); - SigTypeContext typeContext(th); - - IMDInternalImport *pInternalImport = pModule->GetMDImport(); - MDEnumHolder hEnumMethod(pInternalImport); - - // - // First try for the new syntax: - // - IfFailThrow(pInternalImport->EnumInit(mdtMethodDef, cl, &hEnumMethod)); - - while (pInternalImport->EnumNext(&hEnumMethod, &tk)) - { - _ASSERTE(TypeFromToken(tk) == mdtMethodDef); - DWORD dwMemberAttrs; - IfFailThrow(pInternalImport->GetMethodDefProps(tk, &dwMemberAttrs)); - - if (IsMdSpecialName(dwMemberAttrs)) - { - ULONG cSig; - PCCOR_SIGNATURE pSig; - LPCSTR pName; - IfFailThrow(pInternalImport->GetNameAndSigOfMethodDef(tk, &pSig, &cSig, &pName)); - - const char *pBaseName = ""; - int ncBaseName = (int)strlen(pBaseName); - int nc = (int)strlen(pName); - if (nc >= ncBaseName && 0 == strcmp(pName + nc - ncBaseName, pBaseName)) - { - MetaSig msig(pSig, cSig, pModule, &typeContext); - - // Looking for the prototype void (Ptr VC, Ptr VC); - if (msig.NumFixedArgs() == 2) - { - if (msig.GetReturnType() == ELEMENT_TYPE_VOID) - { - if (msig.NextArg() == ELEMENT_TYPE_PTR) - { - SigPointer sp1 = msig.GetArgProps(); - IfFailThrow(sp1.GetElemType(NULL)); - CorElementType eType; - IfFailThrow(sp1.GetElemType(&eType)); - if (eType == ELEMENT_TYPE_VALUETYPE) - { - mdToken tk1; - IfFailThrow(sp1.GetToken(&tk1)); - hr = CompareTypeTokensNT(tk1, cl, pModule, pModule); - if (FAILED(hr)) - { - pInternalImport->EnumClose(&hEnumMethod); - ThrowHR(hr); - } - - if (hr == S_OK) - { - if (msig.NextArg() == ELEMENT_TYPE_PTR) - { - SigPointer sp2 = msig.GetArgProps(); - IfFailThrow(sp2.GetElemType(NULL)); - IfFailThrow(sp2.GetElemType(&eType)); - if (eType == ELEMENT_TYPE_VALUETYPE) - { - mdToken tk2; - IfFailThrow(sp2.GetToken(&tk2)); - - hr = (tk2 == tk1) ? S_OK : CompareTypeTokensNT(tk2, cl, pModule, pModule); - if (hr == S_OK) - { - *pMDOut = pModule->LookupMethodDef(tk); - return; - } - } - } - } - } - } - } - } - } - } - } - - // - // Next try the old syntax: global .__ctor - // - IfFailThrow(pInternalImport->EnumGlobalFunctionsInit(&hEnumMethod)); - - while (pInternalImport->EnumNext(&hEnumMethod, &tk)) - { - _ASSERTE(TypeFromToken(tk) == mdtMethodDef); - DWORD dwMemberAttrs; - IfFailThrow(pInternalImport->GetMethodDefProps(tk, &dwMemberAttrs)); - - if (IsMdSpecialName(dwMemberAttrs)) - { - ULONG cSig; - PCCOR_SIGNATURE pSig; - LPCSTR pName; - IfFailThrow(pInternalImport->GetNameAndSigOfMethodDef(tk, &pSig, &cSig, &pName)); - - const char *pBaseName = ".__ctor"; - int ncBaseName = (int)strlen(pBaseName); - int nc = (int)strlen(pName); - if (nc >= ncBaseName && 0 == strcmp(pName + nc - ncBaseName, pBaseName)) - { - - MetaSig msig(pSig, cSig, pModule, &typeContext); - - // Looking for the prototype Ptr VC __ctor(Ptr VC, ByRef VC); - if (msig.NumFixedArgs() == 2) - { - if (msig.GetReturnType() == ELEMENT_TYPE_PTR) - { - SigPointer spret = msig.GetReturnProps(); - IfFailThrow(spret.GetElemType(NULL)); - CorElementType eType; - IfFailThrow(spret.GetElemType(&eType)); - if (eType == ELEMENT_TYPE_VALUETYPE) - { - mdToken tk0; - IfFailThrow(spret.GetToken(&tk0)); - hr = CompareTypeTokensNT(tk0, cl, pModule, pModule); - if (FAILED(hr)) - { - pInternalImport->EnumClose(&hEnumMethod); - ThrowHR(hr); - } - - if (hr == S_OK) - { - if (msig.NextArg() == ELEMENT_TYPE_PTR) - { - SigPointer sp1 = msig.GetArgProps(); - IfFailThrow(sp1.GetElemType(NULL)); - IfFailThrow(sp1.GetElemType(&eType)); - if (eType == ELEMENT_TYPE_VALUETYPE) - { - mdToken tk1; - IfFailThrow(sp1.GetToken(&tk1)); - hr = (tk1 == tk0) ? S_OK : CompareTypeTokensNT(tk1, cl, pModule, pModule); - if (FAILED(hr)) - { - pInternalImport->EnumClose(&hEnumMethod); - ThrowHR(hr); - } - - if (hr == S_OK) - { - if (msig.NextArg() == ELEMENT_TYPE_PTR && - msig.GetArgProps().HasCustomModifier(pModule, "Microsoft.VisualC.IsCXXReferenceModifier", ELEMENT_TYPE_CMOD_OPT)) - { - SigPointer sp2 = msig.GetArgProps(); - IfFailThrow(sp2.GetElemType(NULL)); - IfFailThrow(sp2.GetElemType(&eType)); - if (eType == ELEMENT_TYPE_VALUETYPE) - { - mdToken tk2; - IfFailThrow(sp2.GetToken(&tk2)); - - hr = (tk2 == tk0) ? S_OK : CompareTypeTokensNT(tk2, cl, pModule, pModule); - if (hr == S_OK) - { - *pMDOut = pModule->LookupMethodDef(tk); - return; - } - } - } - } - } - } - } - } - } - } - } - } - } - } - - - //------------------------------------------------------------------------------------- - // Return the destructor for a VC class (if any exists) - //------------------------------------------------------------------------------------- - void FindDestructor(Module *pModule, MethodTable *pMT, MethodDesc **pMDOut) - { - CONTRACTL - { - THROWS; - GC_TRIGGERS; // CompareTypeTokens may trigger GC - MODE_ANY; - } - CONTRACTL_END; - - *pMDOut = NULL; - - HRESULT hr; - mdMethodDef tk; - mdTypeDef cl = pMT->GetCl(); - TypeHandle th = TypeHandle(pMT); - SigTypeContext typeContext(th); - - IMDInternalImport *pInternalImport = pModule->GetMDImport(); - MDEnumHolder hEnumMethod(pInternalImport); - - // - // First try for the new syntax: - // - IfFailThrow(pInternalImport->EnumInit(mdtMethodDef, cl, &hEnumMethod)); - - while (pInternalImport->EnumNext(&hEnumMethod, &tk)) - { - _ASSERTE(TypeFromToken(tk) == mdtMethodDef); - DWORD dwMemberAttrs; - IfFailThrow(pInternalImport->GetMethodDefProps(tk, &dwMemberAttrs)); - - if (IsMdSpecialName(dwMemberAttrs)) - { - ULONG cSig; - PCCOR_SIGNATURE pSig; - LPCSTR pName; - IfFailThrow(pInternalImport->GetNameAndSigOfMethodDef(tk, &pSig, &cSig, &pName)); - - const char *pBaseName = ""; - int ncBaseName = (int)strlen(pBaseName); - int nc = (int)strlen(pName); - if (nc >= ncBaseName && 0 == strcmp(pName + nc - ncBaseName, pBaseName)) - { - MetaSig msig(pSig, cSig, pModule, &typeContext); - - // Looking for the prototype void (Ptr VC); - if (msig.NumFixedArgs() == 1) - { - if (msig.GetReturnType() == ELEMENT_TYPE_VOID) - { - if (msig.NextArg() == ELEMENT_TYPE_PTR) - { - SigPointer sp1 = msig.GetArgProps(); - IfFailThrow(sp1.GetElemType(NULL)); - CorElementType eType; - IfFailThrow(sp1.GetElemType(&eType)); - if (eType == ELEMENT_TYPE_VALUETYPE) - { - mdToken tk1; - IfFailThrow(sp1.GetToken(&tk1)); - - hr = CompareTypeTokensNT(tk1, cl, pModule, pModule); - IfFailThrow(hr); - - if (hr == S_OK) - { - *pMDOut = pModule->LookupMethodDef(tk); - return; - } - } - } - } - } - } - } - } - - - // - // Next try the old syntax: global .__dtor - // - IfFailThrow(pInternalImport->EnumGlobalFunctionsInit(&hEnumMethod)); - - while (pInternalImport->EnumNext(&hEnumMethod, &tk)) - { - _ASSERTE(TypeFromToken(tk) == mdtMethodDef); - ULONG cSig; - PCCOR_SIGNATURE pSig; - LPCSTR pName; - IfFailThrow(pInternalImport->GetNameAndSigOfMethodDef(tk, &pSig, &cSig, &pName)); - - const char *pBaseName = ".__dtor"; - int ncBaseName = (int)strlen(pBaseName); - int nc = (int)strlen(pName); - if (nc >= ncBaseName && 0 == strcmp(pName + nc - ncBaseName, pBaseName)) - { - MetaSig msig(pSig, cSig, pModule, &typeContext); - - // Looking for the prototype void __dtor(Ptr VC); - if (msig.NumFixedArgs() == 1) - { - if (msig.GetReturnType() == ELEMENT_TYPE_VOID) - { - if (msig.NextArg() == ELEMENT_TYPE_PTR) - { - SigPointer sp1 = msig.GetArgProps(); - IfFailThrow(sp1.GetElemType(NULL)); - CorElementType eType; - IfFailThrow(sp1.GetElemType(&eType)); - if (eType == ELEMENT_TYPE_VALUETYPE) - { - mdToken tk1; - IfFailThrow(sp1.GetToken(&tk1)); - hr = CompareTypeTokensNT(tk1, cl, pModule, pModule); - if (FAILED(hr)) - { - pInternalImport->EnumClose(&hEnumMethod); - ThrowHR(hr); - } - - if (hr == S_OK) - { - *pMDOut = pModule->LookupMethodDef(tk); - return; - } - } - } - } - } - } - } - } -} - //========================================================================== // Set's up the custom marshaler information. //========================================================================== @@ -2382,15 +2044,8 @@ MarshalInfo::MarshalInfo(Module* pModule, if (pCopyCtorModifier != mdTokenNil && !IsFieldScenario()) // We don't support automatically discovering copy constructors for fields. { #if defined(FEATURE_IJW) - MethodDesc *pCopyCtor; - MethodDesc *pDtor; - IJWHelpers::FindCopyConstructor(pModule, m_pMT, &pCopyCtor); - IJWHelpers::FindDestructor(pModule, m_pMT, &pDtor); - m_args.mm.m_pSigMod = ClassLoader::LoadTypeDefOrRefThrowing(pModule, pCopyCtorModifier).AsMethodTable(); m_args.mm.m_pMT = m_pMT; - m_args.mm.m_pCopyCtor = pCopyCtor; - m_args.mm.m_pDtor = pDtor; m_type = MARSHAL_TYPE_BLITTABLEVALUECLASSWITHCOPYCTOR; #else // !defined(FEATURE_IJW) m_resID = IDS_EE_BADMARSHAL_BADMANAGED; @@ -2812,7 +2467,6 @@ namespace void MarshalInfo::GenerateArgumentIL(NDirectStubLinker* psl, int argOffset, // the argument's index is m_paramidx + argOffset - UINT nativeStackOffset, // offset of the argument on the native stack BOOL fMngToNative) { CONTRACTL @@ -2840,8 +2494,7 @@ void MarshalInfo::GenerateArgumentIL(NDirectStubLinker* psl, fMngToNative, &m_args, &resID, - m_paramidx + argOffset, - nativeStackOffset); + m_paramidx + argOffset); if (amostat == OVERRIDDEN) diff --git a/src/coreclr/vm/mlinfo.h b/src/coreclr/vm/mlinfo.h index 54d8fbc0fa2b1..795427ef4b9a4 100644 --- a/src/coreclr/vm/mlinfo.h +++ b/src/coreclr/vm/mlinfo.h @@ -89,8 +89,6 @@ struct OverrideProcArgs { MethodTable* m_pSigMod; MethodTable* m_pMT; - MethodDesc* m_pCopyCtor; - MethodDesc* m_pDtor; } mm; struct @@ -114,8 +112,7 @@ typedef MarshalerOverrideStatus (*OVERRIDEPROC)(NDirectStubLinker* psl, BOOL fManagedToNative, OverrideProcArgs* pargs, UINT* pResID, - UINT argidx, - UINT nativeStackOffset); + UINT argidx); typedef MarshalerOverrideStatus (*RETURNOVERRIDEPROC)(NDirectStubLinker* psl, BOOL fManagedToNative, @@ -123,19 +120,6 @@ typedef MarshalerOverrideStatus (*RETURNOVERRIDEPROC)(NDirectStubLinker* psl, OverrideProcArgs* pargs, UINT* pResID); -namespace IJWHelpers -{ - //------------------------------------------------------------------------------------- - // Return the copy ctor for a VC class (if any exists) - //------------------------------------------------------------------------------------- - void FindCopyConstructor(Module *pModule, MethodTable *pMT, MethodDesc **pMDOut); - - //------------------------------------------------------------------------------------- - // Return the destructor for a VC class (if any exists) - //------------------------------------------------------------------------------------- - void FindDestructor(Module *pModule, MethodTable *pMT, MethodDesc **pMDOut); -} - //========================================================================== // This structure contains the native type information for a given // parameter. @@ -370,7 +354,6 @@ class MarshalInfo void GenerateArgumentIL(NDirectStubLinker* psl, int argOffset, // the argument's index is m_paramidx + argOffset - UINT nativeStackOffset, // offset of the argument on the native stack BOOL fMngToNative); void GenerateReturnIL(NDirectStubLinker* psl, From 1e5541044041b266fd1b7d707b3077ac8b0abae8 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 5 Aug 2024 16:02:17 -0700 Subject: [PATCH 11/30] Fix formatting --- src/coreclr/jit/gentree.cpp | 5 +++-- src/coreclr/jit/gschecks.cpp | 13 ++++++++----- src/coreclr/jit/lclvars.cpp | 2 +- src/coreclr/jit/lower.cpp | 14 +++++++++----- 4 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 0beca9ef9e87a..296b3670f5100 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16682,9 +16682,10 @@ GenTree* Compiler::gtNewTempStore( if (varDsc->lvRequiresSpecialCopy) { JITDUMP("Var V%02u requires special copy\n", tmp); - CORINFO_METHOD_HANDLE copyHelper = info.compCompHnd->GetSpecialCopyHelper(varDsc->GetLayout()->GetClassHandle()); + CORINFO_METHOD_HANDLE copyHelper = + info.compCompHnd->GetSpecialCopyHelper(varDsc->GetLayout()->GetClassHandle()); GenTreeCall* call = gtNewCallNode(CT_USER_FUNC, copyHelper, TYP_VOID); - + GenTree* src; assert(val->OperIs(GT_BLK)); diff --git a/src/coreclr/jit/gschecks.cpp b/src/coreclr/jit/gschecks.cpp index e2aa54721bb33..72b68b5915b97 100644 --- a/src/coreclr/jit/gschecks.cpp +++ b/src/coreclr/jit/gschecks.cpp @@ -518,10 +518,12 @@ void Compiler::gsParamsToShadows() if (varDsc->lvRequiresSpecialCopy) { - JITDUMP("Var V%02u requires special copy, using special copy helper to copy to shadow var V%02u\n", lclNum, shadowVarNum); - CORINFO_METHOD_HANDLE copyHelper = info.compCompHnd->GetSpecialCopyHelper(varDsc->GetLayout()->GetClassHandle()); + JITDUMP("Var V%02u requires special copy, using special copy helper to copy to shadow var V%02u\n", lclNum, + shadowVarNum); + CORINFO_METHOD_HANDLE copyHelper = + info.compCompHnd->GetSpecialCopyHelper(varDsc->GetLayout()->GetClassHandle()); GenTreeCall* call = gtNewCallNode(CT_USER_FUNC, copyHelper, TYP_VOID); - + GenTree* src = gtNewLclVarAddrNode(lclNum); GenTree* dst = gtNewLclVarAddrNode(shadowVarNum); @@ -532,7 +534,8 @@ void Compiler::gsParamsToShadows() compCurBB = fgFirstBB; // Needed by some morphing if (opts.IsReversePInvoke()) { - JITDUMP("Inserting special copy helper call at the end of the first block after Reverse P/Invoke transition\n"); + JITDUMP( + "Inserting special copy helper call at the end of the first block after Reverse P/Invoke transition\n"); // If we are in a reverse P/Invoke, then we need to insert // the call at the end of the first block as we need to do the GC transition // before we can call the helper. @@ -548,7 +551,7 @@ void Compiler::gsParamsToShadows() { GenTree* src = gtNewLclvNode(lclNum, varDsc->TypeGet()); src->gtFlags |= GTF_DONT_CSE; - + GenTree* store = gtNewStoreLclVarNode(shadowVarNum, src); fgEnsureFirstBBisScratch(); diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 701617906bc18..0f7d9fde3051f 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -665,7 +665,7 @@ void Compiler::lvaInitUserArgs(InitVarDscInfo* varDscInfo, unsigned skipArgs, un CorInfoTypeWithMod corInfoType = info.compCompHnd->getArgType(&info.compMethodInfo->args, argLst, &typeHnd); varDsc->lvIsParam = 1; - + if ((corInfoType & CORINFO_TYPE_MOD_COPY_WITH_HELPER) != 0) { if (strip(corInfoType) == CORINFO_TYPE_VALUECLASS) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 102082f9fbe27..88f1b1297bfdb 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -1531,7 +1531,8 @@ void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, GenTreeLclVar* GenTree* dest; #if FEATURE_FIXED_OUT_ARGS - dest = comp->gtNewOperNode(GT_ADD, TYP_I_IMPL, comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL), comp->gtNewIconNode(putArgStk->getArgOffset())); + dest = comp->gtNewOperNode(GT_ADD, TYP_I_IMPL, comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL), + comp->gtNewIconNode(putArgStk->getArgOffset())); #else dest = comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL); #endif @@ -1539,9 +1540,12 @@ void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, GenTreeLclVar* GenTree* src = comp->gtNewLclAddrNode(lclVar->GetLclNum(), lclVar->GetLclOffs(), TYP_I_IMPL); GenTree* destPlaceholder = comp->gtNewZeroConNode(dest->TypeGet()); - GenTree* srcPlaceholder = comp->gtNewZeroConNode(genActualType(src)); + GenTree* srcPlaceholder = comp->gtNewZeroConNode(genActualType(src)); - GenTreeCall* call = comp->gtNewCallNode(CT_USER_FUNC, comp->info.compCompHnd->GetSpecialCopyHelper(lclVar->GetLayout(comp)->GetClassHandle()), TYP_VOID); + GenTreeCall* call = + comp->gtNewCallNode(CT_USER_FUNC, + comp->info.compCompHnd->GetSpecialCopyHelper(lclVar->GetLayout(comp)->GetClassHandle()), + TYP_VOID); call->gtArgs.PushBack(comp, NewCallArg::Primitive(destPlaceholder)); call->gtArgs.PushBack(comp, NewCallArg::Primitive(srcPlaceholder)); @@ -1932,8 +1936,8 @@ void Lowering::LowerArg(GenTreeCall* call, CallArg* callArg, bool late) ReplaceArgWithPutArgOrBitcast(ppArg, putArg); } - if (putArg->OperIsPutArgStk() - && arg->OperIs(GT_LCL_VAR) && comp->lvaTable[arg->AsLclVar()->GetLclNum()].lvRequiresSpecialCopy) + if (putArg->OperIsPutArgStk() && arg->OperIs(GT_LCL_VAR) && + comp->lvaTable[arg->AsLclVar()->GetLclNum()].lvRequiresSpecialCopy) { InsertSpecialCopyArg(putArg->AsPutArgStk(), arg->AsLclVar()); } From 59c4aafa0298b4500ead2a4c7da9f19d11b9b2d5 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 5 Aug 2024 17:02:51 -0700 Subject: [PATCH 12/30] Fix build on Windows x64 --- src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h | 1 + src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp | 7 +++++++ src/coreclr/vm/ilmarshalers.cpp | 2 +- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h index 7fad15034425f..accbdf4c25ed9 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h @@ -1191,6 +1191,7 @@ enum mcPackets Packet_GetTypeForBoxOnStack = 221, Packet_GetTypeDefinition = 222, Packet_GetFpStructLowering = 223, + Packet_GetSpecialCopyHelper = 224, }; void SetDebugDumpVariables(); diff --git a/src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp b/src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp index 4180115db640e..f21a5a26cca19 100644 --- a/src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp +++ b/src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp @@ -1863,3 +1863,10 @@ uint32_t MyICJI::getExpectedTargetArchitecture() DWORD result = jitInstance->mc->repGetExpectedTargetArchitecture(); return result; } + +CORINFO_METHOD_HANDLE MyICJI::GetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) +{ + jitInstance->mc->cr->AddCall("GetSpecialCopyHelper"); + CORINFO_METHOD_HANDLE result = jitInstance->mc->repGetSpecialCopyHelper(type); + return result; +} diff --git a/src/coreclr/vm/ilmarshalers.cpp b/src/coreclr/vm/ilmarshalers.cpp index a9e07da0410b6..fb468aeb4f186 100644 --- a/src/coreclr/vm/ilmarshalers.cpp +++ b/src/coreclr/vm/ilmarshalers.cpp @@ -3456,7 +3456,7 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver pslIL->SetStubTargetArgType(ELEMENT_TYPE_U); // native type is a pointer pslILDispatch->EmitLDLOC(dwPinnedArgLocal); - pslILEmit->EmitCONV_U(); + pslILDispatch->EmitCONV_U(); #endif return OVERRIDDEN; From 390185cb244d3fb661a1f4b6197a7a852924ff45 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 5 Aug 2024 19:45:52 -0700 Subject: [PATCH 13/30] Call copy helper in lowering and heavily reduce changes in higher layers --- src/coreclr/jit/gentree.cpp | 32 ++-------- src/coreclr/jit/gschecks.cpp | 46 ++++----------- src/coreclr/jit/lower.cpp | 110 +++++++++++++++++++++++++++++++++-- src/coreclr/jit/lower.h | 2 + 4 files changed, 125 insertions(+), 65 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 296b3670f5100..3e7898054fa91 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16678,36 +16678,14 @@ GenTree* Compiler::gtNewTempStore( compFloatingPointUsed = true; } - GenTree* store; - if (varDsc->lvRequiresSpecialCopy) - { - JITDUMP("Var V%02u requires special copy\n", tmp); - CORINFO_METHOD_HANDLE copyHelper = - info.compCompHnd->GetSpecialCopyHelper(varDsc->GetLayout()->GetClassHandle()); - GenTreeCall* call = gtNewCallNode(CT_USER_FUNC, copyHelper, TYP_VOID); - - GenTree* src; + GenTree* store = gtNewStoreLclVarNode(tmp, val); - assert(val->OperIs(GT_BLK)); - src = val->AsBlk()->Addr(); + // TODO-ASG: delete this zero-diff quirk. Requires some forward substitution work. + store->gtType = dstTyp; - GenTree* dst = gtNewLclVarAddrNode(tmp); - - call->gtArgs.PushBack(this, NewCallArg::Primitive(dst)); - call->gtArgs.PushBack(this, NewCallArg::Primitive(src)); - store = call; - } - else + if (varTypeIsStruct(varDsc) && !val->IsInitVal()) { - store = gtNewStoreLclVarNode(tmp, val); - - // TODO-ASG: delete this zero-diff quirk. Requires some forward substitution work. - store->gtType = dstTyp; - - if (varTypeIsStruct(varDsc) && !val->IsInitVal()) - { - store = impStoreStruct(store, curLevel, pAfterStmt, di, block); - } + store = impStoreStruct(store, curLevel, pAfterStmt, di, block); } return store; diff --git a/src/coreclr/jit/gschecks.cpp b/src/coreclr/jit/gschecks.cpp index 72b68b5915b97..f40b710325e79 100644 --- a/src/coreclr/jit/gschecks.cpp +++ b/src/coreclr/jit/gschecks.cpp @@ -516,46 +516,24 @@ void Compiler::gsParamsToShadows() continue; } - if (varDsc->lvRequiresSpecialCopy) - { - JITDUMP("Var V%02u requires special copy, using special copy helper to copy to shadow var V%02u\n", lclNum, - shadowVarNum); - CORINFO_METHOD_HANDLE copyHelper = - info.compCompHnd->GetSpecialCopyHelper(varDsc->GetLayout()->GetClassHandle()); - GenTreeCall* call = gtNewCallNode(CT_USER_FUNC, copyHelper, TYP_VOID); + GenTree* src = gtNewLclvNode(lclNum, varDsc->TypeGet()); + src->gtFlags |= GTF_DONT_CSE; - GenTree* src = gtNewLclVarAddrNode(lclNum); - GenTree* dst = gtNewLclVarAddrNode(shadowVarNum); + GenTree* store = gtNewStoreLclVarNode(shadowVarNum, src); - call->gtArgs.PushBack(this, NewCallArg::Primitive(dst)); - call->gtArgs.PushBack(this, NewCallArg::Primitive(src)); + fgEnsureFirstBBisScratch(); + compCurBB = fgFirstBB; // Needed by some morphing - fgEnsureFirstBBisScratch(); - compCurBB = fgFirstBB; // Needed by some morphing - if (opts.IsReversePInvoke()) - { - JITDUMP( - "Inserting special copy helper call at the end of the first block after Reverse P/Invoke transition\n"); - // If we are in a reverse P/Invoke, then we need to insert - // the call at the end of the first block as we need to do the GC transition - // before we can call the helper. - (void)fgNewStmtAtEnd(fgFirstBB, fgMorphTree(call)); - } - else - { - JITDUMP("Inserting special copy helper call at the beginning of the first block\n"); - (void)fgNewStmtAtBeg(fgFirstBB, fgMorphTree(call)); - } + if (varDsc->lvRequiresSpecialCopy && opts.IsReversePInvoke()) + { + // If we are in a reverse P/Invoke, then we need to insert + // the call at the end of the first block as we need to do the GC transition + // before we do the copy. We may end up calling a managed helper. + (void)fgNewStmtAtEnd(fgFirstBB, fgMorphTree(store)); } else { - GenTree* src = gtNewLclvNode(lclNum, varDsc->TypeGet()); - src->gtFlags |= GTF_DONT_CSE; - - GenTree* store = gtNewStoreLclVarNode(shadowVarNum, src); - - fgEnsureFirstBBisScratch(); - compCurBB = fgFirstBB; // Needed by some morphing + (void)fgNewStmtAtBeg(fgFirstBB, fgMorphTree(store)); } } diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 88f1b1297bfdb..3817c1b4a8e88 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4991,7 +4991,14 @@ GenTree* Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore) } #endif // TARGET_XARCH } - convertToStoreObj = false; + if (varDsc->lvRequiresSpecialCopy) + { + convertToStoreObj = true; + } + else + { + convertToStoreObj = false; + } #else // TARGET_ARM64 // This optimization on arm64 allows more SIMD16 vars to be enregistered but it could cause // regressions when there are many calls and before/after each one we have to store/save the upper @@ -8670,6 +8677,92 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) #endif } +//------------------------------------------------------------------------ +// LowerBlockStoreWithSpecialCopyHelper: Lower a block store node using a special copy helper +// +// Arguments: +// blkNode - The block store node to lower +// +void Lowering::LowerBlockStoreWithSpecialCopyHelper(GenTreeBlk* blkNode) +{ + LIR::Use use; + assert(!BlockRange().TryGetUse(blkNode, &use)); + + const bool isVolatile = blkNode->IsVolatile(); + + GenTree* dest = blkNode->Addr(); + GenTree* data = blkNode->Data(); + + if (data->OperIs(GT_IND)) + { + // Drop GT_IND nodes + BlockRange().Remove(data); + data = data->AsIndir()->Addr(); + } + else + { + assert(data->OperIs(GT_LCL_VAR, GT_LCL_FLD)); + + // Convert local to LCL_ADDR + unsigned lclOffset = data->AsLclVarCommon()->GetLclOffs(); + + data->ChangeOper(GT_LCL_ADDR); + data->ChangeType(TYP_I_IMPL); + data->AsLclFld()->SetLclOffs(lclOffset); + data->ClearContained(); + } + + // A hacky way to safely call fgMorphTree in Lower + GenTree* destPlaceholder = comp->gtNewZeroConNode(dest->TypeGet()); + GenTree* dataPlaceholder = comp->gtNewZeroConNode(genActualType(data)); + + GenTreeCall* call = comp->gtNewCallNode(CT_USER_FUNC, comp->info.compCompHnd->GetSpecialCopyHelper(blkNode->GetLayout()->GetClassHandle()), TYP_VOID); + call->gtArgs.PushBack(comp, NewCallArg::Primitive(destPlaceholder)); + call->gtArgs.PushBack(comp, NewCallArg::Primitive(dataPlaceholder)); + + comp->fgMorphArgs(call); + + LIR::Range range = LIR::SeqTree(comp, call); + GenTree* rangeStart = range.FirstNode(); + GenTree* rangeEnd = range.LastNode(); + + BlockRange().InsertBefore(blkNode, std::move(range)); + blkNode->gtBashToNOP(); + + LIR::Use destUse; + BlockRange().TryGetUse(destPlaceholder, &destUse); + destUse.ReplaceWith(dest); + destPlaceholder->SetUnusedValue(); + + LIR::Use dataUse; + BlockRange().TryGetUse(dataPlaceholder, &dataUse); + dataUse.ReplaceWith(data); + dataPlaceholder->SetUnusedValue(); + + LowerRange(rangeStart, rangeEnd); + + // Finally move all GT_PUTARG_* nodes + // Re-use the existing logic for CFG call args here + MoveCFGCallArgs(call); + + BlockRange().Remove(destPlaceholder); + BlockRange().Remove(dataPlaceholder); + +// Wrap with memory barriers on weak memory models +// if the block store was volatile +#ifndef TARGET_XARCH + if (isVolatile) + { + GenTree* firstBarrier = comp->gtNewMemoryBarrier(); + GenTree* secondBarrier = comp->gtNewMemoryBarrier(/*loadOnly*/ true); + BlockRange().InsertBefore(call, firstBarrier); + BlockRange().InsertAfter(call, secondBarrier); + LowerNode(firstBarrier); + LowerNode(secondBarrier); + } +#endif +} + //------------------------------------------------------------------------ // GetLoadStoreCoalescingData: given a STOREIND/IND node, get the data needed to perform // store/load coalescing including pointer to the previous node. @@ -10023,11 +10116,20 @@ void Lowering::LowerBlockStoreCommon(GenTreeBlk* blkNode) assert(blkNode->Data()->IsIntegralConst(0)); } + GenTree* src = blkNode->Data(); + // Lose the type information stored in the source - we no longer need it. - if (blkNode->Data()->OperIs(GT_BLK)) + if (src->OperIs(GT_BLK)) { - blkNode->Data()->SetOper(GT_IND); - LowerIndir(blkNode->Data()->AsIndir()); + src->SetOper(GT_IND); + LowerIndir(src->AsIndir()); + } + + if ((src->OperIsIndir() && src->AsIndir()->Addr()->OperIsLocalRead() && comp->lvaTable[src->AsIndir()->Addr()->AsLclVarCommon()->GetLclNum()].lvRequiresSpecialCopy) + || (src->OperIsLocalRead() && comp->lvaTable[src->AsLclVarCommon()->GetLclNum()].lvRequiresSpecialCopy)) + { + LowerBlockStoreWithSpecialCopyHelper(blkNode); + return; } if (TryTransformStoreObjAsStoreInd(blkNode)) diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 5a7214822d53b..62b50fab0a474 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -360,6 +360,7 @@ class Lowering final : public Phase void MarkTree(GenTree* root); void UnmarkTree(GenTree* root); GenTree* LowerStoreIndir(GenTreeStoreInd* node); + GenTree* LowerStoreIndirWithSpecialCopyHelper(GenTreeStoreInd* node); void LowerStoreIndirCoalescing(GenTreeIndir* node); GenTree* LowerAdd(GenTreeOp* node); GenTree* LowerMul(GenTreeOp* mul); @@ -371,6 +372,7 @@ class Lowering final : public Phase void LowerBlockStore(GenTreeBlk* blkNode); void LowerBlockStoreCommon(GenTreeBlk* blkNode); void LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode); + void LowerBlockStoreWithSpecialCopyHelper(GenTreeBlk* blkNode); bool TryLowerBlockStoreAsGcBulkCopyCall(GenTreeBlk* blkNode); void LowerLclHeap(GenTree* node); void ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenTree* addr, GenTree* addrParent); From 6d5ce12bf48183274d640000ec3868ae0c323241 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 6 Aug 2024 10:56:11 -0700 Subject: [PATCH 14/30] Revert "Call copy helper in lowering and heavily reduce changes in higher layers" This reverts commit 390185cb244d3fb661a1f4b6197a7a852924ff45. --- src/coreclr/jit/gentree.cpp | 32 ++++++++-- src/coreclr/jit/gschecks.cpp | 46 +++++++++++---- src/coreclr/jit/lower.cpp | 110 ++--------------------------------- src/coreclr/jit/lower.h | 2 - 4 files changed, 65 insertions(+), 125 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 3e7898054fa91..296b3670f5100 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16678,14 +16678,36 @@ GenTree* Compiler::gtNewTempStore( compFloatingPointUsed = true; } - GenTree* store = gtNewStoreLclVarNode(tmp, val); + GenTree* store; + if (varDsc->lvRequiresSpecialCopy) + { + JITDUMP("Var V%02u requires special copy\n", tmp); + CORINFO_METHOD_HANDLE copyHelper = + info.compCompHnd->GetSpecialCopyHelper(varDsc->GetLayout()->GetClassHandle()); + GenTreeCall* call = gtNewCallNode(CT_USER_FUNC, copyHelper, TYP_VOID); + + GenTree* src; - // TODO-ASG: delete this zero-diff quirk. Requires some forward substitution work. - store->gtType = dstTyp; + assert(val->OperIs(GT_BLK)); + src = val->AsBlk()->Addr(); - if (varTypeIsStruct(varDsc) && !val->IsInitVal()) + GenTree* dst = gtNewLclVarAddrNode(tmp); + + call->gtArgs.PushBack(this, NewCallArg::Primitive(dst)); + call->gtArgs.PushBack(this, NewCallArg::Primitive(src)); + store = call; + } + else { - store = impStoreStruct(store, curLevel, pAfterStmt, di, block); + store = gtNewStoreLclVarNode(tmp, val); + + // TODO-ASG: delete this zero-diff quirk. Requires some forward substitution work. + store->gtType = dstTyp; + + if (varTypeIsStruct(varDsc) && !val->IsInitVal()) + { + store = impStoreStruct(store, curLevel, pAfterStmt, di, block); + } } return store; diff --git a/src/coreclr/jit/gschecks.cpp b/src/coreclr/jit/gschecks.cpp index f40b710325e79..72b68b5915b97 100644 --- a/src/coreclr/jit/gschecks.cpp +++ b/src/coreclr/jit/gschecks.cpp @@ -516,24 +516,46 @@ void Compiler::gsParamsToShadows() continue; } - GenTree* src = gtNewLclvNode(lclNum, varDsc->TypeGet()); - src->gtFlags |= GTF_DONT_CSE; + if (varDsc->lvRequiresSpecialCopy) + { + JITDUMP("Var V%02u requires special copy, using special copy helper to copy to shadow var V%02u\n", lclNum, + shadowVarNum); + CORINFO_METHOD_HANDLE copyHelper = + info.compCompHnd->GetSpecialCopyHelper(varDsc->GetLayout()->GetClassHandle()); + GenTreeCall* call = gtNewCallNode(CT_USER_FUNC, copyHelper, TYP_VOID); - GenTree* store = gtNewStoreLclVarNode(shadowVarNum, src); + GenTree* src = gtNewLclVarAddrNode(lclNum); + GenTree* dst = gtNewLclVarAddrNode(shadowVarNum); - fgEnsureFirstBBisScratch(); - compCurBB = fgFirstBB; // Needed by some morphing + call->gtArgs.PushBack(this, NewCallArg::Primitive(dst)); + call->gtArgs.PushBack(this, NewCallArg::Primitive(src)); - if (varDsc->lvRequiresSpecialCopy && opts.IsReversePInvoke()) - { - // If we are in a reverse P/Invoke, then we need to insert - // the call at the end of the first block as we need to do the GC transition - // before we do the copy. We may end up calling a managed helper. - (void)fgNewStmtAtEnd(fgFirstBB, fgMorphTree(store)); + fgEnsureFirstBBisScratch(); + compCurBB = fgFirstBB; // Needed by some morphing + if (opts.IsReversePInvoke()) + { + JITDUMP( + "Inserting special copy helper call at the end of the first block after Reverse P/Invoke transition\n"); + // If we are in a reverse P/Invoke, then we need to insert + // the call at the end of the first block as we need to do the GC transition + // before we can call the helper. + (void)fgNewStmtAtEnd(fgFirstBB, fgMorphTree(call)); + } + else + { + JITDUMP("Inserting special copy helper call at the beginning of the first block\n"); + (void)fgNewStmtAtBeg(fgFirstBB, fgMorphTree(call)); + } } else { - + GenTree* src = gtNewLclvNode(lclNum, varDsc->TypeGet()); + src->gtFlags |= GTF_DONT_CSE; + + GenTree* store = gtNewStoreLclVarNode(shadowVarNum, src); + + fgEnsureFirstBBisScratch(); + compCurBB = fgFirstBB; // Needed by some morphing (void)fgNewStmtAtBeg(fgFirstBB, fgMorphTree(store)); } } diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 3817c1b4a8e88..88f1b1297bfdb 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4991,14 +4991,7 @@ GenTree* Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore) } #endif // TARGET_XARCH } - if (varDsc->lvRequiresSpecialCopy) - { - convertToStoreObj = true; - } - else - { - convertToStoreObj = false; - } + convertToStoreObj = false; #else // TARGET_ARM64 // This optimization on arm64 allows more SIMD16 vars to be enregistered but it could cause // regressions when there are many calls and before/after each one we have to store/save the upper @@ -8677,92 +8670,6 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) #endif } -//------------------------------------------------------------------------ -// LowerBlockStoreWithSpecialCopyHelper: Lower a block store node using a special copy helper -// -// Arguments: -// blkNode - The block store node to lower -// -void Lowering::LowerBlockStoreWithSpecialCopyHelper(GenTreeBlk* blkNode) -{ - LIR::Use use; - assert(!BlockRange().TryGetUse(blkNode, &use)); - - const bool isVolatile = blkNode->IsVolatile(); - - GenTree* dest = blkNode->Addr(); - GenTree* data = blkNode->Data(); - - if (data->OperIs(GT_IND)) - { - // Drop GT_IND nodes - BlockRange().Remove(data); - data = data->AsIndir()->Addr(); - } - else - { - assert(data->OperIs(GT_LCL_VAR, GT_LCL_FLD)); - - // Convert local to LCL_ADDR - unsigned lclOffset = data->AsLclVarCommon()->GetLclOffs(); - - data->ChangeOper(GT_LCL_ADDR); - data->ChangeType(TYP_I_IMPL); - data->AsLclFld()->SetLclOffs(lclOffset); - data->ClearContained(); - } - - // A hacky way to safely call fgMorphTree in Lower - GenTree* destPlaceholder = comp->gtNewZeroConNode(dest->TypeGet()); - GenTree* dataPlaceholder = comp->gtNewZeroConNode(genActualType(data)); - - GenTreeCall* call = comp->gtNewCallNode(CT_USER_FUNC, comp->info.compCompHnd->GetSpecialCopyHelper(blkNode->GetLayout()->GetClassHandle()), TYP_VOID); - call->gtArgs.PushBack(comp, NewCallArg::Primitive(destPlaceholder)); - call->gtArgs.PushBack(comp, NewCallArg::Primitive(dataPlaceholder)); - - comp->fgMorphArgs(call); - - LIR::Range range = LIR::SeqTree(comp, call); - GenTree* rangeStart = range.FirstNode(); - GenTree* rangeEnd = range.LastNode(); - - BlockRange().InsertBefore(blkNode, std::move(range)); - blkNode->gtBashToNOP(); - - LIR::Use destUse; - BlockRange().TryGetUse(destPlaceholder, &destUse); - destUse.ReplaceWith(dest); - destPlaceholder->SetUnusedValue(); - - LIR::Use dataUse; - BlockRange().TryGetUse(dataPlaceholder, &dataUse); - dataUse.ReplaceWith(data); - dataPlaceholder->SetUnusedValue(); - - LowerRange(rangeStart, rangeEnd); - - // Finally move all GT_PUTARG_* nodes - // Re-use the existing logic for CFG call args here - MoveCFGCallArgs(call); - - BlockRange().Remove(destPlaceholder); - BlockRange().Remove(dataPlaceholder); - -// Wrap with memory barriers on weak memory models -// if the block store was volatile -#ifndef TARGET_XARCH - if (isVolatile) - { - GenTree* firstBarrier = comp->gtNewMemoryBarrier(); - GenTree* secondBarrier = comp->gtNewMemoryBarrier(/*loadOnly*/ true); - BlockRange().InsertBefore(call, firstBarrier); - BlockRange().InsertAfter(call, secondBarrier); - LowerNode(firstBarrier); - LowerNode(secondBarrier); - } -#endif -} - //------------------------------------------------------------------------ // GetLoadStoreCoalescingData: given a STOREIND/IND node, get the data needed to perform // store/load coalescing including pointer to the previous node. @@ -10116,20 +10023,11 @@ void Lowering::LowerBlockStoreCommon(GenTreeBlk* blkNode) assert(blkNode->Data()->IsIntegralConst(0)); } - GenTree* src = blkNode->Data(); - // Lose the type information stored in the source - we no longer need it. - if (src->OperIs(GT_BLK)) + if (blkNode->Data()->OperIs(GT_BLK)) { - src->SetOper(GT_IND); - LowerIndir(src->AsIndir()); - } - - if ((src->OperIsIndir() && src->AsIndir()->Addr()->OperIsLocalRead() && comp->lvaTable[src->AsIndir()->Addr()->AsLclVarCommon()->GetLclNum()].lvRequiresSpecialCopy) - || (src->OperIsLocalRead() && comp->lvaTable[src->AsLclVarCommon()->GetLclNum()].lvRequiresSpecialCopy)) - { - LowerBlockStoreWithSpecialCopyHelper(blkNode); - return; + blkNode->Data()->SetOper(GT_IND); + LowerIndir(blkNode->Data()->AsIndir()); } if (TryTransformStoreObjAsStoreInd(blkNode)) diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 62b50fab0a474..5a7214822d53b 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -360,7 +360,6 @@ class Lowering final : public Phase void MarkTree(GenTree* root); void UnmarkTree(GenTree* root); GenTree* LowerStoreIndir(GenTreeStoreInd* node); - GenTree* LowerStoreIndirWithSpecialCopyHelper(GenTreeStoreInd* node); void LowerStoreIndirCoalescing(GenTreeIndir* node); GenTree* LowerAdd(GenTreeOp* node); GenTree* LowerMul(GenTreeOp* mul); @@ -372,7 +371,6 @@ class Lowering final : public Phase void LowerBlockStore(GenTreeBlk* blkNode); void LowerBlockStoreCommon(GenTreeBlk* blkNode); void LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode); - void LowerBlockStoreWithSpecialCopyHelper(GenTreeBlk* blkNode); bool TryLowerBlockStoreAsGcBulkCopyCall(GenTreeBlk* blkNode); void LowerLclHeap(GenTree* node); void ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenTree* addr, GenTree* addrParent); From f76a582603503e287e896862aefec7db7fa72ec0 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 6 Aug 2024 11:54:03 -0700 Subject: [PATCH 15/30] Implement copy-constructor handling for P/Invokes via a map back to the original arg and don't do intermediate copy-constructor calls. The bitwise copies are invalid, but there's no possible way for the user to see them. They'll always get passed a correctly copy-constructed value. --- src/coreclr/jit/compiler.cpp | 2 + src/coreclr/jit/compiler.h | 26 +++++++++++++ src/coreclr/jit/gentree.cpp | 38 +++++-------------- src/coreclr/jit/lower.cpp | 19 +++++++++- .../CopyConstructorMarshaler.cs | 9 ++--- 5 files changed, 58 insertions(+), 36 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index d94d1f7e10756..684443d32dd06 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -1989,6 +1989,8 @@ void Compiler::compInit(ArenaAllocator* pAlloc, m_fpStructLoweringCache = nullptr; #endif + m_specialCopyLocalsMap = nullptr; + // check that HelperCallProperties are initialized assert(s_helperCallProperties.IsPure(CORINFO_HELP_GET_GCSTATIC_BASE)); diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index e12f1232ef0c9..a12854884aef3 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5702,6 +5702,32 @@ class Compiler const CORINFO_SWIFT_LOWERING* GetSwiftLowering(CORINFO_CLASS_HANDLE clsHnd); #endif + // This map maps local var ids to the originally-passed-in local var that we need to use a non-bitwise copy + // from when passing out to user code. + typedef JitHashTable, unsigned> SpecialCopyLocalsMap; + SpecialCopyLocalsMap* m_specialCopyLocalsMap; + SpecialCopyLocalsMap* GetSpecialCopyLocalsMap() + { + if (m_specialCopyLocalsMap == nullptr) + { + m_specialCopyLocalsMap = new (getAllocator()) SpecialCopyLocalsMap(getAllocator()); + } + return m_specialCopyLocalsMap; + } + + bool lvaRequiresSpecialCopy(unsigned lclNum) + { + if (lvaTable[lclNum].lvRequiresSpecialCopy) + { + return true; + } + else if (m_specialCopyLocalsMap != nullptr) + { + return m_specialCopyLocalsMap->Lookup(lclNum); + } + return false; + } + void optRecordLoopMemoryDependence(GenTree* tree, BasicBlock* block, ValueNum memoryVN); void optCopyLoopMemoryDependence(GenTree* fromTree, GenTree* toTree); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 296b3670f5100..1566e129e9d69 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16607,10 +16607,12 @@ GenTree* Compiler::gtNewTempStore( if (val->OperGet() == GT_BLK && val->AsBlk()->Addr()->OperGet() == GT_LCL_VAR) { - LclVarDsc* srcVarDsc = &lvaTable[val->AsBlk()->Addr()->AsLclVar()->GetLclNum()]; + GenTreeLclVarCommon* lclVar = val->AsBlk()->Addr()->AsLclVarCommon(); + LclVarDsc* srcVarDsc = &lvaTable[lclVar->GetLclNum()]; if (srcVarDsc->lvRequiresSpecialCopy) { - varDsc->lvRequiresSpecialCopy = true; + JITDUMP("Recording V%02u as an alias of the special-copy local V%02u\n", tmp, lclVar->GetLclNum()); + GetSpecialCopyLocalsMap()->Emplace(tmp, lclVar->GetLclNum()); } } @@ -16678,36 +16680,14 @@ GenTree* Compiler::gtNewTempStore( compFloatingPointUsed = true; } - GenTree* store; - if (varDsc->lvRequiresSpecialCopy) - { - JITDUMP("Var V%02u requires special copy\n", tmp); - CORINFO_METHOD_HANDLE copyHelper = - info.compCompHnd->GetSpecialCopyHelper(varDsc->GetLayout()->GetClassHandle()); - GenTreeCall* call = gtNewCallNode(CT_USER_FUNC, copyHelper, TYP_VOID); - - GenTree* src; + GenTree* store = gtNewStoreLclVarNode(tmp, val); - assert(val->OperIs(GT_BLK)); - src = val->AsBlk()->Addr(); + // TODO-ASG: delete this zero-diff quirk. Requires some forward substitution work. + store->gtType = dstTyp; - GenTree* dst = gtNewLclVarAddrNode(tmp); - - call->gtArgs.PushBack(this, NewCallArg::Primitive(dst)); - call->gtArgs.PushBack(this, NewCallArg::Primitive(src)); - store = call; - } - else + if (varTypeIsStruct(varDsc) && !val->IsInitVal()) { - store = gtNewStoreLclVarNode(tmp, val); - - // TODO-ASG: delete this zero-diff quirk. Requires some forward substitution work. - store->gtType = dstTyp; - - if (varTypeIsStruct(varDsc) && !val->IsInitVal()) - { - store = impStoreStruct(store, curLevel, pAfterStmt, di, block); - } + store = impStoreStruct(store, curLevel, pAfterStmt, di, block); } return store; diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 88f1b1297bfdb..69924b6ffd7df 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -1537,7 +1537,22 @@ void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, GenTreeLclVar* dest = comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL); #endif - GenTree* src = comp->gtNewLclAddrNode(lclVar->GetLclNum(), lclVar->GetLclOffs(), TYP_I_IMPL); + unsigned lclNum = lclVar->GetLclNum(); + + // Use the special copy locals map to map back to the original source local we need to copy-construct + // from. This is necessary because srcLclNum might have a bitwise copy of the source, + // which we can't pass to user code. + GenTree* src; + unsigned srcLclNum; + if (comp->GetSpecialCopyLocalsMap()->Lookup(lclNum, &srcLclNum)) + { + assert(genActualType(comp->lvaGetDesc(srcLclNum)) == TYP_BYREF); + src = comp->gtNewLclVarNode(srcLclNum, TYP_I_IMPL); + } + else + { + src = comp->gtNewLclAddrNode(lclNum, lclVar->GetLclOffs(), TYP_I_IMPL); + } GenTree* destPlaceholder = comp->gtNewZeroConNode(dest->TypeGet()); GenTree* srcPlaceholder = comp->gtNewZeroConNode(genActualType(src)); @@ -1937,7 +1952,7 @@ void Lowering::LowerArg(GenTreeCall* call, CallArg* callArg, bool late) } if (putArg->OperIsPutArgStk() && arg->OperIs(GT_LCL_VAR) && - comp->lvaTable[arg->AsLclVar()->GetLclNum()].lvRequiresSpecialCopy) + comp->lvaRequiresSpecialCopy(arg->AsLclVar()->GetLclNum())) { InsertSpecialCopyArg(putArg->AsPutArgStk(), arg->AsLclVar()); } diff --git a/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs b/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs index f85128ea0be90..01d489955f091 100644 --- a/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs +++ b/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs @@ -11,7 +11,7 @@ namespace CopyConstructorMarshaler { public class CopyConstructorMarshaler { - [Fact] + //[Fact] public static int TestEntryPoint() { if(Environment.OSVersion.Platform != PlatformID.Win32NT || TestLibrary.Utilities.IsWindows7) @@ -26,15 +26,14 @@ public static int TestEntryPoint() object testInstance = Activator.CreateInstance(testType); MethodInfo testMethod = testType.GetMethod("PInvokeNumCopies"); - // On x86, we will copy when we spill before the call and copy into the final arg slot. + // On x86, we will copy in the IL stub to the final arg slot. int platformExtra = 0; if (RuntimeInformation.ProcessArchitecture == Architecture.X86) { - platformExtra = 2; + platformExtra = 1; } // PInvoke will copy once. Once from the managed to native parameter. - // On x86, we will copy when we spill before the call and copy into the final arg slot. Assert.Equal(1 + platformExtra, (int)testMethod.Invoke(testInstance, null)); testMethod = testType.GetMethod("ReversePInvokeNumCopies"); @@ -62,7 +61,7 @@ public static int TestEntryPoint() return 100; } - [Fact] + //[Fact] public static void CopyConstructorsInArgumentStackSlots() { Assembly ijwNativeDll = Assembly.Load("IjwCopyConstructorMarshaler"); From 761e648ac9faddb2f8ab636d593ee1d0e180d6a1 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 7 Aug 2024 17:25:39 -0700 Subject: [PATCH 16/30] Various PR feedback and propagate special copy directly from the IL arguments to the arg slot instead of tracing through locals. --- src/coreclr/inc/corinfo.h | 2 +- src/coreclr/inc/icorjitinfoimpl_generated.h | 2 +- src/coreclr/jit/ICorJitInfo_names_generated.h | 2 +- .../jit/ICorJitInfo_wrapper_generated.hpp | 8 +- src/coreclr/jit/compiler.cpp | 2 +- src/coreclr/jit/compiler.h | 28 +-- src/coreclr/jit/gentree.cpp | 11 -- src/coreclr/jit/gschecks.cpp | 15 +- src/coreclr/jit/lclvars.cpp | 28 +-- src/coreclr/jit/lower.cpp | 174 ++++++++++-------- src/coreclr/jit/lower.h | 3 +- .../tools/Common/JitInterface/CorInfoImpl.cs | 4 +- .../JitInterface/CorInfoImpl_generated.cs | 6 +- .../ThunkGenerator/ThunkInput.txt | 2 +- .../aot/jitinterface/jitinterface_generated.h | 6 +- .../tools/superpmi/superpmi-shared/lwmlist.h | 2 +- .../superpmi-shared/methodcontext.cpp | 22 +-- .../superpmi/superpmi-shared/methodcontext.h | 10 +- .../superpmi-shim-collector/icorjitinfo.cpp | 10 +- .../icorjitinfo_generated.cpp | 6 +- .../icorjitinfo_generated.cpp | 4 +- .../tools/superpmi/superpmi/icorjitinfo.cpp | 6 +- src/coreclr/vm/dllimport.cpp | 41 ++++- src/coreclr/vm/ilmarshalers.cpp | 4 +- src/coreclr/vm/jitinterface.cpp | 59 ++++-- src/coreclr/vm/siginfo.cpp | 29 +-- src/coreclr/vm/siginfo.hpp | 5 +- src/coreclr/vm/stubgen.h | 49 ++++- .../CopyConstructorMarshaler.cs | 6 +- 29 files changed, 316 insertions(+), 230 deletions(-) diff --git a/src/coreclr/inc/corinfo.h b/src/coreclr/inc/corinfo.h index 534d153c807c4..b0c117a0e24b6 100644 --- a/src/coreclr/inc/corinfo.h +++ b/src/coreclr/inc/corinfo.h @@ -3323,7 +3323,7 @@ class ICorDynamicInfo : public ICorStaticInfo // a register during tailcall. virtual void updateEntryPointForTailCall(CORINFO_CONST_LOOKUP* entryPoint) = 0; - virtual CORINFO_METHOD_HANDLE GetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) = 0; + virtual CORINFO_METHOD_HANDLE getSpecialCopyHelper(CORINFO_CLASS_HANDLE type) = 0; }; /**********************************************************************************/ diff --git a/src/coreclr/inc/icorjitinfoimpl_generated.h b/src/coreclr/inc/icorjitinfoimpl_generated.h index 7ef00a4afb3c5..7f9abb85bcf3d 100644 --- a/src/coreclr/inc/icorjitinfoimpl_generated.h +++ b/src/coreclr/inc/icorjitinfoimpl_generated.h @@ -741,7 +741,7 @@ uint32_t getJitFlags( CORJIT_FLAGS* flags, uint32_t sizeInBytes) override; -CORINFO_METHOD_HANDLE GetSpecialCopyHelper( +CORINFO_METHOD_HANDLE getSpecialCopyHelper( CORINFO_CLASS_HANDLE type) override; /**********************************************************************************/ diff --git a/src/coreclr/jit/ICorJitInfo_names_generated.h b/src/coreclr/jit/ICorJitInfo_names_generated.h index 07a1b4af5f5de..2d3865018d1de 100644 --- a/src/coreclr/jit/ICorJitInfo_names_generated.h +++ b/src/coreclr/jit/ICorJitInfo_names_generated.h @@ -180,6 +180,6 @@ DEF_CLR_API(recordRelocation) DEF_CLR_API(getRelocTypeHint) DEF_CLR_API(getExpectedTargetArchitecture) DEF_CLR_API(getJitFlags) -DEF_CLR_API(GetSpecialCopyHelper) +DEF_CLR_API(getSpecialCopyHelper) #undef DEF_CLR_API diff --git a/src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp b/src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp index dae8350b553b6..a5f8a961138cc 100644 --- a/src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp +++ b/src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp @@ -1741,12 +1741,12 @@ uint32_t WrapICorJitInfo::getJitFlags( return temp; } -CORINFO_METHOD_HANDLE WrapICorJitInfo::GetSpecialCopyHelper( +CORINFO_METHOD_HANDLE WrapICorJitInfo::getSpecialCopyHelper( CORINFO_CLASS_HANDLE type) { - API_ENTER(GetSpecialCopyHelper); - CORINFO_METHOD_HANDLE temp = wrapHnd->GetSpecialCopyHelper(type); - API_LEAVE(GetSpecialCopyHelper); + API_ENTER(getSpecialCopyHelper); + CORINFO_METHOD_HANDLE temp = wrapHnd->getSpecialCopyHelper(type); + API_LEAVE(getSpecialCopyHelper); return temp; } diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 684443d32dd06..eeca29a97f703 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -1989,7 +1989,7 @@ void Compiler::compInit(ArenaAllocator* pAlloc, m_fpStructLoweringCache = nullptr; #endif - m_specialCopyLocalsMap = nullptr; + m_specialCopyArgs = nullptr; // check that HelperCallProperties are initialized diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index a12854884aef3..409d1dabf49a9 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -532,8 +532,6 @@ class LclVarDsc #endif unsigned char lvPinned : 1; // is this a pinned variable? - unsigned char lvRequiresSpecialCopy : 1; // Local requires copy helper for stack copies. - unsigned char lvMustInit : 1; // must be initialized private: @@ -5702,30 +5700,20 @@ class Compiler const CORINFO_SWIFT_LOWERING* GetSwiftLowering(CORINFO_CLASS_HANDLE clsHnd); #endif - // This map maps local var ids to the originally-passed-in local var that we need to use a non-bitwise copy - // from when passing out to user code. - typedef JitHashTable, unsigned> SpecialCopyLocalsMap; - SpecialCopyLocalsMap* m_specialCopyLocalsMap; - SpecialCopyLocalsMap* GetSpecialCopyLocalsMap() + jitstd::vector* m_specialCopyArgs; + bool recordArgRequiresSpecialCopy(unsigned argNum) { - if (m_specialCopyLocalsMap == nullptr) + if (m_specialCopyArgs == nullptr) { - m_specialCopyLocalsMap = new (getAllocator()) SpecialCopyLocalsMap(getAllocator()); + m_specialCopyArgs = new (getAllocator()) jitstd::vector(getAllocator()); } - return m_specialCopyLocalsMap; + m_specialCopyArgs->push_back(argNum); + return true; } - bool lvaRequiresSpecialCopy(unsigned lclNum) + bool argRequiresSpecialCopy(unsigned argNum) { - if (lvaTable[lclNum].lvRequiresSpecialCopy) - { - return true; - } - else if (m_specialCopyLocalsMap != nullptr) - { - return m_specialCopyLocalsMap->Lookup(lclNum); - } - return false; + return m_specialCopyArgs != nullptr && std::find(m_specialCopyArgs->begin(), m_specialCopyArgs->end(), argNum) != m_specialCopyArgs->end(); } void optRecordLoopMemoryDependence(GenTree* tree, BasicBlock* block, ValueNum memoryVN); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 1566e129e9d69..ded930e34737e 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16605,17 +16605,6 @@ GenTree* Compiler::gtNewTempStore( val->gtType = valTyp; } - if (val->OperGet() == GT_BLK && val->AsBlk()->Addr()->OperGet() == GT_LCL_VAR) - { - GenTreeLclVarCommon* lclVar = val->AsBlk()->Addr()->AsLclVarCommon(); - LclVarDsc* srcVarDsc = &lvaTable[lclVar->GetLclNum()]; - if (srcVarDsc->lvRequiresSpecialCopy) - { - JITDUMP("Recording V%02u as an alias of the special-copy local V%02u\n", tmp, lclVar->GetLclNum()); - GetSpecialCopyLocalsMap()->Emplace(tmp, lclVar->GetLclNum()); - } - } - var_types dstTyp = varDsc->TypeGet(); /* If the variable's lvType is not yet set then set it here */ diff --git a/src/coreclr/jit/gschecks.cpp b/src/coreclr/jit/gschecks.cpp index 72b68b5915b97..5331d0e531e3b 100644 --- a/src/coreclr/jit/gschecks.cpp +++ b/src/coreclr/jit/gschecks.cpp @@ -516,12 +516,12 @@ void Compiler::gsParamsToShadows() continue; } - if (varDsc->lvRequiresSpecialCopy) + if (lclNum < info.compArgsCount && argRequiresSpecialCopy(lclNum) && (varDsc->TypeGet() == TYP_STRUCT)) { - JITDUMP("Var V%02u requires special copy, using special copy helper to copy to shadow var V%02u\n", lclNum, + JITDUMP("arg%02u requires special copy, using special copy helper to copy to shadow var V%02u\n", lclNum, shadowVarNum); CORINFO_METHOD_HANDLE copyHelper = - info.compCompHnd->GetSpecialCopyHelper(varDsc->GetLayout()->GetClassHandle()); + info.compCompHnd->getSpecialCopyHelper(varDsc->GetLayout()->GetClassHandle()); GenTreeCall* call = gtNewCallNode(CT_USER_FUNC, copyHelper, TYP_VOID); GenTree* src = gtNewLclVarAddrNode(lclNum); @@ -536,6 +536,15 @@ void Compiler::gsParamsToShadows() { JITDUMP( "Inserting special copy helper call at the end of the first block after Reverse P/Invoke transition\n"); + +#ifdef DEBUG + // assert that we don't have any uses of the local variable in the first block + // before we insert the shadow copy statement. + for (Statement* const stmt : fgFirstBB->Statements()) + { + assert(!gtHasRef(stmt->GetRootNode(), lclNum)); + } +#endif // If we are in a reverse P/Invoke, then we need to insert // the call at the end of the first block as we need to do the GC transition // before we can call the helper. diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 0f7d9fde3051f..a407f82a32c2b 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -326,19 +326,6 @@ void Compiler::lvaInitTypeRef() } } - if ((corInfoTypeWithMod & CORINFO_TYPE_MOD_COPY_WITH_HELPER) != 0) - { - if (corInfoType == CORINFO_TYPE_VALUECLASS || corInfoType == CORINFO_TYPE_BYREF) - { - JITDUMP("Setting lvRequiresSpecialCopy for V%02u\n", varNum); - varDsc->lvRequiresSpecialCopy = 1; - } - else - { - JITDUMP("Ignoring special copy for non-struct, non-byref V%02u\n", varNum); - } - } - varDsc->lvOnFrame = true; // The final home for this local variable might be our local stack frame if (corInfoType == CORINFO_TYPE_CLASS) @@ -668,14 +655,11 @@ void Compiler::lvaInitUserArgs(InitVarDscInfo* varDscInfo, unsigned skipArgs, un if ((corInfoType & CORINFO_TYPE_MOD_COPY_WITH_HELPER) != 0) { - if (strip(corInfoType) == CORINFO_TYPE_VALUECLASS) - { - JITDUMP("Setting lvRequiresSpecialCopy for user arg%02u\n", i); - varDsc->lvRequiresSpecialCopy = 1; - } - else + CorInfoType typeWithoutMod = strip(corInfoType); + if (typeWithoutMod == CORINFO_TYPE_VALUECLASS || typeWithoutMod == CORINFO_TYPE_PTR) { - JITDUMP("Ignoring special copy for non-struct user arg%02u\n", i); + JITDUMP("Marking user arg%02u as requiring special copy semantics\n", i); + recordArgRequiresSpecialCopy(i); } } @@ -7618,10 +7602,6 @@ void Compiler::lvaDumpEntry(unsigned lclNum, FrameLayoutState curState, size_t r { printf(" pinned"); } - if (varDsc->lvRequiresSpecialCopy) - { - printf(" special-copy"); - } if (varDsc->lvClassHnd != NO_CLASS_HANDLE) { printf(" class-hnd"); diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 69924b6ffd7df..a3d82d3aec09c 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -1523,77 +1523,6 @@ void Lowering::ReplaceArgWithPutArgOrBitcast(GenTree** argSlot, GenTree* putArgO BlockRange().InsertAfter(arg, putArgOrBitcast); } -void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, GenTreeLclVar* lclVar) -{ - assert(putArgStk != nullptr); - assert(lclVar != nullptr); - - GenTree* dest; - -#if FEATURE_FIXED_OUT_ARGS - dest = comp->gtNewOperNode(GT_ADD, TYP_I_IMPL, comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL), - comp->gtNewIconNode(putArgStk->getArgOffset())); -#else - dest = comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL); -#endif - - unsigned lclNum = lclVar->GetLclNum(); - - // Use the special copy locals map to map back to the original source local we need to copy-construct - // from. This is necessary because srcLclNum might have a bitwise copy of the source, - // which we can't pass to user code. - GenTree* src; - unsigned srcLclNum; - if (comp->GetSpecialCopyLocalsMap()->Lookup(lclNum, &srcLclNum)) - { - assert(genActualType(comp->lvaGetDesc(srcLclNum)) == TYP_BYREF); - src = comp->gtNewLclVarNode(srcLclNum, TYP_I_IMPL); - } - else - { - src = comp->gtNewLclAddrNode(lclNum, lclVar->GetLclOffs(), TYP_I_IMPL); - } - - GenTree* destPlaceholder = comp->gtNewZeroConNode(dest->TypeGet()); - GenTree* srcPlaceholder = comp->gtNewZeroConNode(genActualType(src)); - - GenTreeCall* call = - comp->gtNewCallNode(CT_USER_FUNC, - comp->info.compCompHnd->GetSpecialCopyHelper(lclVar->GetLayout(comp)->GetClassHandle()), - TYP_VOID); - - call->gtArgs.PushBack(comp, NewCallArg::Primitive(destPlaceholder)); - call->gtArgs.PushBack(comp, NewCallArg::Primitive(srcPlaceholder)); - - comp->fgMorphArgs(call); - - LIR::Range callRange = LIR::SeqTree(comp, call); - GenTree* callRangeStart = callRange.FirstNode(); - GenTree* callRangeEnd = callRange.LastNode(); - - BlockRange().InsertAfter(putArgStk, std::move(callRange)); - BlockRange().InsertAfter(putArgStk, dest); - BlockRange().InsertAfter(putArgStk, src); - - LIR::Use destUse; - LIR::Use srcUse; - BlockRange().TryGetUse(destPlaceholder, &destUse); - BlockRange().TryGetUse(srcPlaceholder, &srcUse); - destUse.ReplaceWith(dest); - srcUse.ReplaceWith(src); - destPlaceholder->SetUnusedValue(); - srcPlaceholder->SetUnusedValue(); - - LowerRange(callRangeStart, callRangeEnd); - - // Finally move all GT_PUTARG_* nodes - // Re-use the existing logic for CFG call args here - MoveCFGCallArgs(call); - - BlockRange().Remove(destPlaceholder); - BlockRange().Remove(srcPlaceholder); -} - //------------------------------------------------------------------------ // NewPutArg: rewrites the tree to put an arg in a register or on the stack. // @@ -1950,12 +1879,6 @@ void Lowering::LowerArg(GenTreeCall* call, CallArg* callArg, bool late) { ReplaceArgWithPutArgOrBitcast(ppArg, putArg); } - - if (putArg->OperIsPutArgStk() && arg->OperIs(GT_LCL_VAR) && - comp->lvaRequiresSpecialCopy(arg->AsLclVar()->GetLclNum())) - { - InsertSpecialCopyArg(putArg->AsPutArgStk(), arg->AsLclVar()); - } } arg = *ppArg; @@ -2071,9 +1994,106 @@ void Lowering::LowerArgsForCall(GenTreeCall* call) LowerArg(call, &arg, true); } + LowerSpecialCopyArgs(call); + LegalizeArgPlacement(call); } +void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) +{ + // We only need to use the special copy helper on P/Invoke IL stubs + // for the unmanaged call. + if (comp->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB) && comp->compMethodRequiresPInvokeFrame() && call->IsUnmanaged()) + { + unsigned argIndex = 0; + for (CallArg& arg : call->gtArgs.Args()) + { + if (!arg.IsUserArg()) + { + continue; + } + + if (argIndex >= comp->info.compArgsCount) + { + break; + } + + // check if parameter at the same index as the IL argument is marked as requiring special copy, assuming that it is being passed 1:1 to the pinvoke + unsigned paramIndex = comp->compMap2ILvarNum(argIndex); + if ((paramIndex != ICorDebugInfo::UNKNOWN_ILNUM) && comp->argRequiresSpecialCopy(paramIndex)) + { + assert(arg.GetNode()->OperIs(GT_PUTARG_STK)); + InsertSpecialCopyArg(arg.GetNode()->AsPutArgStk(), arg.GetSignatureClassHandle(), paramIndex); + } + argIndex++; + } + } +} + +void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, CORINFO_CLASS_HANDLE argType, unsigned lclNum) +{ + assert(putArgStk != nullptr); + GenTree* dest; + +#if FEATURE_FIXED_OUT_ARGS + dest = comp->gtNewOperNode(GT_ADD, TYP_I_IMPL, comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL), + comp->gtNewIconNode(putArgStk->getArgOffset())); +#else + dest = comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL); +#endif + + GenTree* src; + var_types lclType = genActualType(comp->lvaGetDesc(lclNum)); + if (lclType == TYP_BYREF || lclType == TYP_I_IMPL) + { + src = comp->gtNewLclVarNode(lclNum, TYP_I_IMPL); + } + else + { + assert(lclType == TYP_STRUCT); + src = comp->gtNewLclAddrNode(lclNum, 0, TYP_I_IMPL); + } + + GenTree* destPlaceholder = comp->gtNewZeroConNode(dest->TypeGet()); + GenTree* srcPlaceholder = comp->gtNewZeroConNode(genActualType(src)); + + GenTreeCall* call = + comp->gtNewCallNode(CT_USER_FUNC, + comp->info.compCompHnd->getSpecialCopyHelper(argType), + TYP_VOID); + + call->gtArgs.PushBack(comp, NewCallArg::Primitive(destPlaceholder)); + call->gtArgs.PushBack(comp, NewCallArg::Primitive(srcPlaceholder)); + + comp->fgMorphArgs(call); + + LIR::Range callRange = LIR::SeqTree(comp, call); + GenTree* callRangeStart = callRange.FirstNode(); + GenTree* callRangeEnd = callRange.LastNode(); + + BlockRange().InsertAfter(putArgStk, std::move(callRange)); + BlockRange().InsertAfter(putArgStk, dest); + BlockRange().InsertAfter(putArgStk, src); + + LIR::Use destUse; + LIR::Use srcUse; + BlockRange().TryGetUse(destPlaceholder, &destUse); + BlockRange().TryGetUse(srcPlaceholder, &srcUse); + destUse.ReplaceWith(dest); + srcUse.ReplaceWith(src); + destPlaceholder->SetUnusedValue(); + srcPlaceholder->SetUnusedValue(); + + LowerRange(callRangeStart, callRangeEnd); + + // Finally move all GT_PUTARG_* nodes + // Re-use the existing logic for CFG call args here + MoveCFGCallArgs(call); + + BlockRange().Remove(destPlaceholder); + BlockRange().Remove(srcPlaceholder); +} + //------------------------------------------------------------------------ // LegalizeArgPlacement: Move arg placement nodes (PUTARG_*) into a legal // ordering after they have been created. diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 5a7214822d53b..efe7e11a49df7 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -183,7 +183,8 @@ class Lowering final : public Phase GenTree* LowerVirtualStubCall(GenTreeCall* call); void LowerArgsForCall(GenTreeCall* call); void ReplaceArgWithPutArgOrBitcast(GenTree** ppChild, GenTree* newNode); - void InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, GenTreeLclVar* lclVar); + void LowerSpecialCopyArgs(GenTreeCall* call); + void InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, CORINFO_CLASS_HANDLE argType, unsigned lclNum); GenTree* NewPutArg(GenTreeCall* call, GenTree* arg, CallArg* callArg, var_types type); void LowerArg(GenTreeCall* call, CallArg* callArg, bool late); #if defined(TARGET_ARMARCH) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs index 48490a22cdd53..37776770260db 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs @@ -4456,9 +4456,9 @@ private static bool TryReadRvaFieldData(FieldDesc field, byte* buffer, int buffe return false; } - private CORINFO_METHOD_STRUCT_* GetSpecialCopyHelper(CORINFO_CLASS_STRUCT_* type) + private CORINFO_METHOD_STRUCT_* getSpecialCopyHelper(CORINFO_CLASS_STRUCT_* type) { - throw new NotImplementedException("GetSpecialCopyHelper"); + throw new NotImplementedException("getSpecialCopyHelper"); } } } diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs b/src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs index 2ffc5d666cd84..9699622790e92 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs @@ -2606,12 +2606,12 @@ private static uint _getJitFlags(IntPtr thisHandle, IntPtr* ppException, CORJIT_ } [UnmanagedCallersOnly] - private static CORINFO_METHOD_STRUCT_* _GetSpecialCopyHelper(IntPtr thisHandle, IntPtr* ppException, CORINFO_CLASS_STRUCT_* type) + private static CORINFO_METHOD_STRUCT_* _getSpecialCopyHelper(IntPtr thisHandle, IntPtr* ppException, CORINFO_CLASS_STRUCT_* type) { var _this = GetThis(thisHandle); try { - return _this.GetSpecialCopyHelper(type); + return _this.getSpecialCopyHelper(type); } catch (Exception ex) { @@ -2801,7 +2801,7 @@ private static IntPtr GetUnmanagedCallbacks() callbacks[173] = (delegate* unmanaged)&_getRelocTypeHint; callbacks[174] = (delegate* unmanaged)&_getExpectedTargetArchitecture; callbacks[175] = (delegate* unmanaged)&_getJitFlags; - callbacks[176] = (delegate* unmanaged)&_GetSpecialCopyHelper; + callbacks[176] = (delegate* unmanaged)&_getSpecialCopyHelper; return (IntPtr)callbacks; } diff --git a/src/coreclr/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txt b/src/coreclr/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txt index 3246d7c65b88f..b7940a5e9596b 100644 --- a/src/coreclr/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txt +++ b/src/coreclr/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txt @@ -340,4 +340,4 @@ FUNCTIONS uint16_t getRelocTypeHint(void* target) uint32_t getExpectedTargetArchitecture() uint32_t getJitFlags(CORJIT_FLAGS* flags, uint32_t sizeInBytes) - CORINFO_METHOD_HANDLE GetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) = 0; + CORINFO_METHOD_HANDLE getSpecialCopyHelper(CORINFO_CLASS_HANDLE type) = 0; diff --git a/src/coreclr/tools/aot/jitinterface/jitinterface_generated.h b/src/coreclr/tools/aot/jitinterface/jitinterface_generated.h index 96e869cb86fd2..b9e3df49aef92 100644 --- a/src/coreclr/tools/aot/jitinterface/jitinterface_generated.h +++ b/src/coreclr/tools/aot/jitinterface/jitinterface_generated.h @@ -187,7 +187,7 @@ struct JitInterfaceCallbacks uint16_t (* getRelocTypeHint)(void * thisHandle, CorInfoExceptionClass** ppException, void* target); uint32_t (* getExpectedTargetArchitecture)(void * thisHandle, CorInfoExceptionClass** ppException); uint32_t (* getJitFlags)(void * thisHandle, CorInfoExceptionClass** ppException, CORJIT_FLAGS* flags, uint32_t sizeInBytes); - CORINFO_METHOD_HANDLE (* GetSpecialCopyHelper)(void * thisHandle, CorInfoExceptionClass** ppException, CORINFO_CLASS_HANDLE type); + CORINFO_METHOD_HANDLE (* getSpecialCopyHelper)(void * thisHandle, CorInfoExceptionClass** ppException, CORINFO_CLASS_HANDLE type); }; @@ -1920,11 +1920,11 @@ class JitInterfaceWrapper : public ICorJitInfo return temp; } - virtual CORINFO_METHOD_HANDLE GetSpecialCopyHelper( + virtual CORINFO_METHOD_HANDLE getSpecialCopyHelper( CORINFO_CLASS_HANDLE type) { CorInfoExceptionClass* pException = nullptr; - CORINFO_METHOD_HANDLE temp = _callbacks->GetSpecialCopyHelper(_thisHandle, &pException, type); + CORINFO_METHOD_HANDLE temp = _callbacks->getSpecialCopyHelper(_thisHandle, &pException, type); if (pException != nullptr) throw pException; return temp; } diff --git a/src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h b/src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h index e00f86d1b6458..9c6c591fb8d40 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h +++ b/src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h @@ -131,7 +131,7 @@ LWM(GetSwiftLowering, DWORDLONG, Agnostic_GetSwiftLowering) LWM(GetFpStructLowering, DWORDLONG, Agnostic_GetFpStructLowering) LWM(GetTailCallHelpers, Agnostic_GetTailCallHelpers, Agnostic_CORINFO_TAILCALL_HELPERS) LWM(UpdateEntryPointForTailCall, Agnostic_CORINFO_CONST_LOOKUP, Agnostic_CORINFO_CONST_LOOKUP) -LWM(GetSpecialCopyHelper, DWORDLONG, DWORDLONG) +LWM(getSpecialCopyHelper, DWORDLONG, DWORDLONG) LWM(GetThreadTLSIndex, DWORD, DLD) LWM(GetTokenTypeAsHandle, GetTokenTypeAsHandleValue, DWORDLONG) LWM(GetTypeForBox, DWORDLONG, DWORDLONG) diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp index 9f9497f166feb..29b0e5615d551 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp @@ -3693,7 +3693,7 @@ void MethodContext::recGetThreadLocalStaticBlocksInfo(CORINFO_THREAD_STATIC_BLOC void MethodContext::dmpGetThreadLocalStaticBlocksInfo(DWORD key, const Agnostic_GetThreadLocalStaticBlocksInfo& value) { printf("GetThreadLocalStaticBlocksInfo key %u, tlsIndex-%s, " - ", tlsGetAddrFtnPtr-%016" PRIX64 ", tlsIndexObject - %016" PRIX64 + ", tlsGetAddrFtnPtr-%016" PRIX64 ", tlsIndexObject - %016" PRIX64 ", threadVarsSection - %016" PRIX64 ", offsetOfThreadLocalStoragePointer-%u" ", offsetOfMaxThreadStaticBlocks-%u" @@ -7187,30 +7187,30 @@ const WCHAR* MethodContext::repGetStringConfigValue(const WCHAR* name) return value; } -void MethodContext::recGetSpecialCopyHelper(CORINFO_CLASS_HANDLE type, CORINFO_METHOD_HANDLE helper) +void MethodContext::recgetSpecialCopyHelper(CORINFO_CLASS_HANDLE type, CORINFO_METHOD_HANDLE helper) { - if (GetSpecialCopyHelper == nullptr) - GetSpecialCopyHelper = new LightWeightMap(); + if (getSpecialCopyHelper == nullptr) + getSpecialCopyHelper = new LightWeightMap(); DWORDLONG key; ZeroMemory(&key, sizeof(key)); // Zero key including any struct padding key = CastHandle(type); DWORDLONG value = CastHandle(helper); - GetSpecialCopyHelper->Add(key, value); - DEBUG_REC(dmpGetSpecialCopyHelper(key, value)); + getSpecialCopyHelper->Add(key, value); + DEBUG_REC(dmpgetSpecialCopyHelper(key, value)); } -void MethodContext::dmpGetSpecialCopyHelper(DWORDLONG key, DWORDLONG value) +void MethodContext::dmpgetSpecialCopyHelper(DWORDLONG key, DWORDLONG value) { - printf("GetSpecialCopyHelper key %016" PRIX64 ", value %016" PRIX64 "", key, value); + printf("getSpecialCopyHelper key %016" PRIX64 ", value %016" PRIX64 "", key, value); } -CORINFO_METHOD_HANDLE MethodContext::repGetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) +CORINFO_METHOD_HANDLE MethodContext::repgetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) { DWORDLONG key = CastHandle(type); - DWORDLONG value = LookupByKeyOrMiss(GetSpecialCopyHelper, key, ": key %016" PRIX64 "", key); - DEBUG_REP(dmpGetSpecialCopyHelper(key, value)); + DWORDLONG value = LookupByKeyOrMiss(getSpecialCopyHelper, key, ": key %016" PRIX64 "", key); + DEBUG_REP(dmpgetSpecialCopyHelper(key, value)); return (CORINFO_METHOD_HANDLE)value; } diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h index accbdf4c25ed9..4608c251f7bd9 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h @@ -587,7 +587,7 @@ class MethodContext void recGetIsClassInitedFlagAddress(CORINFO_CLASS_HANDLE cls, CORINFO_CONST_LOOKUP* addr, int* offset, bool result); void dmpGetIsClassInitedFlagAddress(DWORDLONG key, const Agnostic_GetIsClassInitedFlagAddress& value); bool repGetIsClassInitedFlagAddress(CORINFO_CLASS_HANDLE cls, CORINFO_CONST_LOOKUP* addr, int* offset); - + void recGetStaticBaseAddress(CORINFO_CLASS_HANDLE cls, bool isGc, CORINFO_CONST_LOOKUP* addr, bool result); void dmpGetStaticBaseAddress(DLD key, const Agnostic_GetStaticBaseAddress& value); bool repGetStaticBaseAddress(CORINFO_CLASS_HANDLE cls, bool isGc, CORINFO_CONST_LOOKUP* addr); @@ -896,9 +896,9 @@ class MethodContext void dmpGetStringConfigValue(DWORD nameIndex, DWORD result); const WCHAR* repGetStringConfigValue(const WCHAR* name); - void recGetSpecialCopyHelper(CORINFO_CLASS_HANDLE type, CORINFO_METHOD_HANDLE helper); - void dmpGetSpecialCopyHelper(DWORDLONG key, DWORDLONG value); - CORINFO_METHOD_HANDLE repGetSpecialCopyHelper(CORINFO_CLASS_HANDLE type); + void recgetSpecialCopyHelper(CORINFO_CLASS_HANDLE type, CORINFO_METHOD_HANDLE helper); + void dmpgetSpecialCopyHelper(DWORDLONG key, DWORDLONG value); + CORINFO_METHOD_HANDLE repgetSpecialCopyHelper(CORINFO_CLASS_HANDLE type); void dmpSigInstHandleMap(DWORD key, DWORDLONG value); @@ -1191,7 +1191,7 @@ enum mcPackets Packet_GetTypeForBoxOnStack = 221, Packet_GetTypeDefinition = 222, Packet_GetFpStructLowering = 223, - Packet_GetSpecialCopyHelper = 224, + Packet_getSpecialCopyHelper = 224, }; void SetDebugDumpVariables(); diff --git a/src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp b/src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp index d82bf7f853170..907de5e4b21f8 100644 --- a/src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp @@ -2028,10 +2028,10 @@ bool interceptor_ICJI::notifyInstructionSetUsage(CORINFO_InstructionSet instruct return original_ICorJitInfo->notifyInstructionSetUsage(instructionSet, supported); } -CORINFO_METHOD_HANDLE interceptor_ICJI::GetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) +CORINFO_METHOD_HANDLE interceptor_ICJI::getSpecialCopyHelper(CORINFO_CLASS_HANDLE type) { - mc->cr->AddCall("GetSpecialCopyHelper"); - CORINFO_METHOD_HANDLE temp = original_ICorJitInfo->GetSpecialCopyHelper(type); - mc->recGetSpecialCopyHelper(type, temp); + mc->cr->AddCall("getSpecialCopyHelper"); + CORINFO_METHOD_HANDLE temp = original_ICorJitInfo->getSpecialCopyHelper(type); + mc->recgetSpecialCopyHelper(type, temp); return temp; -} \ No newline at end of file +} diff --git a/src/coreclr/tools/superpmi/superpmi-shim-counter/icorjitinfo_generated.cpp b/src/coreclr/tools/superpmi/superpmi-shim-counter/icorjitinfo_generated.cpp index 33147cb85fef7..2551ee3baf4f5 100644 --- a/src/coreclr/tools/superpmi/superpmi-shim-counter/icorjitinfo_generated.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shim-counter/icorjitinfo_generated.cpp @@ -1436,10 +1436,10 @@ uint32_t interceptor_ICJI::getJitFlags( return original_ICorJitInfo->getJitFlags(flags, sizeInBytes); } -CORINFO_METHOD_HANDLE interceptor_ICJI::GetSpecialCopyHelper( +CORINFO_METHOD_HANDLE interceptor_ICJI::getSpecialCopyHelper( CORINFO_CLASS_HANDLE type) { - mcs->AddCall("GetSpecialCopyHelper"); - return original_ICorJitInfo->GetSpecialCopyHelper(type); + mcs->AddCall("getSpecialCopyHelper"); + return original_ICorJitInfo->getSpecialCopyHelper(type); } diff --git a/src/coreclr/tools/superpmi/superpmi-shim-simple/icorjitinfo_generated.cpp b/src/coreclr/tools/superpmi/superpmi-shim-simple/icorjitinfo_generated.cpp index 4269f5d5b6e5b..d0cb9e09d1fc3 100644 --- a/src/coreclr/tools/superpmi/superpmi-shim-simple/icorjitinfo_generated.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shim-simple/icorjitinfo_generated.cpp @@ -1260,9 +1260,9 @@ uint32_t interceptor_ICJI::getJitFlags( return original_ICorJitInfo->getJitFlags(flags, sizeInBytes); } -CORINFO_METHOD_HANDLE interceptor_ICJI::GetSpecialCopyHelper( +CORINFO_METHOD_HANDLE interceptor_ICJI::getSpecialCopyHelper( CORINFO_CLASS_HANDLE type) { - return original_ICorJitInfo->GetSpecialCopyHelper(type); + return original_ICorJitInfo->getSpecialCopyHelper(type); } diff --git a/src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp b/src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp index f21a5a26cca19..1d8d9c3a3b429 100644 --- a/src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp +++ b/src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp @@ -1864,9 +1864,9 @@ uint32_t MyICJI::getExpectedTargetArchitecture() return result; } -CORINFO_METHOD_HANDLE MyICJI::GetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) +CORINFO_METHOD_HANDLE MyICJI::getSpecialCopyHelper(CORINFO_CLASS_HANDLE type) { - jitInstance->mc->cr->AddCall("GetSpecialCopyHelper"); - CORINFO_METHOD_HANDLE result = jitInstance->mc->repGetSpecialCopyHelper(type); + jitInstance->mc->cr->AddCall("getSpecialCopyHelper"); + CORINFO_METHOD_HANDLE result = jitInstance->mc->repgetSpecialCopyHelper(type); return result; } diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp index cce391b42c302..285e06ee2a131 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -408,7 +408,7 @@ class ILStubState : public StubState { SigPointer sigPtr(pStubMD->GetSig()); - sigPtr.ConvertToInternalSignature(pStubMD->GetModule(), NULL, &sigBuilder); + sigPtr.ConvertToInternalSignature(pStubMD->GetModule(), NULL, &sigBuilder, GetTokenLookupMap()); } // @@ -464,6 +464,25 @@ class ILStubState : public StubState cbTempModuleIndependentSigLength); } + void ConvertMethodDescSigToModuleIndependentSig(MethodDesc* pStubMD) + { + SigBuilder sigBuilder; + SigPointer sigPtr(pStubMD->GetSig()); + sigPtr.ConvertToInternalSignature(pStubMD->GetModule(), NULL, &sigBuilder, GetTokenLookupMap()); + + // + // make a domain-local copy of the sig so that this state can outlive the + // compile time state. + // + DWORD cbNewSig = sigBuilder.GetSignatureLength(); + PVOID pNewSigBuffer = sigBuilder.GetSignature(&cbNewSig); + PCCOR_SIGNATURE pNewSig = (PCCOR_SIGNATURE)(void *)pStubMD->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(cbNewSig)); + + memcpyNoGCRefs((void *)pNewSig, pNewSigBuffer, cbNewSig); + + pStubMD->AsDynamicMethodDesc()->SetStoredMethodSig(pNewSig, cbNewSig); + } + void EmitInvokeTarget(MethodDesc *pStubMD) { STANDARD_VM_CONTRACT; @@ -799,8 +818,17 @@ class ILStubState : public StubState if (SF_IsReverseStub(m_dwStubFlags)) { + // If we're in a Reverse stub, the target signature we've built + // is the signature of the stub, and the current signature of the stub + // is the target signature. We need to swap them. SwapStubSignatures(pStubMD); } + else + { + // If we're not in a Reverse stub, the signatures are correct, + // but we need to convert the signature into a module-independent form. + ConvertMethodDescSigToModuleIndependentSig(pStubMD); + } ILCodeLabel* pTryBeginLabel = nullptr; ILCodeLabel* pTryEndAndCatchBeginLabel = nullptr; @@ -3968,6 +3996,7 @@ namespace DWORD m_StubFlags; INT32 m_iLCIDArg; + INT32 m_tokenMapHash; INT32 m_nParams; BYTE m_rgbSigAndParamData[1]; // (dwParamAttr, cbNativeType) // length: number of parameters @@ -4023,8 +4052,11 @@ namespace // note that ConvertToInternalSignature also resolves generics so different instantiations will get different // hash blobs for methods that have generic parameters in their signature + // The signature may have custom modifiers, so provide a token lookup map to resolve them. + // We'll include a hash of the token lookup map in the hash blob. + TokenLookupMap tokenLookupMap; SigBuilder sigBuilder; - sigPtr.ConvertToInternalSignature(pParams->m_pModule, pParams->m_pTypeContext, &sigBuilder, /* bSkipCustomModifier = */ FALSE); + sigPtr.ConvertToInternalSignature(pParams->m_pModule, pParams->m_pTypeContext, &sigBuilder, &tokenLookupMap); DWORD cbSig; PVOID pSig = sigBuilder.GetSignature(&cbSig); @@ -4059,6 +4091,7 @@ namespace pBlob->m_nlFlags = static_cast(pParams->m_nlFlags & ~nlfNoMangle); // this flag does not affect the stub pBlob->m_iLCIDArg = pParams->m_iLCIDArg; + pBlob->m_tokenMapHash = tokenLookupMap.GetHashValue(); pBlob->m_StubFlags = pParams->m_dwStubFlags; pBlob->m_nParams = pParams->m_nParamTokens; @@ -6400,7 +6433,7 @@ bool GenerateCopyConstructorHelper(MethodDesc* ftn, TypeHandle type, DynamicReso { if (!type.IsValueType()) return false; - + MethodTable * pMT = type.AsMethodTable(); MethodDesc* pCopyCtor = nullptr; @@ -6421,7 +6454,7 @@ bool GenerateCopyConstructorHelper(MethodDesc* ftn, TypeHandle type, DynamicReso &genericContext, ftn, ILSTUB_LINKER_FLAG_NONE); - + ILCodeStream* pCode = sl.NewCodeStream(ILStubLinker::kDispatch); pCode->EmitLDARG(0); diff --git a/src/coreclr/vm/ilmarshalers.cpp b/src/coreclr/vm/ilmarshalers.cpp index fb468aeb4f186..bd89623a36340 100644 --- a/src/coreclr/vm/ilmarshalers.cpp +++ b/src/coreclr/vm/ilmarshalers.cpp @@ -3424,8 +3424,8 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver LocalDesc locDesc(pargs->mm.m_pMT); pslIL->SetStubTargetArgType(&locDesc); // native type is the value type - locDesc.MakeByRef(); locDesc.AddModifier(true, pslIL->GetToken(pargs->mm.m_pSigMod)); + locDesc.MakeByRef(); locDesc.MakePinned(); @@ -3437,7 +3437,7 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver pslIL->EmitLDARG(argidx); pslIL->EmitSTLOC(dwPinnedArgLocal); - pslILDispatch->EmitLDLOC(dwPinnedArgLocal); + pslILDispatch->EmitLDARG(argidx); pslILDispatch->EmitLDOBJ(pslIL->GetToken(pargs->mm.m_pMT)); #else LocalDesc locDesc(pargs->mm.m_pMT); diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 6e4fa79ff124f..b74034ee8d683 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -7630,7 +7630,7 @@ static void getMethodInfoHelper( _ASSERTE(inst.GetNumArgs() == 1); TypeHandle type = inst[0]; - + if (!GenerateCopyConstructorHelper(ftn, type, &cxt.TransientResolver, &cxt.Header, methInfo)) { ThrowHR(COR_E_BADIMAGEFORMAT); @@ -9427,6 +9427,32 @@ CORINFO_ARG_LIST_HANDLE CEEInfo::getArgNext(CORINFO_ARG_LIST_HANDLE args) /*********************************************************************/ +namespace +{ + bool HasCopyConstructorModifier(SigPointer sig, CORINFO_MODULE_HANDLE scope) + { + if (IsDynamicScope(scope)) + { + DynamicResolver* pResolver = GetDynamicResolver(scope); + if (sig.HasCustomModifier(pResolver, "Microsoft.VisualC.NeedsCopyConstructorModifier", ELEMENT_TYPE_CMOD_REQD) || + sig.HasCustomModifier(pResolver, "System.Runtime.CompilerServices.IsCopyConstructed", ELEMENT_TYPE_CMOD_REQD)) + { + return true; + } + } + else + { + Module* pModule = GetModule(scope); + if (sig.HasCustomModifier(pModule, "Microsoft.VisualC.NeedsCopyConstructorModifier", ELEMENT_TYPE_CMOD_REQD) || + sig.HasCustomModifier(pModule, "System.Runtime.CompilerServices.IsCopyConstructed", ELEMENT_TYPE_CMOD_REQD)) + { + return true; + } + } + return false; + } +} + CorInfoTypeWithMod CEEInfo::getArgType ( CORINFO_SIG_INFO* sig, CORINFO_ARG_LIST_HANDLE args, @@ -9459,24 +9485,9 @@ CorInfoTypeWithMod CEEInfo::getArgType ( Module* pModule = GetModule(sig->scope); - if (!IsDynamicScope(sig->scope)) - { - SigPointer sigtmp = ptr; - if (sigtmp.HasCustomModifier(pModule, "Microsoft.VisualC.NeedsCopyConstructorModifier", ELEMENT_TYPE_CMOD_REQD) || - sigtmp.HasCustomModifier(pModule, "System.Runtime.CompilerServices.IsCopyConstructed", ELEMENT_TYPE_CMOD_REQD)) - { - result = (CorInfoTypeWithMod)((int)result | CORINFO_TYPE_MOD_COPY_WITH_HELPER); - } - } - else + if (HasCopyConstructorModifier(ptr, sig->scope)) { - SigPointer sigtmp = ptr; - DynamicResolver* pResolver = GetDynamicResolver(sig->scope); - if (sigtmp.HasCustomModifier(pResolver, "Microsoft.VisualC.NeedsCopyConstructorModifier", ELEMENT_TYPE_CMOD_REQD) || - sigtmp.HasCustomModifier(pResolver, "System.Runtime.CompilerServices.IsCopyConstructed", ELEMENT_TYPE_CMOD_REQD)) - { - result = (CorInfoTypeWithMod)((int)result | CORINFO_TYPE_MOD_COPY_WITH_HELPER); - } + result = CorInfoTypeWithMod((int)result | CORINFO_TYPE_MOD_COPY_WITH_HELPER); } // Now read off the "real" element type after taking any instantiations into consideration @@ -9520,6 +9531,14 @@ CorInfoTypeWithMod CEEInfo::getArgType ( classMustBeLoadedBeforeCodeIsRun(CORINFO_CLASS_HANDLE(thPtr.AsPtr())); } } + FALLTHROUGH; + + case ELEMENT_TYPE_BYREF: + IfFailThrow(ptr.GetElemType(NULL)); + if (HasCopyConstructorModifier(ptr, sig->scope)) + { + result = CorInfoTypeWithMod((int)result | CORINFO_TYPE_MOD_COPY_WITH_HELPER); + } break; case ELEMENT_TYPE_VOID: @@ -9972,7 +9991,7 @@ void CEEInfo::getAddressOfPInvokeTarget(CORINFO_METHOD_HANDLE method, EE_TO_JIT_TRANSITION(); } -CORINFO_METHOD_HANDLE CEEInfo::GetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) +CORINFO_METHOD_HANDLE CEEInfo::getSpecialCopyHelper(CORINFO_CLASS_HANDLE type) { CONTRACTL { THROWS; @@ -9997,7 +10016,7 @@ CORINFO_METHOD_HANDLE CEEInfo::GetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) Instantiation(&th, 1), FALSE, FALSE); - + pHelperMD->CheckRestore(); result = (CORINFO_METHOD_HANDLE)pHelperMD; diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index 3a7cfadd196fa..f1adff6d4f3ad 100644 --- a/src/coreclr/vm/siginfo.cpp +++ b/src/coreclr/vm/siginfo.cpp @@ -137,7 +137,7 @@ unsigned GetSizeForCorElementType(CorElementType etyp) #ifndef DACCESS_COMPILE -void SigPointer::ConvertToInternalExactlyOne(Module* pSigModule, SigTypeContext *pTypeContext, SigBuilder * pSigBuilder, BOOL bSkipCustomModifier) +void SigPointer::ConvertToInternalExactlyOne(Module* pSigModule, SigTypeContext *pTypeContext, SigBuilder * pSigBuilder, TokenLookupMap* pTokenLookupMap) { CONTRACTL { @@ -152,9 +152,10 @@ void SigPointer::ConvertToInternalExactlyOne(Module* pSigModule, SigTypeContext CorElementType typ = ELEMENT_TYPE_END; - // Check whether we need to skip custom modifier - // Only preserve custom modifier when calculating IL stub hash blob - if (bSkipCustomModifier) + // If we don't have a token lookup map, skip custom modifiers. + // We can't accurately represent them in the internal signature unless we can + // resolve tokens through a token lookup map. + if (pTokenLookupMap == nullptr) { // GetElemType eats sentinel and custom modifiers IfFailThrowBF(GetElemType(&typ), BFA_BAD_COMPLUS_SIG, pSigModule); @@ -230,16 +231,16 @@ void SigPointer::ConvertToInternalExactlyOne(Module* pSigModule, SigTypeContext case ELEMENT_TYPE_PTR: case ELEMENT_TYPE_PINNED: case ELEMENT_TYPE_SZARRAY: - ConvertToInternalExactlyOne(pSigModule, pTypeContext, pSigBuilder, bSkipCustomModifier); + ConvertToInternalExactlyOne(pSigModule, pTypeContext, pSigBuilder, pTokenLookupMap); break; case ELEMENT_TYPE_FNPTR: - ConvertToInternalSignature(pSigModule, pTypeContext, pSigBuilder, bSkipCustomModifier); + ConvertToInternalSignature(pSigModule, pTypeContext, pSigBuilder, pTokenLookupMap); break; case ELEMENT_TYPE_ARRAY: { - ConvertToInternalExactlyOne(pSigModule, pTypeContext, pSigBuilder, bSkipCustomModifier); + ConvertToInternalExactlyOne(pSigModule, pTypeContext, pSigBuilder, pTokenLookupMap); uint32_t rank = 0; // Get rank IfFailThrowBF(GetData(&rank), BFA_BAD_COMPLUS_SIG, pSigModule); @@ -302,7 +303,7 @@ void SigPointer::ConvertToInternalExactlyOne(Module* pSigModule, SigTypeContext while (argCnt--) { - ConvertToInternalExactlyOne(pSigModule, pTypeContext, pSigBuilder, bSkipCustomModifier); + ConvertToInternalExactlyOne(pSigModule, pTypeContext, pSigBuilder, pTokenLookupMap); } } break; @@ -311,20 +312,20 @@ void SigPointer::ConvertToInternalExactlyOne(Module* pSigModule, SigTypeContext case ELEMENT_TYPE_CMOD_OPT: case ELEMENT_TYPE_CMOD_REQD: { + _ASSERTE_MSG(pTokenLookupMap != nullptr, "A token lookup map must be provided for custom modifiers"); mdToken tk; IfFailThrowBF(GetToken(&tk), BFA_BAD_COMPLUS_SIG, pSigModule); TypeHandle th = ClassLoader::LoadTypeDefOrRefThrowing(pSigModule, tk); - pSigBuilder->AppendElementType(ELEMENT_TYPE_INTERNAL); - pSigBuilder->AppendPointer(th.AsPtr()); + pSigBuilder->AppendToken(pTokenLookupMap->GetToken(th)); - ConvertToInternalExactlyOne(pSigModule, pTypeContext, pSigBuilder, bSkipCustomModifier); + ConvertToInternalExactlyOne(pSigModule, pTypeContext, pSigBuilder, pTokenLookupMap); } break; } } } -void SigPointer::ConvertToInternalSignature(Module* pSigModule, SigTypeContext *pTypeContext, SigBuilder * pSigBuilder, BOOL bSkipCustomModifier) +void SigPointer::ConvertToInternalSignature(Module* pSigModule, SigTypeContext *pTypeContext, SigBuilder * pSigBuilder, TokenLookupMap* pTokenLookupMap) { CONTRACTL { @@ -361,7 +362,7 @@ void SigPointer::ConvertToInternalSignature(Module* pSigModule, SigTypeContext * // Skip args. while (cArgs) { - ConvertToInternalExactlyOne(pSigModule, pTypeContext, pSigBuilder, bSkipCustomModifier); + ConvertToInternalExactlyOne(pSigModule, pTypeContext, pSigBuilder, pTokenLookupMap); cArgs--; } } @@ -949,7 +950,7 @@ BOOL IsTypeRefOrDef( if (resolved.TypeHandle.IsNull()) return false; - + DefineFullyQualifiedNameForClassOnStack(); LPCUTF8 fullyQualifiedName = GetFullyQualifiedNameForClass(resolved.TypeHandle.GetMethodTable()); diff --git a/src/coreclr/vm/siginfo.hpp b/src/coreclr/vm/siginfo.hpp index 82d2029c63593..935d6ccd7fe10 100644 --- a/src/coreclr/vm/siginfo.hpp +++ b/src/coreclr/vm/siginfo.hpp @@ -49,6 +49,7 @@ unsigned GetSizeForCorElementType(CorElementType etyp); class SigBuilder; class ArgDestination; +class TokenLookupMap; typedef const struct HardCodedMetaSig *LPHARDCODEDMETASIG; @@ -125,8 +126,8 @@ class SigPointer : public SigParser //========================================================================= - void ConvertToInternalExactlyOne(Module* pSigModule, SigTypeContext *pTypeContext, SigBuilder * pSigBuilder, BOOL bSkipCustomModifier = TRUE); - void ConvertToInternalSignature(Module* pSigModule, SigTypeContext *pTypeContext, SigBuilder * pSigBuilder, BOOL bSkipCustomModifier = TRUE); + void ConvertToInternalExactlyOne(Module* pSigModule, SigTypeContext *pTypeContext, SigBuilder * pSigBuilder, TokenLookupMap* pTokenMap = nullptr); + void ConvertToInternalSignature(Module* pSigModule, SigTypeContext *pTypeContext, SigBuilder * pSigBuilder, TokenLookupMap* pTokenMap = nullptr); //========================================================================= diff --git a/src/coreclr/vm/stubgen.h b/src/coreclr/vm/stubgen.h index 6847c3d72423b..192fde653e654 100644 --- a/src/coreclr/vm/stubgen.h +++ b/src/coreclr/vm/stubgen.h @@ -283,7 +283,7 @@ class FunctionSigBuilder : protected StubSigBuilder //--------------------------------------------------------------------------------------- // -class TokenLookupMap +class TokenLookupMap final { public: TokenLookupMap() @@ -490,7 +490,52 @@ class TokenLookupMap return token; } -protected: + // Generate a hash value of the token lookup map + int GetHashValue() + { + int hash = 0; + + for (size_t i = 0; i < m_nextAvailableRid; i++) + { + // Hash in the pointer values + // for the simple token map + hash = _rotl(hash, 1) + ((size_t*)m_qbEntries.Ptr())[i]; + } + + for (COUNT_T i = 0; i < m_signatures.GetCount(); i++) + { + // Hash the signatures for the signature tokens + CQuickBytesSpecifySize<16>& sigData = m_signatures[i]; + PCCOR_SIGNATURE pSig = (PCCOR_SIGNATURE)sigData.Ptr(); + DWORD cbSig = static_cast(sigData.Size()); + for (DWORD j = 0; j < cbSig; j++) + { + hash = _rotl(hash, 1) + pSig[j]; + } + } + + for (COUNT_T i = 0; i < m_memberRefs.GetCount(); i++) + { + // Hash the member ref entries + MemberRefEntry& entry = m_memberRefs[i]; + hash = _rotl(hash, 1) + entry.Type; + hash = _rotl(hash, 1) + entry.ClassSignatureToken; + hash = _rotl(hash, 1) + (size_t)entry.Entry.Method; + } + + for (COUNT_T i = 0; i < m_methodSpecs.GetCount(); i++) + { + // Hash the method spec entries + MethodSpecEntry& entry = m_methodSpecs[i]; + hash = _rotl(hash, 1) + entry.ClassSignatureToken; + hash = _rotl(hash, 1) + entry.MethodSignatureToken; + hash = _rotl(hash, 1) + (size_t)entry.Method; + } + + return hash; + } + +private: mdToken GetMemberRefWorker(MemberRefEntry** entry) { CONTRACTL diff --git a/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs b/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs index 01d489955f091..757b35cfb07a9 100644 --- a/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs +++ b/src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs @@ -11,7 +11,7 @@ namespace CopyConstructorMarshaler { public class CopyConstructorMarshaler { - //[Fact] + [Fact] public static int TestEntryPoint() { if(Environment.OSVersion.Platform != PlatformID.Win32NT || TestLibrary.Utilities.IsWindows7) @@ -32,7 +32,7 @@ public static int TestEntryPoint() { platformExtra = 1; } - + // PInvoke will copy once. Once from the managed to native parameter. Assert.Equal(1 + platformExtra, (int)testMethod.Invoke(testInstance, null)); @@ -61,7 +61,7 @@ public static int TestEntryPoint() return 100; } - //[Fact] + [Fact] public static void CopyConstructorsInArgumentStackSlots() { Assembly ijwNativeDll = Assembly.Load("IjwCopyConstructorMarshaler"); From 78d770e4d06b648e1f439447bfbc4d24dd22eb13 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 8 Aug 2024 09:55:39 -0700 Subject: [PATCH 17/30] Use a simple bool array instead of a vector as a function that has one parameter of this type will likely have multiple. --- src/coreclr/jit/compiler.h | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 409d1dabf49a9..58019fb347460 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5700,20 +5700,23 @@ class Compiler const CORINFO_SWIFT_LOWERING* GetSwiftLowering(CORINFO_CLASS_HANDLE clsHnd); #endif - jitstd::vector* m_specialCopyArgs; + bool* m_specialCopyArgs; bool recordArgRequiresSpecialCopy(unsigned argNum) { if (m_specialCopyArgs == nullptr) { - m_specialCopyArgs = new (getAllocator()) jitstd::vector(getAllocator()); + m_specialCopyArgs = new (getAllocator()) bool[info.compMethodInfo->args.numArgs]; + memset(m_specialCopyArgs, 0, info.compMethodInfo->args.numArgs * sizeof(bool)); } - m_specialCopyArgs->push_back(argNum); + assert(argNum < info.compMethodInfo->args.numArgs); + m_specialCopyArgs[argNum] = true; return true; } bool argRequiresSpecialCopy(unsigned argNum) { - return m_specialCopyArgs != nullptr && std::find(m_specialCopyArgs->begin(), m_specialCopyArgs->end(), argNum) != m_specialCopyArgs->end(); + assert(argNum < info.compMethodInfo->args.numArgs); + return m_specialCopyArgs != nullptr && m_specialCopyArgs[argNum]; } void optRecordLoopMemoryDependence(GenTree* tree, BasicBlock* block, ValueNum memoryVN); From 0f0f2440e52b8f21c4286bf3cc20ed52b24d5b5e Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 8 Aug 2024 13:34:03 -0700 Subject: [PATCH 18/30] Format --- src/coreclr/jit/lower.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index a3d82d3aec09c..bf134031be1c7 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2003,7 +2003,8 @@ void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) { // We only need to use the special copy helper on P/Invoke IL stubs // for the unmanaged call. - if (comp->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB) && comp->compMethodRequiresPInvokeFrame() && call->IsUnmanaged()) + if (comp->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB) && comp->compMethodRequiresPInvokeFrame() && + call->IsUnmanaged()) { unsigned argIndex = 0; for (CallArg& arg : call->gtArgs.Args()) @@ -2018,7 +2019,8 @@ void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) break; } - // check if parameter at the same index as the IL argument is marked as requiring special copy, assuming that it is being passed 1:1 to the pinvoke + // check if parameter at the same index as the IL argument is marked as requiring special copy, assuming + // that it is being passed 1:1 to the pinvoke unsigned paramIndex = comp->compMap2ILvarNum(argIndex); if ((paramIndex != ICorDebugInfo::UNKNOWN_ILNUM) && comp->argRequiresSpecialCopy(paramIndex)) { @@ -2042,7 +2044,7 @@ void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, CORINFO_CLASS_H dest = comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL); #endif - GenTree* src; + GenTree* src; var_types lclType = genActualType(comp->lvaGetDesc(lclNum)); if (lclType == TYP_BYREF || lclType == TYP_I_IMPL) { @@ -2058,9 +2060,7 @@ void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, CORINFO_CLASS_H GenTree* srcPlaceholder = comp->gtNewZeroConNode(genActualType(src)); GenTreeCall* call = - comp->gtNewCallNode(CT_USER_FUNC, - comp->info.compCompHnd->getSpecialCopyHelper(argType), - TYP_VOID); + comp->gtNewCallNode(CT_USER_FUNC, comp->info.compCompHnd->getSpecialCopyHelper(argType), TYP_VOID); call->gtArgs.PushBack(comp, NewCallArg::Primitive(destPlaceholder)); call->gtArgs.PushBack(comp, NewCallArg::Primitive(srcPlaceholder)); From fa74b2a84fd78f49d2007348d21fc4aae5e46e1b Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 8 Aug 2024 13:36:05 -0700 Subject: [PATCH 19/30] Update hashing --- src/coreclr/vm/stubgen.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/stubgen.h b/src/coreclr/vm/stubgen.h index 192fde653e654..d1641743ac0eb 100644 --- a/src/coreclr/vm/stubgen.h +++ b/src/coreclr/vm/stubgen.h @@ -499,7 +499,7 @@ class TokenLookupMap final { // Hash in the pointer values // for the simple token map - hash = _rotl(hash, 1) + ((size_t*)m_qbEntries.Ptr())[i]; + hash = _rotl(hash, 1) + (int)((size_t*)m_qbEntries.Ptr())[i]; } for (COUNT_T i = 0; i < m_signatures.GetCount(); i++) @@ -520,7 +520,7 @@ class TokenLookupMap final MemberRefEntry& entry = m_memberRefs[i]; hash = _rotl(hash, 1) + entry.Type; hash = _rotl(hash, 1) + entry.ClassSignatureToken; - hash = _rotl(hash, 1) + (size_t)entry.Entry.Method; + hash = _rotl(hash, 1) + (int)entry.Entry.Method; } for (COUNT_T i = 0; i < m_methodSpecs.GetCount(); i++) @@ -529,7 +529,7 @@ class TokenLookupMap final MethodSpecEntry& entry = m_methodSpecs[i]; hash = _rotl(hash, 1) + entry.ClassSignatureToken; hash = _rotl(hash, 1) + entry.MethodSignatureToken; - hash = _rotl(hash, 1) + (size_t)entry.Method; + hash = _rotl(hash, 1) + (int)entry.Method; } return hash; From dce5b1134295fbb8d3d90ba80338985a4ff3c769 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 8 Aug 2024 13:38:38 -0700 Subject: [PATCH 20/30] Remove assert --- src/coreclr/jit/compiler.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 58019fb347460..58310ca352857 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5703,12 +5703,17 @@ class Compiler bool* m_specialCopyArgs; bool recordArgRequiresSpecialCopy(unsigned argNum) { + if (argNum >= info.compMethodInfo->args.numArgs) + { + return false; + } + if (m_specialCopyArgs == nullptr) { m_specialCopyArgs = new (getAllocator()) bool[info.compMethodInfo->args.numArgs]; memset(m_specialCopyArgs, 0, info.compMethodInfo->args.numArgs * sizeof(bool)); } - assert(argNum < info.compMethodInfo->args.numArgs); + m_specialCopyArgs[argNum] = true; return true; } From 416da3739d6308fd69d96259eaf1a452274061a6 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 8 Aug 2024 13:39:48 -0700 Subject: [PATCH 21/30] Update Misc test --- .../Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs b/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs index 57ec4d9632426..10d9678567b7e 100644 --- a/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs +++ b/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs @@ -25,10 +25,10 @@ public static unsafe int StructWithCtorTest(StructWithCtor* ptrStruct, ref Struc return 2; } - int expectedCallCount = 2; + int expectedCallCount = 0; if (RuntimeInformation.ProcessArchitecture == Architecture.X86) { - expectedCallCount = 4; + expectedCallCount = 1; } if (StructWithCtor.CopyCtorCallCount != expectedCallCount) From 1287ae0cb9ad91b3dd834cdab1ee8c66330bad3b Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 8 Aug 2024 15:54:52 -0700 Subject: [PATCH 22/30] Cast through size_t to make it explicit that we're dropping the top bits --- src/coreclr/vm/stubgen.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/stubgen.h b/src/coreclr/vm/stubgen.h index d1641743ac0eb..00cc8c21d53e1 100644 --- a/src/coreclr/vm/stubgen.h +++ b/src/coreclr/vm/stubgen.h @@ -520,7 +520,7 @@ class TokenLookupMap final MemberRefEntry& entry = m_memberRefs[i]; hash = _rotl(hash, 1) + entry.Type; hash = _rotl(hash, 1) + entry.ClassSignatureToken; - hash = _rotl(hash, 1) + (int)entry.Entry.Method; + hash = _rotl(hash, 1) + (int)(size_t)entry.Entry.Method; } for (COUNT_T i = 0; i < m_methodSpecs.GetCount(); i++) @@ -529,7 +529,7 @@ class TokenLookupMap final MethodSpecEntry& entry = m_methodSpecs[i]; hash = _rotl(hash, 1) + entry.ClassSignatureToken; hash = _rotl(hash, 1) + entry.MethodSignatureToken; - hash = _rotl(hash, 1) + (int)entry.Method; + hash = _rotl(hash, 1) + (int)(size_t)entry.Method; } return hash; From 22a456bf32eab925fc90b1c9f91aa5cd5e82753c Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 8 Aug 2024 15:57:11 -0700 Subject: [PATCH 23/30] Use compArgsCount and remove assert --- src/coreclr/jit/compiler.h | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 58310ca352857..f31612f8862d5 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5703,15 +5703,15 @@ class Compiler bool* m_specialCopyArgs; bool recordArgRequiresSpecialCopy(unsigned argNum) { - if (argNum >= info.compMethodInfo->args.numArgs) + if (argNum >= info.compArgsCount) { return false; } if (m_specialCopyArgs == nullptr) { - m_specialCopyArgs = new (getAllocator()) bool[info.compMethodInfo->args.numArgs]; - memset(m_specialCopyArgs, 0, info.compMethodInfo->args.numArgs * sizeof(bool)); + m_specialCopyArgs = new (getAllocator()) bool[info.compArgsCount]; + memset(m_specialCopyArgs, 0, info.compArgsCount * sizeof(bool)); } m_specialCopyArgs[argNum] = true; @@ -5720,8 +5720,7 @@ class Compiler bool argRequiresSpecialCopy(unsigned argNum) { - assert(argNum < info.compMethodInfo->args.numArgs); - return m_specialCopyArgs != nullptr && m_specialCopyArgs[argNum]; + return argNum < info.compArgsCount && m_specialCopyArgs != nullptr && m_specialCopyArgs[argNum]; } void optRecordLoopMemoryDependence(GenTree* tree, BasicBlock* block, ValueNum memoryVN); From bb7217b7dce5751bf5c0dea4c5ce0c1942c3181f Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 9 Aug 2024 11:14:02 -0700 Subject: [PATCH 24/30] Various fixes --- src/coreclr/jit/lower.cpp | 3 ++- src/coreclr/vm/ilmarshalers.cpp | 3 --- .../Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs | 5 +---- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index bf134031be1c7..a0abc228631c3 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2022,7 +2022,8 @@ void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) // check if parameter at the same index as the IL argument is marked as requiring special copy, assuming // that it is being passed 1:1 to the pinvoke unsigned paramIndex = comp->compMap2ILvarNum(argIndex); - if ((paramIndex != ICorDebugInfo::UNKNOWN_ILNUM) && comp->argRequiresSpecialCopy(paramIndex)) + if ((paramIndex != ICorDebugInfo::UNKNOWN_ILNUM) && comp->argRequiresSpecialCopy(paramIndex) && + genActualType(arg.GetNode()) == TYP_STRUCT) { assert(arg.GetNode()->OperIs(GT_PUTARG_STK)); InsertSpecialCopyArg(arg.GetNode()->AsPutArgStk(), arg.GetSignatureClassHandle(), paramIndex); diff --git a/src/coreclr/vm/ilmarshalers.cpp b/src/coreclr/vm/ilmarshalers.cpp index bd89623a36340..ab3a8b4539d14 100644 --- a/src/coreclr/vm/ilmarshalers.cpp +++ b/src/coreclr/vm/ilmarshalers.cpp @@ -3423,8 +3423,6 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver #ifdef TARGET_X86 LocalDesc locDesc(pargs->mm.m_pMT); pslIL->SetStubTargetArgType(&locDesc); // native type is the value type - - locDesc.AddModifier(true, pslIL->GetToken(pargs->mm.m_pSigMod)); locDesc.MakeByRef(); locDesc.MakePinned(); @@ -3441,7 +3439,6 @@ MarshalerOverrideStatus ILBlittableValueClassWithCopyCtorMarshaler::ArgumentOver pslILDispatch->EmitLDOBJ(pslIL->GetToken(pargs->mm.m_pMT)); #else LocalDesc locDesc(pargs->mm.m_pMT); - locDesc.AddModifier(true, pslIL->GetToken(pargs->mm.m_pSigMod)); locDesc.MakeByRef(); locDesc.MakePinned(); diff --git a/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs b/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs index 10d9678567b7e..63dcd520cee49 100644 --- a/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs +++ b/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs @@ -26,10 +26,6 @@ public static unsafe int StructWithCtorTest(StructWithCtor* ptrStruct, ref Struc } int expectedCallCount = 0; - if (RuntimeInformation.ProcessArchitecture == Architecture.X86) - { - expectedCallCount = 1; - } if (StructWithCtor.CopyCtorCallCount != expectedCallCount) { @@ -48,6 +44,7 @@ public static unsafe int StructWithCtorTest(StructWithCtor* ptrStruct, ref Struc [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))] + [SkipOnCoreClr("JitStress can introduce extra copies", RuntimeTestModes.JitStress)] public static unsafe void ValidateCopyConstructorAndDestructorCalled() { CopyCtorUtil.TestDelegate del = (CopyCtorUtil.TestDelegate)Delegate.CreateDelegate(typeof(CopyCtorUtil.TestDelegate), typeof(CopyCtor).GetMethod("StructWithCtorTest")); From 8e977cc4aba01954f1c16317261fcb251a8b4234 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 12 Aug 2024 15:15:34 -0700 Subject: [PATCH 25/30] Fix GCC build --- src/coreclr/jit/lower.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index a0abc228631c3..ccb778f3e6cd3 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2022,7 +2022,7 @@ void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) // check if parameter at the same index as the IL argument is marked as requiring special copy, assuming // that it is being passed 1:1 to the pinvoke unsigned paramIndex = comp->compMap2ILvarNum(argIndex); - if ((paramIndex != ICorDebugInfo::UNKNOWN_ILNUM) && comp->argRequiresSpecialCopy(paramIndex) && + if ((paramIndex != (unsigned)ICorDebugInfo::UNKNOWN_ILNUM) && comp->argRequiresSpecialCopy(paramIndex) && genActualType(arg.GetNode()) == TYP_STRUCT) { assert(arg.GetNode()->OperIs(GT_PUTARG_STK)); From 850e78d3133203414d022b284eb56bfe8de1f19b Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 12 Aug 2024 15:25:05 -0700 Subject: [PATCH 26/30] Fix signature type check --- src/coreclr/jit/lower.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index ccb778f3e6cd3..07ec81853cba6 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2023,7 +2023,7 @@ void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) // that it is being passed 1:1 to the pinvoke unsigned paramIndex = comp->compMap2ILvarNum(argIndex); if ((paramIndex != (unsigned)ICorDebugInfo::UNKNOWN_ILNUM) && comp->argRequiresSpecialCopy(paramIndex) && - genActualType(arg.GetNode()) == TYP_STRUCT) + arg.GetSignatureType() == TYP_STRUCT) { assert(arg.GetNode()->OperIs(GT_PUTARG_STK)); InsertSpecialCopyArg(arg.GetNode()->AsPutArgStk(), arg.GetSignatureClassHandle(), paramIndex); From 670c40d8829ef47e24bb9a420a875af0a1530ce1 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 13 Aug 2024 15:35:32 -0700 Subject: [PATCH 27/30] Feedback --- src/coreclr/jit/compiler.cpp | 2 ++ src/coreclr/jit/compiler.h | 2 ++ src/coreclr/jit/gschecks.cpp | 2 ++ src/coreclr/jit/lclvars.cpp | 2 ++ src/coreclr/jit/lower.cpp | 25 +++++++------------ src/coreclr/jit/lower.h | 2 ++ .../superpmi-shared/methodcontext.cpp | 10 ++++---- .../superpmi/superpmi-shared/methodcontext.h | 8 +++--- .../superpmi-shim-collector/icorjitinfo.cpp | 2 +- .../tools/superpmi/superpmi/icorjitinfo.cpp | 2 +- 10 files changed, 30 insertions(+), 27 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index eeca29a97f703..7d2c2eceebce1 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -1989,7 +1989,9 @@ void Compiler::compInit(ArenaAllocator* pAlloc, m_fpStructLoweringCache = nullptr; #endif +#if defined(TARGET_X86) && defined(FEATURE_IJW) m_specialCopyArgs = nullptr; +#endif // check that HelperCallProperties are initialized diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index f54688ccd2234..f9e88ac12dc36 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5703,6 +5703,7 @@ class Compiler const CORINFO_SWIFT_LOWERING* GetSwiftLowering(CORINFO_CLASS_HANDLE clsHnd); #endif +#if defined(TARGET_X86) && defined(FEATURE_IJW) bool* m_specialCopyArgs; bool recordArgRequiresSpecialCopy(unsigned argNum) { @@ -5725,6 +5726,7 @@ class Compiler { return argNum < info.compArgsCount && m_specialCopyArgs != nullptr && m_specialCopyArgs[argNum]; } +#endif void optRecordLoopMemoryDependence(GenTree* tree, BasicBlock* block, ValueNum memoryVN); void optCopyLoopMemoryDependence(GenTree* fromTree, GenTree* toTree); diff --git a/src/coreclr/jit/gschecks.cpp b/src/coreclr/jit/gschecks.cpp index 5331d0e531e3b..06802181eea22 100644 --- a/src/coreclr/jit/gschecks.cpp +++ b/src/coreclr/jit/gschecks.cpp @@ -516,6 +516,7 @@ void Compiler::gsParamsToShadows() continue; } +#if defined(TARGET_X86) && defined(FEATURE_IJW) if (lclNum < info.compArgsCount && argRequiresSpecialCopy(lclNum) && (varDsc->TypeGet() == TYP_STRUCT)) { JITDUMP("arg%02u requires special copy, using special copy helper to copy to shadow var V%02u\n", lclNum, @@ -557,6 +558,7 @@ void Compiler::gsParamsToShadows() } } else +#endif // TARGET_X86 && FEATURE_IJW { GenTree* src = gtNewLclvNode(lclNum, varDsc->TypeGet()); src->gtFlags |= GTF_DONT_CSE; diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index a407f82a32c2b..e1fbeceaf3bcc 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -653,6 +653,7 @@ void Compiler::lvaInitUserArgs(InitVarDscInfo* varDscInfo, unsigned skipArgs, un CorInfoTypeWithMod corInfoType = info.compCompHnd->getArgType(&info.compMethodInfo->args, argLst, &typeHnd); varDsc->lvIsParam = 1; +#if defined(TARGET_X86) && defined(FEATURE_IJW) if ((corInfoType & CORINFO_TYPE_MOD_COPY_WITH_HELPER) != 0) { CorInfoType typeWithoutMod = strip(corInfoType); @@ -662,6 +663,7 @@ void Compiler::lvaInitUserArgs(InitVarDscInfo* varDscInfo, unsigned skipArgs, un recordArgRequiresSpecialCopy(i); } } +#endif // TARGET_X86 && FEATURE_IJW lvaInitVarDsc(varDsc, varDscInfo->varNum, strip(corInfoType), typeHnd, argLst, &info.compMethodInfo->args); diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 4d724e2cbc750..36cd17abd3045 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -1994,11 +1994,14 @@ void Lowering::LowerArgsForCall(GenTreeCall* call) LowerArg(call, &arg, true); } +#if defined(TARGET_X86) && defined(FEATURE_IJW) LowerSpecialCopyArgs(call); +#endif // defined(TARGET_X86) && defined(FEATURE_IJW) LegalizeArgPlacement(call); } +#if defined(TARGET_X86) && defined(FEATURE_IJW) void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) { // We only need to use the special copy helper on P/Invoke IL stubs @@ -2014,19 +2017,15 @@ void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) continue; } - if (argIndex >= comp->info.compArgsCount) - { - break; - } + unsigned paramLclNum = comp->compMapILargNum(argIndex); + assert(paramLclNum < comp->info.compArgsCount); // check if parameter at the same index as the IL argument is marked as requiring special copy, assuming // that it is being passed 1:1 to the pinvoke - unsigned paramIndex = comp->compMap2ILvarNum(argIndex); - if ((paramIndex != (unsigned)ICorDebugInfo::UNKNOWN_ILNUM) && comp->argRequiresSpecialCopy(paramIndex) && - arg.GetSignatureType() == TYP_STRUCT) + if (comp->argRequiresSpecialCopy(paramLclNum) && (arg.GetSignatureType() == TYP_STRUCT)) { assert(arg.GetNode()->OperIs(GT_PUTARG_STK)); - InsertSpecialCopyArg(arg.GetNode()->AsPutArgStk(), arg.GetSignatureClassHandle(), paramIndex); + InsertSpecialCopyArg(arg.GetNode()->AsPutArgStk(), arg.GetSignatureClassHandle(), paramLclNum); } argIndex++; } @@ -2036,14 +2035,7 @@ void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, CORINFO_CLASS_HANDLE argType, unsigned lclNum) { assert(putArgStk != nullptr); - GenTree* dest; - -#if FEATURE_FIXED_OUT_ARGS - dest = comp->gtNewOperNode(GT_ADD, TYP_I_IMPL, comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL), - comp->gtNewIconNode(putArgStk->getArgOffset())); -#else - dest = comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL); -#endif + GenTree* dest = comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL); GenTree* src; var_types lclType = genActualType(comp->lvaGetDesc(lclNum)); @@ -2094,6 +2086,7 @@ void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, CORINFO_CLASS_H BlockRange().Remove(destPlaceholder); BlockRange().Remove(srcPlaceholder); } +#endif // defined(TARGET_X86) && defined(FEATURE_IJW) //------------------------------------------------------------------------ // LegalizeArgPlacement: Move arg placement nodes (PUTARG_*) into a legal diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index efe7e11a49df7..6ff448d452cd9 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -183,8 +183,10 @@ class Lowering final : public Phase GenTree* LowerVirtualStubCall(GenTreeCall* call); void LowerArgsForCall(GenTreeCall* call); void ReplaceArgWithPutArgOrBitcast(GenTree** ppChild, GenTree* newNode); +#if defined(TARGET_X86) && defined(FEATURE_IJW) void LowerSpecialCopyArgs(GenTreeCall* call); void InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, CORINFO_CLASS_HANDLE argType, unsigned lclNum); +#endif // defined(TARGET_X86) && defined(FEATURE_IJW) GenTree* NewPutArg(GenTreeCall* call, GenTree* arg, CallArg* callArg, var_types type); void LowerArg(GenTreeCall* call, CallArg* callArg, bool late); #if defined(TARGET_ARMARCH) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp index 35515aa5aa683..17ee0d167851b 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp @@ -7212,7 +7212,7 @@ const WCHAR* MethodContext::repGetStringConfigValue(const WCHAR* name) return value; } -void MethodContext::recgetSpecialCopyHelper(CORINFO_CLASS_HANDLE type, CORINFO_METHOD_HANDLE helper) +void MethodContext::recGetSpecialCopyHelper(CORINFO_CLASS_HANDLE type, CORINFO_METHOD_HANDLE helper) { if (getSpecialCopyHelper == nullptr) getSpecialCopyHelper = new LightWeightMap(); @@ -7223,19 +7223,19 @@ void MethodContext::recgetSpecialCopyHelper(CORINFO_CLASS_HANDLE type, CORINFO_M DWORDLONG value = CastHandle(helper); getSpecialCopyHelper->Add(key, value); - DEBUG_REC(dmpgetSpecialCopyHelper(key, value)); + DEBUG_REC(dmpGetSpecialCopyHelper(key, value)); } -void MethodContext::dmpgetSpecialCopyHelper(DWORDLONG key, DWORDLONG value) +void MethodContext::dmpGetSpecialCopyHelper(DWORDLONG key, DWORDLONG value) { printf("getSpecialCopyHelper key %016" PRIX64 ", value %016" PRIX64 "", key, value); } -CORINFO_METHOD_HANDLE MethodContext::repgetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) +CORINFO_METHOD_HANDLE MethodContext::repGetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) { DWORDLONG key = CastHandle(type); DWORDLONG value = LookupByKeyOrMiss(getSpecialCopyHelper, key, ": key %016" PRIX64 "", key); - DEBUG_REP(dmpgetSpecialCopyHelper(key, value)); + DEBUG_REP(dmpGetSpecialCopyHelper(key, value)); return (CORINFO_METHOD_HANDLE)value; } diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h index 63bbe66fcd507..4625ad44aab37 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h @@ -898,9 +898,9 @@ class MethodContext void dmpGetStringConfigValue(DWORD nameIndex, DWORD result); const WCHAR* repGetStringConfigValue(const WCHAR* name); - void recgetSpecialCopyHelper(CORINFO_CLASS_HANDLE type, CORINFO_METHOD_HANDLE helper); - void dmpgetSpecialCopyHelper(DWORDLONG key, DWORDLONG value); - CORINFO_METHOD_HANDLE repgetSpecialCopyHelper(CORINFO_CLASS_HANDLE type); + void recGetSpecialCopyHelper(CORINFO_CLASS_HANDLE type, CORINFO_METHOD_HANDLE helper); + void dmpGetSpecialCopyHelper(DWORDLONG key, DWORDLONG value); + CORINFO_METHOD_HANDLE repGetSpecialCopyHelper(CORINFO_CLASS_HANDLE type); void dmpSigInstHandleMap(DWORD key, DWORDLONG value); @@ -1193,7 +1193,7 @@ enum mcPackets Packet_GetTypeForBoxOnStack = 221, Packet_GetTypeDefinition = 222, Packet_GetFpStructLowering = 223, - Packet_getSpecialCopyHelper = 224, + Packet_GetSpecialCopyHelper = 224, }; void SetDebugDumpVariables(); diff --git a/src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp b/src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp index 94ea159666c02..b3fab2e939dae 100644 --- a/src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp @@ -2033,6 +2033,6 @@ CORINFO_METHOD_HANDLE interceptor_ICJI::getSpecialCopyHelper(CORINFO_CLASS_HANDL { mc->cr->AddCall("getSpecialCopyHelper"); CORINFO_METHOD_HANDLE temp = original_ICorJitInfo->getSpecialCopyHelper(type); - mc->recgetSpecialCopyHelper(type, temp); + mc->recGetSpecialCopyHelper(type, temp); return temp; } diff --git a/src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp b/src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp index 44dabb84c538e..64c52f61705ae 100644 --- a/src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp +++ b/src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp @@ -1868,6 +1868,6 @@ uint32_t MyICJI::getExpectedTargetArchitecture() CORINFO_METHOD_HANDLE MyICJI::getSpecialCopyHelper(CORINFO_CLASS_HANDLE type) { jitInstance->mc->cr->AddCall("getSpecialCopyHelper"); - CORINFO_METHOD_HANDLE result = jitInstance->mc->repgetSpecialCopyHelper(type); + CORINFO_METHOD_HANDLE result = jitInstance->mc->repGetSpecialCopyHelper(type); return result; } From dc3754f3c37123ca0896f6054d5647f7c11fba94 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 14 Aug 2024 10:10:16 -0700 Subject: [PATCH 28/30] Add headers --- src/coreclr/jit/lower.cpp | 40 ++++++++++++++++++++++++++++++++++++--- src/coreclr/jit/lower.h | 4 ++-- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 36cd17abd3045..59ec1d7bc5642 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2002,6 +2002,23 @@ void Lowering::LowerArgsForCall(GenTreeCall* call) } #if defined(TARGET_X86) && defined(FEATURE_IJW) +//------------------------------------------------------------------------ +// LowerSpecialCopyArgs: Lower special copy arguments for P/Invoke IL stubs +// +// Arguments: +// call - the call node +// +// Notes: +// This method is used for P/Invoke IL stubs on x86 to handle arguments with special copy semantics. +// In particular, this method implements copy-constructor semantics for managed-to-unmanaged IL stubs +// for C++/CLI. In this case, the managed argument is passed by (managed or unmanaged) pointer in the +// P/Invoke signature with a speial modreq, but is passed to the unmanaged function by value. +// The value passed to the unmanaged function must be created through a copy-constructor call copying from +// the original source argument. +// We assume that the IL stub will be generated such that the following holds true: +// - If an argument to the IL stub has the special modreq, then its corresponding argument to the +// unmanaged function will be passed as the same argument index. Therefore, we can introduce the copy call +// from the original source argument to the argument slot in the unmanaged call. void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) { // We only need to use the special copy helper on P/Invoke IL stubs @@ -2032,16 +2049,33 @@ void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) } } +//------------------------------------------------------------------------ +// InsertSpecialCopyArg: Insert a call to the special copy helper to copy from the (possibly value pointed-to by) local +// lclnum to the argument slot represented by putArgStk +// +// Arguments: +// putArgStk - the PutArgStk node representing the stack slot of the argument +// argType - the struct type of the argument +// lclNum - the local to use as the source for the special copy helper +// +// Notes: +// This method assumes that lclNum is either a by-ref to a struct of type argType +// or a struct of type argType. +// We use this to preserve special copy semantics for interop calls where we pass in a byref to a struct into a +// P/Invoke with a special modreq and the native function expects to recieve the struct by value with the argument +// being passed in having been created by the special copy helper. +// void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, CORINFO_CLASS_HANDLE argType, unsigned lclNum) { assert(putArgStk != nullptr); GenTree* dest = comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL); GenTree* src; - var_types lclType = genActualType(comp->lvaGetDesc(lclNum)); + var_types lclType = comp->lvaGetDesc(lclNum); + if (lclType == TYP_BYREF || lclType == TYP_I_IMPL) { - src = comp->gtNewLclVarNode(lclNum, TYP_I_IMPL); + src = comp->gtNewLclVarNode(lclNum, lclType); } else { @@ -2050,7 +2084,7 @@ void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, CORINFO_CLASS_H } GenTree* destPlaceholder = comp->gtNewZeroConNode(dest->TypeGet()); - GenTree* srcPlaceholder = comp->gtNewZeroConNode(genActualType(src)); + GenTree* srcPlaceholder = comp->gtNewZeroConNode(src); GenTreeCall* call = comp->gtNewCallNode(CT_USER_FUNC, comp->info.compCompHnd->getSpecialCopyHelper(argType), TYP_VOID); diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 6ff448d452cd9..c5c771167025e 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -184,8 +184,8 @@ class Lowering final : public Phase void LowerArgsForCall(GenTreeCall* call); void ReplaceArgWithPutArgOrBitcast(GenTree** ppChild, GenTree* newNode); #if defined(TARGET_X86) && defined(FEATURE_IJW) - void LowerSpecialCopyArgs(GenTreeCall* call); - void InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, CORINFO_CLASS_HANDLE argType, unsigned lclNum); + void LowerSpecialCopyArgs(GenTreeCall* call); + void InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, CORINFO_CLASS_HANDLE argType, unsigned lclNum); #endif // defined(TARGET_X86) && defined(FEATURE_IJW) GenTree* NewPutArg(GenTreeCall* call, GenTree* arg, CallArg* callArg, var_types type); void LowerArg(GenTreeCall* call, CallArg* callArg, bool late); From b0ec55c5c629650039ef866828fdebd9c5f2bcb1 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 14 Aug 2024 11:30:41 -0700 Subject: [PATCH 29/30] Fix superpmi build failures. Correctly handle multiple-arg copy constructor scenarios (by counting args in the correct direction) --- src/coreclr/jit/lclvars.cpp | 3 ++- src/coreclr/jit/lower.cpp | 27 ++++++++++++++++--- .../tools/superpmi/superpmi-shared/lwmlist.h | 2 +- .../superpmi-shared/methodcontext.cpp | 8 +++--- .../Miscellaneous/CopyCtor/CopyCtorTest.cs | 2 +- 5 files changed, 31 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index e1fbeceaf3bcc..780d098978365 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -657,7 +657,8 @@ void Compiler::lvaInitUserArgs(InitVarDscInfo* varDscInfo, unsigned skipArgs, un if ((corInfoType & CORINFO_TYPE_MOD_COPY_WITH_HELPER) != 0) { CorInfoType typeWithoutMod = strip(corInfoType); - if (typeWithoutMod == CORINFO_TYPE_VALUECLASS || typeWithoutMod == CORINFO_TYPE_PTR) + if (typeWithoutMod == CORINFO_TYPE_VALUECLASS || typeWithoutMod == CORINFO_TYPE_PTR || + typeWithoutMod == CORINFO_TYPE_BYREF) { JITDUMP("Marking user arg%02u as requiring special copy semantics\n", i); recordArgRequiresSpecialCopy(i); diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 59ec1d7bc5642..ed66342a56948 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2026,7 +2026,12 @@ void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) if (comp->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB) && comp->compMethodRequiresPInvokeFrame() && call->IsUnmanaged()) { - unsigned argIndex = 0; + // Unmanaged calling conventions on Windows x86 are passed in reverse order + // of managed args, so we need to count down the number of args. + // If the call is thiscall, we need to account for the this parameter, + // which will be first in the list. + // The this parameter is always passed in registers, so we can ignore it. + unsigned argIndex = call->gtArgs.CountUserArgs() - 1; for (CallArg& arg : call->gtArgs.Args()) { if (!arg.IsUserArg()) @@ -2034,6 +2039,19 @@ void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) continue; } + if (call->GetUnmanagedCallConv() == CorInfoCallConvExtension::Thiscall && + argIndex == call->gtArgs.CountUserArgs() - 1) + { + assert(arg.GetNode()->OperIs(GT_PUTARG_REG)); + continue; + } + + if (argIndex >= comp->info.compILargsCount) + { + argIndex--; + continue; + } + unsigned paramLclNum = comp->compMapILargNum(argIndex); assert(paramLclNum < comp->info.compArgsCount); @@ -2044,7 +2062,8 @@ void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) assert(arg.GetNode()->OperIs(GT_PUTARG_STK)); InsertSpecialCopyArg(arg.GetNode()->AsPutArgStk(), arg.GetSignatureClassHandle(), paramLclNum); } - argIndex++; + + argIndex--; } } } @@ -2071,7 +2090,7 @@ void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, CORINFO_CLASS_H GenTree* dest = comp->gtNewPhysRegNode(REG_SPBASE, TYP_I_IMPL); GenTree* src; - var_types lclType = comp->lvaGetDesc(lclNum); + var_types lclType = comp->lvaGetRealType(lclNum); if (lclType == TYP_BYREF || lclType == TYP_I_IMPL) { @@ -2084,7 +2103,7 @@ void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, CORINFO_CLASS_H } GenTree* destPlaceholder = comp->gtNewZeroConNode(dest->TypeGet()); - GenTree* srcPlaceholder = comp->gtNewZeroConNode(src); + GenTree* srcPlaceholder = comp->gtNewZeroConNode(src->TypeGet()); GenTreeCall* call = comp->gtNewCallNode(CT_USER_FUNC, comp->info.compCompHnd->getSpecialCopyHelper(argType), TYP_VOID); diff --git a/src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h b/src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h index 9c6c591fb8d40..e00f86d1b6458 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h +++ b/src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h @@ -131,7 +131,7 @@ LWM(GetSwiftLowering, DWORDLONG, Agnostic_GetSwiftLowering) LWM(GetFpStructLowering, DWORDLONG, Agnostic_GetFpStructLowering) LWM(GetTailCallHelpers, Agnostic_GetTailCallHelpers, Agnostic_CORINFO_TAILCALL_HELPERS) LWM(UpdateEntryPointForTailCall, Agnostic_CORINFO_CONST_LOOKUP, Agnostic_CORINFO_CONST_LOOKUP) -LWM(getSpecialCopyHelper, DWORDLONG, DWORDLONG) +LWM(GetSpecialCopyHelper, DWORDLONG, DWORDLONG) LWM(GetThreadTLSIndex, DWORD, DLD) LWM(GetTokenTypeAsHandle, GetTokenTypeAsHandleValue, DWORDLONG) LWM(GetTypeForBox, DWORDLONG, DWORDLONG) diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp index 17ee0d167851b..d1078fe8d356c 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp @@ -7214,15 +7214,15 @@ const WCHAR* MethodContext::repGetStringConfigValue(const WCHAR* name) void MethodContext::recGetSpecialCopyHelper(CORINFO_CLASS_HANDLE type, CORINFO_METHOD_HANDLE helper) { - if (getSpecialCopyHelper == nullptr) - getSpecialCopyHelper = new LightWeightMap(); + if (GetSpecialCopyHelper == nullptr) + GetSpecialCopyHelper = new LightWeightMap(); DWORDLONG key; ZeroMemory(&key, sizeof(key)); // Zero key including any struct padding key = CastHandle(type); DWORDLONG value = CastHandle(helper); - getSpecialCopyHelper->Add(key, value); + GetSpecialCopyHelper->Add(key, value); DEBUG_REC(dmpGetSpecialCopyHelper(key, value)); } @@ -7234,7 +7234,7 @@ void MethodContext::dmpGetSpecialCopyHelper(DWORDLONG key, DWORDLONG value) CORINFO_METHOD_HANDLE MethodContext::repGetSpecialCopyHelper(CORINFO_CLASS_HANDLE type) { DWORDLONG key = CastHandle(type); - DWORDLONG value = LookupByKeyOrMiss(getSpecialCopyHelper, key, ": key %016" PRIX64 "", key); + DWORDLONG value = LookupByKeyOrMiss(GetSpecialCopyHelper, key, ": key %016" PRIX64 "", key); DEBUG_REP(dmpGetSpecialCopyHelper(key, value)); return (CORINFO_METHOD_HANDLE)value; } diff --git a/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs b/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs index 63dcd520cee49..9601a835bb79a 100644 --- a/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs +++ b/src/tests/Interop/PInvoke/Miscellaneous/CopyCtor/CopyCtorTest.cs @@ -25,7 +25,7 @@ public static unsafe int StructWithCtorTest(StructWithCtor* ptrStruct, ref Struc return 2; } - int expectedCallCount = 0; + int expectedCallCount = RuntimeInformation.ProcessArchitecture == Architecture.X86 ? 2 : 0; if (StructWithCtor.CopyCtorCallCount != expectedCallCount) { From 048b0c3a6828f5bee318b27fd3071f87cbee9be9 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 14 Aug 2024 12:21:48 -0700 Subject: [PATCH 30/30] Add check and assert --- src/coreclr/jit/compiler.h | 5 +++++ src/coreclr/jit/lower.cpp | 9 ++------- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index f9e88ac12dc36..1315a57a139a5 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5726,6 +5726,11 @@ class Compiler { return argNum < info.compArgsCount && m_specialCopyArgs != nullptr && m_specialCopyArgs[argNum]; } + + bool compHasSpecialCopyArgs() + { + return m_specialCopyArgs != nullptr; + } #endif void optRecordLoopMemoryDependence(GenTree* tree, BasicBlock* block, ValueNum memoryVN); diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index ed66342a56948..f8bc800241491 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2024,7 +2024,7 @@ void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) // We only need to use the special copy helper on P/Invoke IL stubs // for the unmanaged call. if (comp->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB) && comp->compMethodRequiresPInvokeFrame() && - call->IsUnmanaged()) + call->IsUnmanaged() && comp->compHasSpecialCopyArgs()) { // Unmanaged calling conventions on Windows x86 are passed in reverse order // of managed args, so we need to count down the number of args. @@ -2032,6 +2032,7 @@ void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) // which will be first in the list. // The this parameter is always passed in registers, so we can ignore it. unsigned argIndex = call->gtArgs.CountUserArgs() - 1; + assert(call->gtArgs.CountUserArgs() == comp->info.compILargsCount); for (CallArg& arg : call->gtArgs.Args()) { if (!arg.IsUserArg()) @@ -2046,12 +2047,6 @@ void Lowering::LowerSpecialCopyArgs(GenTreeCall* call) continue; } - if (argIndex >= comp->info.compILargsCount) - { - argIndex--; - continue; - } - unsigned paramLclNum = comp->compMapILargNum(argIndex); assert(paramLclNum < comp->info.compArgsCount);