From cd8e5b32309ec396a28ac5f42bc6d490fe1ba1e0 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Mon, 7 Apr 2025 16:23:51 -0700 Subject: [PATCH 1/7] CoreCLR support for FieldRVA table in EnC. --- src/coreclr/vm/encee.cpp | 112 +++++++++++++++++------- src/coreclr/vm/encee.h | 10 +++ src/coreclr/vm/field.cpp | 7 +- src/coreclr/vm/reflectioninvocation.cpp | 12 ++- 4 files changed, 105 insertions(+), 36 deletions(-) diff --git a/src/coreclr/vm/encee.cpp b/src/coreclr/vm/encee.cpp index b61b8acdc8bfe2..fdb0bbba6c14a6 100644 --- a/src/coreclr/vm/encee.cpp +++ b/src/coreclr/vm/encee.cpp @@ -129,7 +129,6 @@ HRESULT EditAndContinueModule::ApplyEditAndContinue( // Debugging hook to dump out all edits to dmeta and dil files static BOOL dumpChanges = -1; - if (dumpChanges == -1) dumpChanges = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_EncDumpApplyChanges); @@ -139,12 +138,12 @@ HRESULT EditAndContinueModule::ApplyEditAndContinue( int ec; fn.Printf("ApplyChanges.%d.dmeta", m_applyChangesCount); FILE *fp; - ec = _wfopen_s(&fp, fn.GetUnicode(), W("wb")); + ec = fopen_s(&fp, fn.GetUTF8(), "wb"); _ASSERTE(SUCCEEDED(ec)); fwrite(pDeltaMD, 1, cbDeltaMD, fp); fclose(fp); fn.Printf("ApplyChanges.%d.dil", m_applyChangesCount); - ec = _wfopen_s(&fp, fn.GetUnicode(), W("wb")); + ec = fopen_s(&fp, fn.GetUTF8(), "wb"); _ASSERTE(SUCCEEDED(ec)); fwrite(pDeltaIL, 1, cbDeltaIL, fp); fclose(fp); @@ -152,11 +151,6 @@ HRESULT EditAndContinueModule::ApplyEditAndContinue( #endif HRESULT hr = S_OK; - HENUMInternal enumENC; - - BYTE *pLocalILMemory = NULL; - IMDInternalImport *pMDImport = NULL; - IMDInternalImport *pNewMDImport = NULL; CONTRACT_VIOLATION(GCViolation); // SafeComHolder goes to preemptive mode, which will trigger a GC SafeComHolder pIMDInternalImportENC; @@ -180,36 +174,39 @@ HRESULT EditAndContinueModule::ApplyEditAndContinue( } EX_CATCH_HRESULT(hr); - IfFailGo(hr); + IfFailRet(hr); // Grab the current importer. - pMDImport = GetMDImport(); + IMDInternalImport* pMDImport = GetMDImport(); // Apply the EnC delta to this module's metadata. - IfFailGo(pMDImport->ApplyEditAndContinue(pDeltaMD, cbDeltaMD, &pNewMDImport)); + IMDInternalImport* pNewMDImport = NULL; + IfFailRet(pMDImport->ApplyEditAndContinue(pDeltaMD, cbDeltaMD, &pNewMDImport)); // The importer should not have changed! We assert that, and back-stop in a retail build just to be sure. if (pNewMDImport != pMDImport) { _ASSERTE( !"ApplyEditAndContinue should not have needed to create a new metadata importer!" ); - IfFailGo(CORDBG_E_ENC_INTERNAL_ERROR); + IfFailRet(CORDBG_E_ENC_INTERNAL_ERROR); } // get the delta interface - IfFailGo(pMDImport->QueryInterface(IID_IMDInternalImportENC, (void **)&pIMDInternalImportENC)); + IfFailRet(pMDImport->QueryInterface(IID_IMDInternalImportENC, (void **)&pIMDInternalImportENC)); // get an emitter interface - IfFailGo(GetMetaDataPublicInterfaceFromInternal(pMDImport, IID_IMetaDataEmit, (void **)&pEmitter)); + IfFailRet(GetMetaDataPublicInterfaceFromInternal(pMDImport, IID_IMetaDataEmit, (void **)&pEmitter)); - // Copy the deltaIL into our RVAable IL memory - pLocalILMemory = new BYTE[cbDeltaIL]; + // Copy the delta IL into our RVA-able IL memory + BYTE* pLocalILMemory = (BYTE*)(void*)GetLoaderAllocator()->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(cbDeltaIL)); memcpy(pLocalILMemory, pDeltaIL, cbDeltaIL); // Enumerate all of the EnC delta tokens - HENUMInternal::ZeroEnum(&enumENC); - IfFailGo(pIMDInternalImportENC->EnumDeltaTokensInit(&enumENC)); + MDEnumHolder enumENC{ pIMDInternalImportENC }; + IfFailRet(pIMDInternalImportENC->EnumDeltaTokensInit(&enumENC)); mdToken token; + MethodDesc* pMethod; + FieldDesc* pFieldDesc; while (pIMDInternalImportENC->EnumNext(&enumENC, &token)) { STRESS_LOG1(LF_ENC, LL_INFO100, "EACM::AEAC: updated token 0x%08x\n", token); @@ -223,28 +220,27 @@ HRESULT EditAndContinueModule::ApplyEditAndContinue( ULONG dwMethodRVA; DWORD dwMethodFlags; - IfFailGo(pMDImport->GetMethodImplProps(token, &dwMethodRVA, &dwMethodFlags)); + IfFailRet(pMDImport->GetMethodImplProps(token, &dwMethodRVA, &dwMethodFlags)); if (dwMethodRVA >= cbDeltaIL) { - LOG((LF_ENC, LL_INFO10000, "EACM::AEAC: Failure RVA of %d with cbDeltaIl %d\n", dwMethodRVA, cbDeltaIL)); - IfFailGo(E_INVALIDARG); + LOG((LF_ENC, LL_INFO10000, "EACM::AEAC: Failure RVA of %u with cbDeltaIL %u\n", dwMethodRVA, cbDeltaIL)); + IfFailRet(E_INVALIDARG); } - SetDynamicIL(token, (TADDR)(pLocalILMemory + dwMethodRVA)); + SetDynamicIL(token, (TADDR)(&pLocalILMemory[dwMethodRVA])); // use module to resolve to method - MethodDesc *pMethod; pMethod = LookupMethodDef(token); if (pMethod) { // Method exists already - update it - IfFailGo(UpdateMethod(pMethod)); + IfFailRet(UpdateMethod(pMethod)); } else { // This is a new method token - create a new method - IfFailGo(AddMethod(token)); + IfFailRet(AddMethod(token)); } break; @@ -254,14 +250,39 @@ HRESULT EditAndContinueModule::ApplyEditAndContinue( // FieldDef token - add a new field LOG((LF_ENC, LL_INFO10000, "EACM::AEAC: Found field 0x%08x\n", token)); - if (LookupFieldDef(token)) + DWORD dwFlags; + IfFailRet(pMDImport->GetFieldDefProps(token, &dwFlags)); + + if (IsFdHasFieldRVA(dwFlags)) + { + LOG((LF_ENC, LL_INFO10000, "EACM::AEAC: Detected a FieldRVA\n")); + + ULONG dwFieldRVA; + IfFailRet(pMDImport->GetFieldRVA(token, &dwFieldRVA)); + + if (dwFieldRVA >= cbDeltaIL) + { + LOG((LF_ENC, LL_INFO10000, "EACM::AEAC: Failure RVA of %u with cbDeltaIL %u\n", dwFieldRVA, cbDeltaIL)); + IfFailRet(E_INVALIDARG); + } + + // The FieldRVA data is stored in the IL memory stream. This decision + // was made to avoid creating another stream of data. + SetDynamicRvaField(token, (TADDR)(&pLocalILMemory[dwFieldRVA])); + } + + pFieldDesc = LookupFieldDef(token); + if (pFieldDesc) { // Field already exists - just ignore for now continue; } + else + { + // Field is new - add it + IfFailRet(AddField(token)); + } - // Field is new - add it - IfFailGo(AddField(token)); break; } } @@ -269,10 +290,6 @@ HRESULT EditAndContinueModule::ApplyEditAndContinue( // Update the AvailableClassHash for reflection, etc. ensure that the new TypeRefs, AssemblyRefs and MethodDefs can be stored. ApplyMetaData(); -ErrExit: - if (pIMDInternalImportENC) - pIMDInternalImportENC->EnumClose(&enumENC); - return hr; } @@ -463,7 +480,7 @@ HRESULT EditAndContinueModule::AddField(mdFieldDef token) if (FAILED(hr)) { - LOG((LF_ENC, LL_INFO100, "**Error** EnCModule::AF can't find parent token for field token 0x%08x\n", token)); + LOG((LF_ENC, LL_INFO100, "**Error** EACM::AF can't find parent token for field token 0x%08x, hr: 0x%08x\n", token, hr)); return E_FAIL; } @@ -475,7 +492,7 @@ HRESULT EditAndContinueModule::AddField(mdFieldDef token) MethodTable * pParentType = LookupTypeDef(parentTypeDef).AsMethodTable(); if (pParentType == NULL) { - LOG((LF_ENC, LL_INFO100, "EnCModule::AF class 0x%08x not loaded (field 0x%08x), our work is done\n", parentTypeDef, token)); + LOG((LF_ENC, LL_INFO100, "EACM::AF class 0x%08x not loaded (field 0x%08x), our work is done\n", parentTypeDef, token)); return S_OK; } @@ -510,6 +527,33 @@ HRESULT EditAndContinueModule::AddField(mdFieldDef token) return hr; } +void EditAndContinueModule::SetDynamicRvaField(mdToken token, TADDR blobAddress) +{ + CONTRACTL + { + THROWS; + GC_NOTRIGGER; + MODE_COOPERATIVE; + } + CONTRACTL_END; + + // Reuse existing dynamic IL mechanism to store/map the data. + SetDynamicIL(token, blobAddress); +} + +TADDR EditAndContinueModule::GetDynamicRvaField(mdToken token) +{ + CONTRACTL + { + THROWS; + GC_NOTRIGGER; + } + CONTRACTL_END + + // Reuse existing dynamic IL mechanism to store/map the data. + return GetDynamicIL(token); +} + //--------------------------------------------------------------------------------------- // // JitUpdatedFunction - Jit the new version of a function for EnC. diff --git a/src/coreclr/vm/encee.h b/src/coreclr/vm/encee.h index 04a469c14fe8c9..c279298eac0a64 100644 --- a/src/coreclr/vm/encee.h +++ b/src/coreclr/vm/encee.h @@ -229,6 +229,7 @@ class EditAndContinueModule : public Module DWORD cbIL, BYTE *pIL); +private: // Called when a method has been modified (new IL) HRESULT UpdateMethod(MethodDesc *pMethod); @@ -238,9 +239,18 @@ class EditAndContinueModule : public Module // Called when a new field has been added to the module's metadata HRESULT AddField(mdFieldDef token); + // Set the data for a FieldRVA added by EnC + void SetDynamicRvaField(mdToken token, TADDR blobAddress); + +public: + // Get the data for a FieldRVA added by EnC + TADDR GetDynamicRvaField(mdToken token); + +private: // JIT the new version of a function for EnC PCODE JitUpdatedFunction(MethodDesc *pMD, T_CONTEXT *pContext); +public: // Remap execution to the latest version of an edited method HRESULT ResumeInUpdatedFunction(MethodDesc *pMD, void *oldDebuggerFuncHandle, diff --git a/src/coreclr/vm/field.cpp b/src/coreclr/vm/field.cpp index 2655dc62867b40..39415b8b96ecb6 100644 --- a/src/coreclr/vm/field.cpp +++ b/src/coreclr/vm/field.cpp @@ -243,12 +243,17 @@ PTR_VOID FieldDesc::GetStaticAddressHandle(PTR_VOID base) #ifdef DACCESS_COMPILE DacNotImpl(); #else + if (IsRVA()) + { + retVal = (void*)pModule->GetDynamicRvaField(pFD->GetMemberDef()); + } + else { GCX_COOP(); // This routine doesn't have a failure semantic - but Resolve*Field(...) does. // Something needs to be rethought here and I think it's E&C. CONTRACT_VIOLATION(ThrowsViolation|FaultViolation|GCViolation); //B#25680 (Fix Enc violations) - retVal = (void *)(pModule->ResolveOrAllocateField(NULL, pFD)); + retVal = (void*)(pModule->ResolveOrAllocateField(NULL, pFD)); } #endif // !DACCESS_COMPILE return retVal; diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index f65613c98ea46a..07f226a44ba985 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/src/coreclr/vm/reflectioninvocation.cpp @@ -1155,7 +1155,17 @@ extern "C" BOOL QCALLTYPE RuntimeFieldHandle_GetRVAFieldInfo(FieldDesc* pField, if (pField != NULL && pField->IsRVA()) { Module* pModule = pField->GetModule(); - *address = pModule->GetRvaField(pField->GetOffset()); + if (!pField->IsEnCNew()) + { + *address = pModule->GetRvaField(pField->GetOffset()); + } + else + { + _ASSERTE(pModule->IsEditAndContinueEnabled()); + EditAndContinueModule *pEnCModule = (EditAndContinueModule*)pModule; + *address = (void*)pEnCModule->GetDynamicRvaField(pField->GetMemberDef()); + } + *size = pField->LoadSize(); ret = TRUE; From 1308ed08de2ee12a20c45bf650e4bbc25f5f47b1 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Mon, 7 Apr 2025 20:09:27 -0700 Subject: [PATCH 2/7] Offline feedback --- src/coreclr/vm/encee.cpp | 22 ++++++++++------------ src/coreclr/vm/reflectioninvocation.cpp | 14 +------------- 2 files changed, 11 insertions(+), 25 deletions(-) diff --git a/src/coreclr/vm/encee.cpp b/src/coreclr/vm/encee.cpp index fdb0bbba6c14a6..08a9672c2f7b82 100644 --- a/src/coreclr/vm/encee.cpp +++ b/src/coreclr/vm/encee.cpp @@ -250,6 +250,13 @@ HRESULT EditAndContinueModule::ApplyEditAndContinue( // FieldDef token - add a new field LOG((LF_ENC, LL_INFO10000, "EACM::AEAC: Found field 0x%08x\n", token)); + pFieldDesc = LookupFieldDef(token); + if (pFieldDesc) + { + // Field already exists - just ignore for now + continue; + } + DWORD dwFlags; IfFailRet(pMDImport->GetFieldDefProps(token, &dwFlags)); @@ -271,17 +278,8 @@ HRESULT EditAndContinueModule::ApplyEditAndContinue( SetDynamicRvaField(token, (TADDR)(&pLocalILMemory[dwFieldRVA])); } - pFieldDesc = LookupFieldDef(token); - if (pFieldDesc) - { - // Field already exists - just ignore for now - continue; - } - else - { - // Field is new - add it - IfFailRet(AddField(token)); - } + // Field is new - add it + IfFailRet(AddField(token)); break; } @@ -545,7 +543,7 @@ TADDR EditAndContinueModule::GetDynamicRvaField(mdToken token) { CONTRACTL { - THROWS; + NOTHROW; GC_NOTRIGGER; } CONTRACTL_END diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index 07f226a44ba985..a7b3284da9d22f 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/src/coreclr/vm/reflectioninvocation.cpp @@ -1154,20 +1154,8 @@ extern "C" BOOL QCALLTYPE RuntimeFieldHandle_GetRVAFieldInfo(FieldDesc* pField, if (pField != NULL && pField->IsRVA()) { - Module* pModule = pField->GetModule(); - if (!pField->IsEnCNew()) - { - *address = pModule->GetRvaField(pField->GetOffset()); - } - else - { - _ASSERTE(pModule->IsEditAndContinueEnabled()); - EditAndContinueModule *pEnCModule = (EditAndContinueModule*)pModule; - *address = (void*)pEnCModule->GetDynamicRvaField(pField->GetMemberDef()); - } - + *address = pField->GetStaticAddressHandle(NULL); *size = pField->LoadSize(); - ret = TRUE; } From b75bd18cf9d8af52152812e3da0955b888c85888 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 9 Apr 2025 17:36:09 -0700 Subject: [PATCH 3/7] EnC for adding new FieldRVA entries works. --- .../Reflection/Metadata/MetadataUpdater.cs | 2 +- src/coreclr/debug/daccess/dacdbiimpl.cpp | 3 +- src/coreclr/debug/ee/controller.cpp | 10 +-- src/coreclr/vm/ceeload.cpp | 29 +++++++++ src/coreclr/vm/ceeload.h | 10 +++ src/coreclr/vm/encee.cpp | 27 -------- src/coreclr/vm/encee.h | 8 --- src/coreclr/vm/field.cpp | 64 +++++++++++------- src/coreclr/vm/field.h | 65 +++++++++++-------- src/coreclr/vm/memberload.cpp | 2 +- src/coreclr/vm/methodtablebuilder.cpp | 59 +++++++---------- 11 files changed, 152 insertions(+), 127 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Metadata/MetadataUpdater.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Metadata/MetadataUpdater.cs index 413a970afcf622..f0b6774ac45503 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Metadata/MetadataUpdater.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Metadata/MetadataUpdater.cs @@ -55,7 +55,7 @@ public static void ApplyUpdate(Assembly assembly, ReadOnlySpan metadataDel /// /// Returns the metadata update capabilities. /// - internal static string GetCapabilities() => "Baseline AddMethodToExistingType AddStaticFieldToExistingType AddInstanceFieldToExistingType NewTypeDefinition ChangeCustomAttributes UpdateParameters GenericUpdateMethod GenericAddMethodToExistingType GenericAddFieldToExistingType"; + internal static string GetCapabilities() => "Baseline AddMethodToExistingType AddStaticFieldToExistingType AddInstanceFieldToExistingType NewTypeDefinition ChangeCustomAttributes UpdateParameters GenericUpdateMethod GenericAddMethodToExistingType GenericAddFieldToExistingType AddFieldRva"; /// /// Returns true if the apply assembly update is enabled and available. diff --git a/src/coreclr/debug/daccess/dacdbiimpl.cpp b/src/coreclr/debug/daccess/dacdbiimpl.cpp index 5b65dc24e7ab2d..f4cca01d3a6a89 100644 --- a/src/coreclr/debug/daccess/dacdbiimpl.cpp +++ b/src/coreclr/debug/daccess/dacdbiimpl.cpp @@ -1579,8 +1579,7 @@ void DacDbiInterfaceImpl::ComputeFieldData(PTR_FieldDesc pFD, if (pFD->IsRVA()) { // RVA statics are relative to a base module address - DWORD offset = pFD->GetOffset(); - PTR_VOID addr = pFD->GetModule()->GetRvaField(offset); + PTR_VOID addr = pFD->GetStaticAddressHandle(NULL); if (pCurrentFieldData->OkToGetOrSetStaticAddress()) { pCurrentFieldData->SetStaticAddress(PTR_TO_TADDR(addr)); diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index e14e39d9a0ca90..fd7901f0d3ebc1 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -4268,7 +4268,7 @@ bool DebuggerController::DispatchNativeException(EXCEPTION_RECORD *pException, CONTRACTL_END; LOG((LF_CORDB, LL_EVERYTHING, "DispatchNativeException was called\n")); - LOG((LF_CORDB, LL_INFO10000, "Native exception at 0x%p, code=0x%8x, context=0x%p, er=0x%p\n", + LOG((LF_CORDB, LL_INFO10000, "Native exception at %p, code=0x%8x, context=%p, er=%p\n", pException->ExceptionAddress, dwCode, pContext, pException)); @@ -4477,7 +4477,7 @@ bool DebuggerController::DispatchNativeException(EXCEPTION_RECORD *pException, return FALSE; } pCurThread->SetThreadState(Thread::TS_SSToExitApcCallDone); - pCurThread->ResetThreadState(Thread::TS_SSToExitApcCall); + pCurThread->ResetThreadState(Thread::TS_SSToExitApcCall); DebuggerController::UnapplyTraceFlag(pCurThread); pCurThread->MarkForSuspensionAndWait(Thread::TS_DebugSuspendPending); } @@ -6061,7 +6061,7 @@ bool DebuggerStepper::TrapStep(ControllerStackInfo *info, bool in) fCallingIntoFunclet = IsAddrWithinMethodIncludingFunclet(ji, info->m_activeFrame.md, walker.GetNextIP()) && ((CORDB_ADDRESS)(SIZE_T)walker.GetNextIP() != ji->m_addrOfCode); #endif - // If we are stepping into a tail call that uses the StoreTailCallArgs + // If we are stepping into a tail call that uses the StoreTailCallArgs // we need to enable the method enter, otherwise it will behave like a resume if (in && IsTailCall(walker.GetNextIP(), info, TailCallFunctionType::StoreTailCallArgs)) { @@ -7600,9 +7600,9 @@ bool DebuggerStepper::TriggerSingleStep(Thread *thread, const BYTE *ip) // Sometimes we can get here with a callstack that is coming from an APC // this will disable the single stepping and incorrectly resume an app that the user // is stepping through. -#ifdef FEATURE_THREAD_ACTIVATION +#ifdef FEATURE_THREAD_ACTIVATION if ((thread->m_State & Thread::TS_DebugWillSync) == 0) -#endif +#endif // FEATURE_THREAD_ACTIVATION { DisableSingleStep(); } diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index 3494962fe425df..4003bd8135040d 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -932,6 +932,35 @@ TADDR Module::GetDynamicIL(mdToken token) return entry.m_il; } +#ifndef DACCESS_COMPILE +void Module::SetDynamicRvaField(mdToken token, TADDR blobAddress) +{ + CONTRACTL + { + THROWS; + GC_NOTRIGGER; + MODE_COOPERATIVE; + } + CONTRACTL_END; + + // Reuse existing dynamic IL mechanism to store/map the data. + SetDynamicIL(token, blobAddress); +} +#endif // !DACCESS_COMPILE + +TADDR Module::GetDynamicRvaField(mdToken token) +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + } + CONTRACTL_END + + // Reuse existing dynamic IL mechanism to store/map the data. + return GetDynamicIL(token); +} + #if !defined(DACCESS_COMPILE) //--------------------------------------------------------------------------------------- // diff --git a/src/coreclr/vm/ceeload.h b/src/coreclr/vm/ceeload.h index 431b84085ed7ea..0d69cbb96cf809 100644 --- a/src/coreclr/vm/ceeload.h +++ b/src/coreclr/vm/ceeload.h @@ -1524,9 +1524,19 @@ class Module : public ModuleBase void StartUnload(); public: +#ifndef DACCESS_COMPILE void SetDynamicIL(mdToken token, TADDR blobAddress); +#endif // !DACCESS_COMPILE TADDR GetDynamicIL(mdToken token); +protected: +#ifndef DACCESS_COMPILE + void SetDynamicRvaField(mdToken token, TADDR blobAddress); +#endif // !DACCESS_COMPILE + +public: + TADDR GetDynamicRvaField(mdToken token); + // store and retrieve the instrumented IL offset mapping for a particular method #if !defined(DACCESS_COMPILE) void SetInstrumentedILOffsetMapping(mdMethodDef token, InstrumentedILOffsetMapping mapping); diff --git a/src/coreclr/vm/encee.cpp b/src/coreclr/vm/encee.cpp index 08a9672c2f7b82..a4dbce3ba1a04f 100644 --- a/src/coreclr/vm/encee.cpp +++ b/src/coreclr/vm/encee.cpp @@ -525,33 +525,6 @@ HRESULT EditAndContinueModule::AddField(mdFieldDef token) return hr; } -void EditAndContinueModule::SetDynamicRvaField(mdToken token, TADDR blobAddress) -{ - CONTRACTL - { - THROWS; - GC_NOTRIGGER; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - // Reuse existing dynamic IL mechanism to store/map the data. - SetDynamicIL(token, blobAddress); -} - -TADDR EditAndContinueModule::GetDynamicRvaField(mdToken token) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - } - CONTRACTL_END - - // Reuse existing dynamic IL mechanism to store/map the data. - return GetDynamicIL(token); -} - //--------------------------------------------------------------------------------------- // // JitUpdatedFunction - Jit the new version of a function for EnC. diff --git a/src/coreclr/vm/encee.h b/src/coreclr/vm/encee.h index c279298eac0a64..53079f0052769b 100644 --- a/src/coreclr/vm/encee.h +++ b/src/coreclr/vm/encee.h @@ -239,14 +239,6 @@ class EditAndContinueModule : public Module // Called when a new field has been added to the module's metadata HRESULT AddField(mdFieldDef token); - // Set the data for a FieldRVA added by EnC - void SetDynamicRvaField(mdToken token, TADDR blobAddress); - -public: - // Get the data for a FieldRVA added by EnC - TADDR GetDynamicRvaField(mdToken token); - -private: // JIT the new version of a function for EnC PCODE JitUpdatedFunction(MethodDesc *pMD, T_CONTEXT *pContext); diff --git a/src/coreclr/vm/field.cpp b/src/coreclr/vm/field.cpp index 39415b8b96ecb6..b043f6761bceb9 100644 --- a/src/coreclr/vm/field.cpp +++ b/src/coreclr/vm/field.cpp @@ -227,25 +227,27 @@ PTR_VOID FieldDesc::GetStaticAddressHandle(PTR_VOID base) } CONTRACTL_END - _ASSERTE(IsStatic()); + LOG((LF_CORDB, LL_INFO1000, "FD::GSAH: this=%p, token=0x%08x, base=%p\n", this, GetMemberDef(), (void*)base)); + + PTR_VOID retVal = NULL; + #ifdef FEATURE_METADATA_UPDATER if (IsEnCNew()) { - EnCFieldDesc * pFD = dac_cast(this); - _ASSERTE_IMPL(pFD->GetApproxEnclosingMethodTable()->SanityCheck()); - _ASSERTE(pFD->GetModule()->IsEditAndContinueEnabled()); - - EditAndContinueModule *pModule = (EditAndContinueModule*)pFD->GetModule(); - _ASSERTE(pModule->IsEditAndContinueEnabled()); - - PTR_VOID retVal = NULL; + LOG((LF_ENC, LL_INFO1000, "FD::GSAH: EnC field\n")); #ifdef DACCESS_COMPILE DacNotImpl(); #else + EnCFieldDesc * pFD = dac_cast(this); + _ASSERTE_IMPL(pFD->GetApproxEnclosingMethodTable()->SanityCheck()); + _ASSERTE(pFD->GetModule()->IsEditAndContinueEnabled()); + + EditAndContinueModule *pEnCModule = (EditAndContinueModule*)pFD->GetModule(); if (IsRVA()) { - retVal = (void*)pModule->GetDynamicRvaField(pFD->GetMemberDef()); + LOG((LF_ENC, LL_INFO1000, "FD::GSAH: EnC - RVA\n")); + retVal = (void*)pEnCModule->GetDynamicRvaField(pFD->GetMemberDef()); } else { @@ -253,28 +255,46 @@ PTR_VOID FieldDesc::GetStaticAddressHandle(PTR_VOID base) // This routine doesn't have a failure semantic - but Resolve*Field(...) does. // Something needs to be rethought here and I think it's E&C. CONTRACT_VIOLATION(ThrowsViolation|FaultViolation|GCViolation); //B#25680 (Fix Enc violations) - retVal = (void*)(pModule->ResolveOrAllocateField(NULL, pFD)); + retVal = (void*)(pEnCModule->ResolveOrAllocateField(NULL, pFD)); } #endif // !DACCESS_COMPILE + + LOG((LF_ENC, LL_INFO1000, "FD::GSAH: EnC - retVal=%p\n", (void*)retVal)); return retVal; } #endif // FEATURE_METADATA_UPDATER - if (IsRVA()) - { - Module* pModule = GetModule(); - PTR_VOID ret = pModule->GetRvaField(GetOffset()); - - _ASSERTE(pModule->IsReflectionEmit() || !pModule->IsRvaFieldTls(GetOffset())); + DWORD offset = GetOffset(); + LOG((LF_CORDB, LL_INFO1000, "FD::GSAH: Offset=0x%x\n", offset)); - return(ret); + if (!IsRVA()) + { + CONSISTENCY_CHECK(CheckPointer(base)); + retVal = PTR_VOID(dac_cast(base) + offset); } + else + { + LOG((LF_CORDB, LL_INFO1000, "FD::GSAH: RVA field\n")); - CONSISTENCY_CHECK(CheckPointer(base)); - - PTR_VOID ret = PTR_VOID(dac_cast(base) + GetOffset()); + Module* pModule = GetModule(); + if (offset == FIELD_OFFSET_DYNAMIC_RVA) + { +#ifdef FEATURE_METADATA_UPDATER + _ASSERTE(!IsEnCNew()); + _ASSERTE(pModule->IsEditAndContinueEnabled()); + LOG((LF_ENC, LL_INFO1000, "FD::GSAH: Dynamic (EnC) - RVA\n")); + retVal = PTR_VOID(pModule->GetDynamicRvaField(GetMemberDef())); +#endif // FEATURE_METADATA_UPDATER + } + else + { + _ASSERTE(pModule->IsReflectionEmit() || !pModule->IsRvaFieldTls(offset)); + retVal = pModule->GetRvaField(offset); + } + } - return ret; + LOG((LF_CORDB, LL_INFO1000, "FD::GSAH: retVal=%p\n", (void*)retVal)); + return retVal; } diff --git a/src/coreclr/vm/field.h b/src/coreclr/vm/field.h index a3a068f32604cc..bfb0e69dd7b874 100644 --- a/src/coreclr/vm/field.h +++ b/src/coreclr/vm/field.h @@ -22,7 +22,9 @@ // Offset to indicate an EnC added field. They don't have offsets as aren't placed in the object. #define FIELD_OFFSET_NEW_ENC (FIELD_OFFSET_MAX-4) #define FIELD_OFFSET_BIG_RVA (FIELD_OFFSET_MAX-5) -#define FIELD_OFFSET_LAST_REAL_OFFSET (FIELD_OFFSET_MAX-6) // real fields have to be smaller than this +// Offset to indicate a FieldRVA that is added by EnC, but whose enclosing type is not yet loaded. +#define FIELD_OFFSET_DYNAMIC_RVA (FIELD_OFFSET_MAX-6) +#define FIELD_OFFSET_LAST_REAL_OFFSET (FIELD_OFFSET_MAX-7) // real fields have to be smaller than this // @@ -156,8 +158,16 @@ class FieldDesc return m_prot; } - // Please only use this in a path that you have already guaranteed - // the assert is true + // This is the raw offset of the field in the object. It doesn't + // do any checks to see if the field is a real field or not. + DWORD GetOffsetRaw() + { + LIMITED_METHOD_CONTRACT; + return m_dwOffset; + } + + // Please only use this in a path that you have already guaranteed + // the assert is true DWORD GetOffsetUnsafe() { LIMITED_METHOD_CONTRACT; @@ -173,34 +183,33 @@ class FieldDesc // Note FieldDescs are no longer on "hot" paths so the optimized code here // does not look necessary. + if (m_dwOffset == FIELD_OFFSET_BIG_RVA) + return CallMDImportForFieldRVA(); - if (m_dwOffset != FIELD_OFFSET_BIG_RVA) { - // Assert that the big RVA case handling doesn't get out of sync - // with the normal RVA case. -#ifdef _DEBUG - // The OutOfLine_BigRVAOffset() can't be correctly evaluated during the time - // that we repurposed m_pMTOfEnclosingClass for holding the field size - // I don't see any good way to determine when this is so hurray for - // heuristics! - // - // As of 4/11/2012 I could repro this by turning on the COMPLUS log and - // the LOG() at line methodtablebuilder.cpp:7845 - // MethodTableBuilder::PlaceRegularStaticFields() calls GetOffset() - if((DWORD)reinterpret_cast(m_pMTOfEnclosingClass) > 16) - { - _ASSERTE(!this->IsRVA() || (m_dwOffset == OutOfLine_BigRVAOffset())); - } -#endif - return m_dwOffset; - } + // Assert that the big RVA case handling doesn't get out of sync + // with the normal RVA case. - return OutOfLine_BigRVAOffset(); +#ifndef DACCESS_COMPILE + // The CallMDImportForFieldRVA() can't be correctly evaluated when we + // reuse the m_pMTOfEnclosingClass field for holding the field size. + // + // There are calls to FieldDesc::GetOffset() during type loading. + if ((DWORD)reinterpret_cast(m_pMTOfEnclosingClass) > 16) + { + _ASSERTE(!IsRVA() + || m_dwOffset == FIELD_OFFSET_DYNAMIC_RVA + || m_dwOffset == CallMDImportForFieldRVA()); + } +#endif // !DACCESS_COMPILE + return m_dwOffset; } - DWORD OutOfLine_BigRVAOffset() + DWORD CallMDImportForFieldRVA() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE(IsRVA()); + DWORD rva; // I'm discarding a potential error here. According to the code in MDInternalRO.cpp, @@ -235,13 +244,17 @@ class FieldDesc } // Okay, we've stolen too many bits from FieldDescs. In the RVA case, there's no - // reason to believe they will be limited to 22 bits. So use a sentinel for the + // reason to believe they will be limited to . So use a sentinel for the // huge cases, and recover them from metadata on-demand. void SetOffsetRVA(DWORD dwOffset) { LIMITED_METHOD_CONTRACT; - m_dwOffset = (dwOffset > FIELD_OFFSET_LAST_REAL_OFFSET) + // The FIELD_OFFSET_DYNAMIC_RVA is a special case for EnC added fields when the + // type they are on is not yet loaded. + // The FIELD_OFFSET_BIG_RVA is a special case for large RVA fields that + // don't fit into FIELD_OFFSET_MAX of the m_dwOffset field. + m_dwOffset = (dwOffset > FIELD_OFFSET_LAST_REAL_OFFSET && dwOffset != FIELD_OFFSET_DYNAMIC_RVA) ? FIELD_OFFSET_BIG_RVA : dwOffset; } diff --git a/src/coreclr/vm/memberload.cpp b/src/coreclr/vm/memberload.cpp index 472847f041790b..202008d3ef0bcc 100644 --- a/src/coreclr/vm/memberload.cpp +++ b/src/coreclr/vm/memberload.cpp @@ -697,7 +697,7 @@ FieldDesc* MemberLoader::GetFieldDescFromFieldDef(Module *pModule, if (pModule->IsEditAndContinueEnabled() && pFD->IsEnCNew()) { EnCFieldDesc *pEnCFD = (EnCFieldDesc*)pFD; - // we may not have the full FieldDesc info at applyEnC time becuase we don't + // we may not have the full FieldDesc info at applyEnC time because we don't // have a thread so can't do things like load classes (due to possible exceptions) if (pEnCFD->NeedsFixup()) { diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 16186cb5b08f1a..4111f2a759af13 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -4428,27 +4428,16 @@ VOID MethodTableBuilder::InitializeFieldDescs(FieldDesc *pFieldDescList, // The PE should be loaded by now. _ASSERT(GetModule()->GetPEAssembly()->IsLoaded()); - DWORD fldSize; - if (FieldDescElementType == ELEMENT_TYPE_VALUETYPE) - { - if (IsSelfRef(pByValueClass)) - { - _ASSERTE(bmtFP->fHasSelfReferencingStaticValueTypeField_WithRVA); - - // We do not known the size yet - _ASSERTE(bmtFP->NumInstanceFieldBytes == 0); - // We will check just the RVA with size 0 now, the full size verification will happen in code:VerifySelfReferencingStaticValueTypeFields_WithRVA - fldSize = 0; - } - else - { - fldSize = pByValueClass->GetNumInstanceFieldBytes(); - } - } - else +#ifdef FEATURE_METADATA_UPDATER + // This is a special case for EnC. The RVA field is not actually in the image, but + // is instead registered in a dynamic map. We need to set the RVA to a special + // value so when the address is looked up, it will be found in the dynamic map. + if (GetModule()->IsEditAndContinueEnabled()) { - fldSize = GetSizeForCorElementType(FieldDescElementType); + if (GetModule()->GetDynamicRvaField(pFD->GetMemberDef()) != NULL) + rva = FIELD_OFFSET_DYNAMIC_RVA; } +#endif // FEATURE_METADATA_UPDATER pFD->SetOffsetRVA(rva); } @@ -7870,7 +7859,6 @@ VOID MethodTableBuilder::PlaceRegularStaticFields() bmtFP->NumRegularStaticFieldsOfSize[i] = 0; } - if (dwCumulativeStaticFieldPos > FIELD_OFFSET_LAST_REAL_OFFSET) BuildMethodTableThrowException(IDS_CLASSLOAD_GENERAL); @@ -7894,8 +7882,8 @@ VOID MethodTableBuilder::PlaceRegularStaticFields() for (i = 0; i < bmtEnumFields->dwNumStaticFields - bmtEnumFields->dwNumThreadStaticFields; i++) { FieldDesc * pCurField = &pFieldDescList[bmtEnumFields->dwNumInstanceFields+i]; - DWORD dwLog2FieldSize = (DWORD)(DWORD_PTR&)pCurField->m_pMTOfEnclosingClass; // log2(field size) - DWORD dwOffset = (DWORD) pCurField->m_dwOffset; // offset or type of field + DWORD dwLog2FieldSize = (DWORD)(DWORD_PTR)pCurField->m_pMTOfEnclosingClass; // log2(field size) + DWORD dwOffset = (DWORD) pCurField->GetOffsetRaw(); // offset or type of field switch (dwOffset) { @@ -7903,7 +7891,7 @@ VOID MethodTableBuilder::PlaceRegularStaticFields() // Place GC reference static field pCurField->SetOffset(dwCumulativeStaticGCFieldPos + dwGCOffset); dwCumulativeStaticGCFieldPos += 1<GetOffset())); + LOG((LF_CLASSLOADER, LL_INFO10000, "STATICS: Field placed at GC offset\n")); break; @@ -7911,7 +7899,7 @@ VOID MethodTableBuilder::PlaceRegularStaticFields() // Place boxed GC reference static field pCurField->SetOffset(dwCumulativeStaticBoxFieldPos + dwGCOffset); dwCumulativeStaticBoxFieldPos += 1<GetOffset())); + LOG((LF_CLASSLOADER, LL_INFO10000, "STATICS: Field placed at GC offset\n")); break; @@ -7921,15 +7909,16 @@ VOID MethodTableBuilder::PlaceRegularStaticFields() (bmtFP->NumRegularStaticFieldsOfSize[dwLog2FieldSize] << dwLog2FieldSize) + dwNonGCOffset); bmtFP->NumRegularStaticFieldsOfSize[dwLog2FieldSize]++; - LOG((LF_CLASSLOADER, LL_INFO10000, "STATICS: Field placed at non GC offset 0x%x\n", pCurField->GetOffset())); + LOG((LF_CLASSLOADER, LL_INFO10000, "STATICS: Field placed at non GC offset\n")); break; default: // RVA field + LOG((LF_CLASSLOADER, LL_INFO10000, "STATICS: RVA field\n")); break; } - LOG((LF_CLASSLOADER, LL_INFO1000000, "Offset of %s: %i\n", pCurField->m_debugName, pCurField->GetOffset())); + LOG((LF_CLASSLOADER, LL_INFO10000, "STATICS: Offset of %s: 0x%x\n", pCurField->m_debugName, pCurField->GetOffsetRaw())); } _ASSERTE(dwNonGCOffset == 0 || (dwNonGCOffset == sizeof(TADDR) * 2)); @@ -7937,14 +7926,13 @@ VOID MethodTableBuilder::PlaceRegularStaticFields() LOG((LF_CLASSLOADER, LL_INFO10000, "STATICS: Static field bytes needed %i\n", bmtProp->dwNonGCRegularStaticFieldBytes)); } - VOID MethodTableBuilder::PlaceThreadStaticFields() { STANDARD_VM_CONTRACT; DWORD i; - LOG((LF_CLASSLOADER, LL_INFO10000, "STATICS: Placing ThreadStatics for %s\n", this->GetDebugClassName())); + LOG((LF_CLASSLOADER, LL_INFO10000, "THREAD STATICS: Placing ThreadStatics for %s\n", this->GetDebugClassName())); // // Place gc refs and value types first, as they need to have handles created for them. @@ -7998,8 +7986,8 @@ VOID MethodTableBuilder::PlaceThreadStaticFields() for (i = 0; i < bmtEnumFields->dwNumThreadStaticFields; i++) { FieldDesc * pCurField = &pFieldDescList[bmtEnumFields->dwNumInstanceFields + bmtEnumFields->dwNumStaticFields - bmtEnumFields->dwNumThreadStaticFields + i]; - DWORD dwLog2FieldSize = (DWORD)(DWORD_PTR&)pCurField->m_pMTOfEnclosingClass; // log2(field size) - DWORD dwOffset = (DWORD) pCurField->m_dwOffset; // offset or type of field + DWORD dwLog2FieldSize = (DWORD)(DWORD_PTR)pCurField->m_pMTOfEnclosingClass; // log2(field size) + DWORD dwOffset = (DWORD) pCurField->GetOffsetRaw(); // offset or type of field switch (dwOffset) { @@ -8007,7 +7995,7 @@ VOID MethodTableBuilder::PlaceThreadStaticFields() // Place GC reference static field pCurField->SetOffset(dwCumulativeStaticGCFieldPos + dwGCOffset); dwCumulativeStaticGCFieldPos += 1<GetOffset())); + LOG((LF_CLASSLOADER, LL_INFO10000, "THREAD STATICS: Field placed at GC offset\n")); break; @@ -8015,7 +8003,7 @@ VOID MethodTableBuilder::PlaceThreadStaticFields() // Place boxed GC reference static field pCurField->SetOffset(dwCumulativeStaticBoxFieldPos + dwGCOffset); dwCumulativeStaticBoxFieldPos += 1<GetOffset())); + LOG((LF_CLASSLOADER, LL_INFO10000, "THREAD STATICS: Field placed at GC offset\n")); break; @@ -8025,15 +8013,16 @@ VOID MethodTableBuilder::PlaceThreadStaticFields() (bmtFP->NumThreadStaticFieldsOfSize[dwLog2FieldSize] << dwLog2FieldSize) + dwNonGCOffset); bmtFP->NumThreadStaticFieldsOfSize[dwLog2FieldSize]++; - LOG((LF_CLASSLOADER, LL_INFO10000, "THREAD STATICS: Field placed at non GC offset 0x%x\n", pCurField->GetOffset())); + LOG((LF_CLASSLOADER, LL_INFO10000, "THREAD STATICS: Field placed at non GC offset\n")); break; default: // RVA field + LOG((LF_CLASSLOADER, LL_INFO10000, "THREAD STATICS: RVA field\n")); break; } - LOG((LF_CLASSLOADER, LL_INFO1000000, "Offset of %s: %i\n", pCurField->m_debugName, pCurField->GetOffset())); + LOG((LF_CLASSLOADER, LL_INFO10000, "THREAD STATICS: Offset of %s: 0x%x\n", pCurField->m_debugName, pCurField->GetOffsetRaw())); } if (dwCumulativeStaticFieldPos != 0) @@ -8045,7 +8034,7 @@ VOID MethodTableBuilder::PlaceThreadStaticFields() { bmtProp->dwNonGCThreadStaticFieldBytes = 0; } - LOG((LF_CLASSLOADER, LL_INFO10000, "STATICS: ThreadStatic field bytes needed (0 is normal for non dynamic case)%i\n", bmtProp->dwNonGCThreadStaticFieldBytes)); + LOG((LF_CLASSLOADER, LL_INFO10000, "THREAD STATICS: ThreadStatic field bytes needed (0 is normal for non dynamic case)%i\n", bmtProp->dwNonGCThreadStaticFieldBytes)); } //******************************************************************************* From 2f5d3d5bec464c3f8549f724b2f578186b55a152 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 9 Apr 2025 22:04:18 -0700 Subject: [PATCH 4/7] Update JIT test with new max field offset value. --- .../JIT/Regression/CLR-x86-JIT/V2.0-Beta2/b091942/b091942.il | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/JIT/Regression/CLR-x86-JIT/V2.0-Beta2/b091942/b091942.il b/src/tests/JIT/Regression/CLR-x86-JIT/V2.0-Beta2/b091942/b091942.il index 7f8b2fc100a944..68a450464dd0c5 100644 --- a/src/tests/JIT/Regression/CLR-x86-JIT/V2.0-Beta2/b091942/b091942.il +++ b/src/tests/JIT/Regression/CLR-x86-JIT/V2.0-Beta2/b091942/b091942.il @@ -19,7 +19,7 @@ .class public explicit ansi beforefieldinit BigClass { .field [0] public int32 firstField - .field [0x7FFFFF9] public int32 lastField + .field [0x7FFFFF8] public int32 lastField // Offset should match FIELD_OFFSET_LAST_REAL_OFFSET in runtime. .method public hidebysig specialname rtspecialname instance void .ctor() cil managed From 788e6f9ca5a6d2df7b4761342577b0d8db8752f4 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 11 Apr 2025 15:06:19 -0700 Subject: [PATCH 5/7] Feedback. Logging updates. --- src/coreclr/vm/ceeload.cpp | 2 +- src/coreclr/vm/encee.cpp | 58 +++++++++++++++++++-------- src/coreclr/vm/field.h | 17 ++++++-- src/coreclr/vm/methodtablebuilder.cpp | 18 ++++----- 4 files changed, 64 insertions(+), 31 deletions(-) diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index 4003bd8135040d..767f0d5b0ad23c 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -868,7 +868,7 @@ void Module::InitializeDynamicILCrst() // Add a (token, address) pair to the table of IL blobs for reflection/dynamics // Arguments: // Input: -// token method token +// token method token or field token (See SetDynamicRvaField) // blobAddress address of the start of the IL blob address, including the header // Output: not explicit, but if the pair was not already in the table it will be added. // Does not add duplicate tokens to the table. diff --git a/src/coreclr/vm/encee.cpp b/src/coreclr/vm/encee.cpp index a4dbce3ba1a04f..10c61a190250e5 100644 --- a/src/coreclr/vm/encee.cpp +++ b/src/coreclr/vm/encee.cpp @@ -209,18 +209,30 @@ HRESULT EditAndContinueModule::ApplyEditAndContinue( FieldDesc* pFieldDesc; while (pIMDInternalImportENC->EnumNext(&enumENC, &token)) { - STRESS_LOG1(LF_ENC, LL_INFO100, "EACM::AEAC: updated token 0x%08x\n", token); + STRESS_LOG1(LF_ENC, LL_INFO10000, "EACM::AEAC: Next token 0x%08x\n", token); switch (TypeFromToken(token)) { case mdtMethodDef: // MethodDef token - update/add a method - LOG((LF_ENC, LL_INFO10000, "EACM::AEAC: Found method 0x%08x\n", token)); ULONG dwMethodRVA; - DWORD dwMethodFlags; - IfFailRet(pMDImport->GetMethodImplProps(token, &dwMethodRVA, &dwMethodFlags)); + DWORD dwMethodImplFlags; + IfFailRet(pMDImport->GetMethodImplProps(token, &dwMethodRVA, &dwMethodImplFlags)); + +#ifdef LOGGING + if (LoggingEnabled()) + { + LPCSTR szMethodName; + IfFailRet(pMDImport->GetNameOfMethodDef(token, &szMethodName)); + + DWORD dwMethodFlags; + IfFailRet(pMDImport->GetMethodDefProps(token, &dwMethodFlags)); + + LOG((LF_ENC, LL_INFO10000, "EACM::AEAC: Found method '%s' tok:0x%08x, RVA:%u, MethodAttrs:0x%08x, MethodImplAttrs:0x%08x\n", szMethodName, token, dwMethodRVA, dwMethodFlags, dwMethodImplFlags)); + } +#endif // LOGGING if (dwMethodRVA >= cbDeltaIL) { @@ -248,7 +260,19 @@ HRESULT EditAndContinueModule::ApplyEditAndContinue( case mdtFieldDef: // FieldDef token - add a new field - LOG((LF_ENC, LL_INFO10000, "EACM::AEAC: Found field 0x%08x\n", token)); + +#ifdef LOGGING + if (LoggingEnabled()) + { + LPCSTR szFieldName; + IfFailRet(pMDImport->GetNameOfFieldDef(token, &szFieldName)); + + DWORD dwFieldFlags; + IfFailRet(pMDImport->GetFieldDefProps(token, &dwFieldFlags)); + + LOG((LF_ENC, LL_INFO10000, "EACM::AEAC: Found field '%s' tok:0x%08x, FieldAttrs:0x%08x\n", szFieldName, token, dwFieldFlags)); + } +#endif // LOGGING pFieldDesc = LookupFieldDef(token); if (pFieldDesc) @@ -401,7 +425,7 @@ HRESULT EditAndContinueModule::AddMethod(mdMethodDef token) HRESULT hr = GetMDImport()->GetParentToken(token, &parentTypeDef); if (FAILED(hr)) { - LOG((LF_ENC, LL_INFO100, "**Error** EnCModule::AM can't find parent token for method token %08x\n", token)); + LOG((LF_ENC, LL_INFO100, "**Error** EACM::AM can't find parent token for method token %08x\n", token)); return E_FAIL; } @@ -411,7 +435,7 @@ HRESULT EditAndContinueModule::AddMethod(mdMethodDef token) { // Class isn't loaded yet, don't have to modify any existing EE data structures beyond the metadata. // Just notify debugger and return. - LOG((LF_ENC, LL_INFO100, "EnCModule::AM class %08x not loaded (method %08x), our work is done\n", parentTypeDef, token)); + LOG((LF_ENC, LL_INFO100, "EACM::AM class %08x not loaded (method %08x), our work is done\n", parentTypeDef, token)); if (CORDebuggerAttached()) { hr = g_pDebugInterface->UpdateNotYetLoadedFunction(token, this, m_applyChangesCount); @@ -547,7 +571,7 @@ PCODE EditAndContinueModule::JitUpdatedFunction( MethodDesc *pMD, } CONTRACTL_END; - LOG((LF_ENC, LL_INFO100, "EnCModule::JitUpdatedFunction for %s::%s\n", + LOG((LF_ENC, LL_INFO100, "EACM::JitUpdatedFunction for %s::%s\n", pMD->m_pszDebugClassName, pMD->m_pszDebugMethodName)); PCODE jittedCode = (PCODE)NULL; @@ -584,13 +608,13 @@ PCODE EditAndContinueModule::JitUpdatedFunction( MethodDesc *pMD, EX_TRY { if (pMD->IsPointingToNativeCode()) { - LOG((LF_ENC, LL_INFO100, "EnCModule::ResumeInUpdatedFunction %p already JITed\n", pMD)); + LOG((LF_ENC, LL_INFO100, "EACM::ResumeInUpdatedFunction %p already JITed\n", pMD)); } else { GCX_PREEMP(); pMD->DoPrestub(NULL); - LOG((LF_ENC, LL_INFO100, "EnCModule::ResumeInUpdatedFunction JIT of %p successful\n", pMD)); + LOG((LF_ENC, LL_INFO100, "EACM::ResumeInUpdatedFunction JIT of %p successful\n", pMD)); } jittedCode = pMD->GetNativeCode(); } EX_CATCH { @@ -605,7 +629,7 @@ PCODE EditAndContinueModule::JitUpdatedFunction( MethodDesc *pMD, SString errorMessage; GetExceptionMessage(GET_THROWABLE(), exceptionMessage); errorMessage.AppendASCII("**Error: Probable rude edit.**\n\n" - "EnCModule::JITUpdatedFunction JIT failed with the following exception:\n\n"); + "EACM::JITUpdatedFunction JIT failed with the following exception:\n\n"); errorMessage.Append(exceptionMessage); DbgAssertDialog(__FILE__, __LINE__, errorMessage.GetUTF8()); LOG((LF_ENC, LL_INFO100, errorMessage.GetUTF8())); @@ -650,7 +674,7 @@ HRESULT EditAndContinueModule::ResumeInUpdatedFunction( #if defined(TARGET_ARM) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) return E_NOTIMPL; #else - LOG((LF_ENC, LL_INFO100, "EnCModule::ResumeInUpdatedFunction for %s::%s at IL offset 0x%zx\n", + LOG((LF_ENC, LL_INFO100, "EACM::ResumeInUpdatedFunction for %s::%s at IL offset 0x%zx\n", pMD->m_pszDebugClassName, pMD->m_pszDebugMethodName, newILOffset)); #ifdef _DEBUG @@ -729,7 +753,7 @@ HRESULT EditAndContinueModule::ResumeInUpdatedFunction( // Ask the EECodeManager to actually fill in the context and stack for the new frame so that // values of locals etc. are preserved. - LOG((LF_ENC, LL_INFO100, "EnCModule::ResumeInUpdatedFunction calling FixContextAndResume oldNativeOffset: 0x%x, newNativeOffset: 0x%x," + LOG((LF_ENC, LL_INFO100, "EACM::ResumeInUpdatedFunction calling FixContextAndResume oldNativeOffset: 0x%x, newNativeOffset: 0x%x," "oldFrameSize: 0x%x, newFrameSize: 0x%x\n", oldCodeInfo.GetRelOffset(), newCodeInfo.GetRelOffset(), oldFrameSize, newFrameSize)); @@ -740,7 +764,7 @@ HRESULT EditAndContinueModule::ResumeInUpdatedFunction( &newCodeInfo); // At this point we shouldn't have failed, so this is genuinely erroneous. - LOG((LF_ENC, LL_ERROR, "**Error** EnCModule::ResumeInUpdatedFunction returned from ResumeAtJit")); + LOG((LF_ENC, LL_ERROR, "**Error** EACM::ResumeInUpdatedFunction returned from ResumeAtJit")); _ASSERTE(!"Should not return from FixContextAndResume()"); // If we fail for any reason we have already potentially trashed with new locals and we have also unwound any @@ -846,14 +870,14 @@ NOINLINE void EditAndContinueModule::FixContextAndResume( // "gracefully" (it's all relative). if (FAILED(hr)) { - LOG((LF_ENC, LL_INFO100, "**Error** EnCModule::ResumeInUpdatedFunction for FixContextForEnC failed\n")); + LOG((LF_ENC, LL_INFO100, "**Error** EACM::ResumeInUpdatedFunction for FixContextForEnC failed\n")); EEPOLICY_HANDLE_FATAL_ERROR(hr); } // Set the new IP // Note that all we're really doing here is setting the IP register. We unfortunately don't // share any code with the implementation of debugger SetIP, despite the similarities. - LOG((LF_ENC, LL_INFO100, "EnCModule::ResumeInUpdatedFunction: Resume at EIP=%p\n", pNewCodeInfo->GetCodeAddress())); + LOG((LF_ENC, LL_INFO100, "EACM::ResumeInUpdatedFunction: Resume at EIP=%p\n", pNewCodeInfo->GetCodeAddress())); Thread *pCurThread = GetThread(); pCurThread->SetFilterContext(pContext); @@ -884,7 +908,7 @@ NOINLINE void EditAndContinueModule::FixContextAndResume( #endif // OUT_OF_PROCESS_SETTHREADCONTEXT // At this point we shouldn't have failed, so this is genuinely erroneous. - LOG((LF_ENC, LL_ERROR, "**Error** EnCModule::ResumeInUpdatedFunction returned from ResumeAtJit")); + LOG((LF_ENC, LL_ERROR, "**Error** EACM::ResumeInUpdatedFunction returned from ResumeAtJit")); _ASSERTE(!"Should not return from ResumeAtJit()"); } #endif // #ifdef FEATURE_REMAP_FUNCTION diff --git a/src/coreclr/vm/field.h b/src/coreclr/vm/field.h index bfb0e69dd7b874..bf10e953281fc4 100644 --- a/src/coreclr/vm/field.h +++ b/src/coreclr/vm/field.h @@ -160,6 +160,8 @@ class FieldDesc // This is the raw offset of the field in the object. It doesn't // do any checks to see if the field is a real field or not. + // It should be only used during type loading to handle temporary offset + // values like FIELD_OFFSET_UNPLACED. DWORD GetOffsetRaw() { LIMITED_METHOD_CONTRACT; @@ -244,21 +246,28 @@ class FieldDesc } // Okay, we've stolen too many bits from FieldDescs. In the RVA case, there's no - // reason to believe they will be limited to . So use a sentinel for the + // reason to believe they will be limited to FIELD_OFFSET_MAX. So use a sentinel for the // huge cases, and recover them from metadata on-demand. void SetOffsetRVA(DWORD dwOffset) { LIMITED_METHOD_CONTRACT; - // The FIELD_OFFSET_DYNAMIC_RVA is a special case for EnC added fields when the - // type they are on is not yet loaded. // The FIELD_OFFSET_BIG_RVA is a special case for large RVA fields that // don't fit into FIELD_OFFSET_MAX of the m_dwOffset field. - m_dwOffset = (dwOffset > FIELD_OFFSET_LAST_REAL_OFFSET && dwOffset != FIELD_OFFSET_DYNAMIC_RVA) + m_dwOffset = (dwOffset > FIELD_OFFSET_LAST_REAL_OFFSET) ? FIELD_OFFSET_BIG_RVA : dwOffset; } + void SetDynamicRVA() + { + LIMITED_METHOD_CONTRACT; + + // The FIELD_OFFSET_DYNAMIC_RVA is a special case for EnC added fields when the + // type they are on is not yet loaded. + m_dwOffset = FIELD_OFFSET_DYNAMIC_RVA; + } + DWORD IsStatic() const { LIMITED_METHOD_DAC_CONTRACT; diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 4111f2a759af13..65fb54e76b993c 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -4421,10 +4421,6 @@ VOID MethodTableBuilder::InitializeFieldDescs(FieldDesc *pFieldDescList, } } - // Set the field offset - DWORD rva; - IfFailThrow(pInternalImport->GetFieldRVA(pFD->GetMemberDef(), &rva)); - // The PE should be loaded by now. _ASSERT(GetModule()->GetPEAssembly()->IsLoaded()); @@ -4432,14 +4428,18 @@ VOID MethodTableBuilder::InitializeFieldDescs(FieldDesc *pFieldDescList, // This is a special case for EnC. The RVA field is not actually in the image, but // is instead registered in a dynamic map. We need to set the RVA to a special // value so when the address is looked up, it will be found in the dynamic map. - if (GetModule()->IsEditAndContinueEnabled()) + if (GetModule()->GetDynamicRvaField(pFD->GetMemberDef()) != NULL) { - if (GetModule()->GetDynamicRvaField(pFD->GetMemberDef()) != NULL) - rva = FIELD_OFFSET_DYNAMIC_RVA; + pFD->SetDynamicRVA(); } + else #endif // FEATURE_METADATA_UPDATER - - pFD->SetOffsetRVA(rva); + { + // Set the field offset + DWORD rva; + IfFailThrow(pInternalImport->GetFieldRVA(pFD->GetMemberDef(), &rva)); + pFD->SetOffsetRVA(rva); + } } else if (fIsThreadStatic) { From a85769339761e0f01c086bd42dacb3d9e2acf8b8 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 11 Apr 2025 17:26:01 -0700 Subject: [PATCH 6/7] Fix linux build --- src/coreclr/vm/methodtablebuilder.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 65fb54e76b993c..b4d3c742debd92 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -4428,7 +4428,7 @@ VOID MethodTableBuilder::InitializeFieldDescs(FieldDesc *pFieldDescList, // This is a special case for EnC. The RVA field is not actually in the image, but // is instead registered in a dynamic map. We need to set the RVA to a special // value so when the address is looked up, it will be found in the dynamic map. - if (GetModule()->GetDynamicRvaField(pFD->GetMemberDef()) != NULL) + if (GetModule()->GetDynamicRvaField(pFD->GetMemberDef()) != (TADDR)NULL) { pFD->SetDynamicRVA(); } From 4859e0fd06ea4468417405aa0b43d99b7b0336c0 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Mon, 14 Apr 2025 11:31:03 -0700 Subject: [PATCH 7/7] Add testing. --- .../AddFieldRVA.cs | 36 ++++++++++++++++ .../AddFieldRVA_v1.cs | 39 ++++++++++++++++++ ...tadata.ApplyUpdate.Test.AddFieldRVA.csproj | 11 +++++ .../deltascript.json | 6 +++ .../tests/ApplyUpdateTest.cs | 41 ++++++++++++++++++- .../tests/ApplyUpdateUtil.cs | 2 + .../tests/System.Runtime.Loader.Tests.csproj | 1 + 7 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/AddFieldRVA.cs create mode 100644 src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/AddFieldRVA_v1.cs create mode 100644 src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA.csproj create mode 100644 src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/deltascript.json diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/AddFieldRVA.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/AddFieldRVA.cs new file mode 100644 index 00000000000000..97b8835efd9654 --- /dev/null +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/AddFieldRVA.cs @@ -0,0 +1,36 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +using System; + +namespace System.Reflection.Metadata.ApplyUpdate.Test +{ + public static class AddFieldRVA + { + public static int LocalReadOnlySpan() + { + return -1; + } + + class InnerClass + { + public int MemberFieldArray() + { + return -1; + } + } + + public static int MemberFieldArray() + { + return new InnerClass().MemberFieldArray(); + } + + public static int LocalStackAllocReadOnlySpan() + { + return -1; + } + + public static ReadOnlySpan Utf8LiteralReadOnlySpan() => "0"u8; + + public static string StringLiteral() => "0"; + } +} diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/AddFieldRVA_v1.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/AddFieldRVA_v1.cs new file mode 100644 index 00000000000000..3de778525e18f6 --- /dev/null +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/AddFieldRVA_v1.cs @@ -0,0 +1,39 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +using System; + +namespace System.Reflection.Metadata.ApplyUpdate.Test +{ + public static class AddFieldRVA + { + public static int LocalReadOnlySpan() + { + ReadOnlySpan s = [1, 2, 3]; + return s.Length; + } + + class InnerClass + { + byte[] _byteArray = { 1, 2, 3, 4 }; + public int MemberFieldArray() + { + return _byteArray.Length; + } + } + + public static int MemberFieldArray() + { + return new InnerClass().MemberFieldArray(); + } + + // public static int LocalStackAllocReadOnlySpan() + // { + // ReadOnlySpan s = stackalloc byte[] { 1, 2, 3, 4, 5 }; + // return s.Length; + // } + + public static ReadOnlySpan Utf8LiteralReadOnlySpan() => "123456"u8; + + public static string StringLiteral() => "1234567"; + } +} diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA.csproj b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA.csproj new file mode 100644 index 00000000000000..311e06e6701cdc --- /dev/null +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA.csproj @@ -0,0 +1,11 @@ + + + System.Runtime.Loader.Tests + $(NetCoreAppCurrent) + true + deltascript.json + + + + + diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/deltascript.json b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/deltascript.json new file mode 100644 index 00000000000000..d41f2e1ea4ece0 --- /dev/null +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddFieldRVA/deltascript.json @@ -0,0 +1,6 @@ +{ + "changes": [ + {"document": "AddFieldRVA.cs", "update": "AddFieldRVA_v1.cs"}, + ] +} + diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs index 2c1a3fc887b446..c5beba1c8ea775 100644 --- a/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs @@ -323,6 +323,45 @@ public static void TestAddStaticField() }); } + // FieldRVA update tests are not supported on the Mono runtime. + //[ConditionalFact(typeof(ApplyUpdateUtil), nameof (ApplyUpdateUtil.IsSupported), nameof (ApplyUpdateUtil.IsNotMonoRuntime))] + //[ActiveIssue("https://github.com/dotnet/runtime/issues/54617", typeof(PlatformDetection), nameof(PlatformDetection.IsBrowser), nameof(PlatformDetection.IsMonoAOT))] + void TestAddFieldRVA() + { + ApplyUpdateUtil.TestCase(static () => + { + var assm = typeof (ApplyUpdate.Test.AddFieldRVA).Assembly; + + { + var lrosLen = ApplyUpdate.Test.AddFieldRVA.LocalReadOnlySpan(); + Assert.Equal(-1, lrosLen); + var mfaLen = ApplyUpdate.Test.AddFieldRVA.MemberFieldArray(); + Assert.Equal(-1, mfaLen); + // var lsarosLen = ApplyUpdate.Test.AddFieldRVA.LocalStackAllocReadOnlySpan(); + // Assert.Equal(-1, mfaLen); + var utf8ros = ApplyUpdate.Test.AddFieldRVA.Utf8LiteralReadOnlySpan(); + Assert.Equal(1, utf8ros.Length); + var strlit = ApplyUpdate.Test.AddFieldRVA.StringLiteral(); + Assert.Equal("0", strlit); + } + + ApplyUpdateUtil.ApplyUpdate(assm); + + { + var lrosLen = ApplyUpdate.Test.AddFieldRVA.LocalReadOnlySpan(); + Assert.Equal(3, lrosLen); + var mfaLen = ApplyUpdate.Test.AddFieldRVA.MemberFieldArray(); + Assert.Equal(4, mfaLen); + // var lsarosLen = ApplyUpdate.Test.AddFieldRVA.LocalStackAllocReadOnlySpan(); + // Assert.Equal(5, mfaLen); + var utf8ros = ApplyUpdate.Test.AddFieldRVA.Utf8LiteralReadOnlySpan(); + Assert.Equal(6, utf8ros.Length); + var strlit = ApplyUpdate.Test.AddFieldRVA.StringLiteral(); + Assert.Equal("1234567", strlit); + } + }); + } + [ConditionalFact(typeof(ApplyUpdateUtil), nameof(ApplyUpdateUtil.IsSupported))] public static void TestAddInstanceField() { @@ -1002,5 +1041,5 @@ void TestIncreaseMetadataRowSize() Assert.Equal("x800", pars[0].Name); }); } - } + } } diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdateUtil.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdateUtil.cs index 0f0ea993c9fa10..b74144dceffd59 100644 --- a/src/libraries/System.Runtime.Loader/tests/ApplyUpdateUtil.cs +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdateUtil.cs @@ -38,6 +38,8 @@ public class ApplyUpdateUtil { public static bool IsMonoRuntime => PlatformDetection.IsMonoRuntime; + public static bool IsNotMonoRuntime => !PlatformDetection.IsMonoRuntime; + private static readonly Lazy s_isSupportedMonoConfiguration = new Lazy(CheckSupportedMonoConfiguration); public static bool IsSupportedMonoConfiguration => s_isSupportedMonoConfiguration.Value; diff --git a/src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj b/src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj index f26917f6f2c8b6..ca7f64f107fffd 100644 --- a/src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj +++ b/src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj @@ -57,6 +57,7 @@ +