From 2b79f1c809d45186761aec66c99fa21833e121f5 Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Wed, 24 Feb 2021 13:09:17 -0800 Subject: [PATCH 1/3] Add EXTERNAL_DOTNET_MODIFIABLE_ASSEMBLIES env var for hot reload support --- src/coreclr/inc/clrconfigvalues.h | 1 + src/coreclr/vm/ceeload.cpp | 13 ++++++++----- src/coreclr/vm/eeconfig.cpp | 7 +++++++ src/coreclr/vm/eeconfig.h | 2 ++ 4 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/coreclr/inc/clrconfigvalues.h b/src/coreclr/inc/clrconfigvalues.h index 74f5ac2815728f..5ed4f5dcb51ae3 100644 --- a/src/coreclr/inc/clrconfigvalues.h +++ b/src/coreclr/inc/clrconfigvalues.h @@ -450,6 +450,7 @@ CONFIG_DWORD_INFO_EX(INTERNAL_MD_MiniMDBreak, W("MD_MiniMDBreak"), 0, "ASSERT wh CONFIG_DWORD_INFO_EX(INTERNAL_MD_PreSaveBreak, W("MD_PreSaveBreak"), 0, "ASSERT when calling CMiniMdRw::PreSave", CLRConfig::EEConfig_default) CONFIG_DWORD_INFO_EX(INTERNAL_MD_RegMetaBreak, W("MD_RegMetaBreak"), 0, "ASSERT when creating RegMeta class", CLRConfig::EEConfig_default) CONFIG_DWORD_INFO_EX(INTERNAL_MD_RegMetaDump, W("MD_RegMetaDump"), 0, "Dump MD in 4 functions (?)", CLRConfig::EEConfig_default) +RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_DOTNET_MODIFIABLE_ASSEMBLIES, W("DOTNET_MODIFIABLE_ASSEMBLIES"), "Enables hot reload on debug built assemblies with the 'debug' keyword", CLRConfig::DontPrependCOMPlus_ | CLRConfig::TrimWhiteSpaceFromStringValue); // Metadata - mscordbi only - this flag is only intended to mitigate potential issues in bug fix 458597. RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_MD_PreserveDebuggerMetadataMemory, W("MD_PreserveDebuggerMetadataMemory"), 0, "Save all versions of metadata memory in the debugger when debuggee metadata is updated", CLRConfig::EEConfig_default) diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index c17f64e9ff4562..a286d4116f7065 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -686,10 +686,13 @@ void Module::Initialize(AllocMemTracker *pamTracker, LPCWSTR szName) Module::CreateAssemblyRefByNameTable(pamTracker); } - // If the program has the "ForceEnc" env variable set we ensure every eligible - // module has EnC turned on. - if (g_pConfig->ForceEnc() && IsEditAndContinueCapable()) - EnableEditAndContinue(); + if (IsEditAndContinueCapable()) + { + // If the program has the "ForceEnc" env variable set we ensure every eligible module has EnC turned on + // or if the debug built modifiable assemblies are enabled and the module is a debug built module. + if (g_pConfig->ForceEnc() || (g_pConfig->DebugAssembliesModifiable() && CORDisableJITOptimizations(GetDebuggerInfoBits()))) + EnableEditAndContinue(); + } #if defined(PROFILING_SUPPORTED) && !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE) m_pJitInlinerTrackingMap = NULL; @@ -1087,7 +1090,7 @@ void Module::SetDebuggerInfoBits(DebuggerAssemblyControlFlags newBits) } else { - if (!g_pConfig->ForceEnc()) + if (!(g_pConfig->ForceEnc() || (g_pConfig->DebugAssembliesModifiable() && CORDisableJITOptimizations(GetDebuggerInfoBits())))) DisableEditAndContinue(); } #endif // DEBUGGING_SUPPORTED diff --git a/src/coreclr/vm/eeconfig.cpp b/src/coreclr/vm/eeconfig.cpp index dd89bf56e7767a..6d5ea5a3d267cf 100644 --- a/src/coreclr/vm/eeconfig.cpp +++ b/src/coreclr/vm/eeconfig.cpp @@ -584,6 +584,13 @@ fTrackDynamicMethodDebugInfo = CLRConfig::GetConfigValue(CLRConfig::UNSUPPORTED_ fStressLog = GetConfigDWORD_DontUse_(CLRConfig::UNSUPPORTED_StressLog, fStressLog) != 0; fForceEnc = GetConfigDWORD_DontUse_(CLRConfig::UNSUPPORTED_ForceEnc, fForceEnc) != 0; + { + NewArrayHolder wszModifiableAssemblies; + IfFailRet(CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_MODIFIABLE_ASSEMBLIES, &wszModifiableAssemblies)); + if (wszModifiableAssemblies) + fDebugAssembliesModifiable = _wcsicmp(wszModifiableAssemblies, W("debug")) == 0; + } + iRequireZaps = RequireZapsType(GetConfigDWORD_DontUse_(CLRConfig::EXTERNAL_ZapRequire, iRequireZaps)); if (IsCompilationProcess() || iRequireZaps >= REQUIRE_ZAPS_COUNT) iRequireZaps = REQUIRE_ZAPS_NONE; diff --git a/src/coreclr/vm/eeconfig.h b/src/coreclr/vm/eeconfig.h index 5c5babfd094299..11f1d286d9f92a 100644 --- a/src/coreclr/vm/eeconfig.h +++ b/src/coreclr/vm/eeconfig.h @@ -463,6 +463,7 @@ class EEConfig bool StressLog() const { LIMITED_METHOD_CONTRACT; return fStressLog; } bool ForceEnc() const { LIMITED_METHOD_CONTRACT; return fForceEnc; } + bool DebugAssembliesModifiable() const { LIMITED_METHOD_CONTRACT; return fDebugAssembliesModifiable; } // Optimizations to improve working set @@ -691,6 +692,7 @@ class EEConfig bool fStressLog; bool fForceEnc; + bool fDebugAssembliesModifiable; bool fProbeForStackOverflow; // Stackwalk optimization flag From 94a0a48e76bc0f0caca558b15fc8be116db2679a Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Wed, 24 Feb 2021 18:54:55 -0800 Subject: [PATCH 2/3] Code review feedback --- src/coreclr/vm/ceeload.cpp | 24 ++++++------------------ src/coreclr/vm/ceeload.h | 9 --------- 2 files changed, 6 insertions(+), 27 deletions(-) diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index a286d4116f7065..e121270dd72491 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -686,14 +686,6 @@ void Module::Initialize(AllocMemTracker *pamTracker, LPCWSTR szName) Module::CreateAssemblyRefByNameTable(pamTracker); } - if (IsEditAndContinueCapable()) - { - // If the program has the "ForceEnc" env variable set we ensure every eligible module has EnC turned on - // or if the debug built modifiable assemblies are enabled and the module is a debug built module. - if (g_pConfig->ForceEnc() || (g_pConfig->DebugAssembliesModifiable() && CORDisableJITOptimizations(GetDebuggerInfoBits()))) - EnableEditAndContinue(); - } - #if defined(PROFILING_SUPPORTED) && !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE) m_pJitInlinerTrackingMap = NULL; if (ReJitManager::IsReJITInlineTrackingEnabled()) @@ -1081,17 +1073,13 @@ void Module::SetDebuggerInfoBits(DebuggerAssemblyControlFlags newBits) m_dwTransientFlags |= (newBits << DEBUGGER_INFO_SHIFT_PRIV); #ifdef DEBUGGING_SUPPORTED - BOOL setEnC = ((newBits & DACF_ENC_ENABLED) != 0) && IsEditAndContinueCapable(); - - // The only way can change Enc is through debugger override. - if (setEnC) - { - EnableEditAndContinue(); - } - else + if (IsEditAndContinueCapable()) { - if (!(g_pConfig->ForceEnc() || (g_pConfig->DebugAssembliesModifiable() && CORDisableJITOptimizations(GetDebuggerInfoBits())))) - DisableEditAndContinue(); + BOOL setEnC = (newBits & DACF_ENC_ENABLED) != 0 || g_pConfig->ForceEnc() || (g_pConfig->DebugAssembliesModifiable() && CORDisableJITOptimizations(GetDebuggerInfoBits())); + if (setEnC) + { + EnableEditAndContinue(); + } } #endif // DEBUGGING_SUPPORTED diff --git a/src/coreclr/vm/ceeload.h b/src/coreclr/vm/ceeload.h index f87e8431254845..f372392d44b8cc 100644 --- a/src/coreclr/vm/ceeload.h +++ b/src/coreclr/vm/ceeload.h @@ -1806,15 +1806,6 @@ class Module m_dwTransientFlags |= IS_EDIT_AND_CONTINUE; } - void DisableEditAndContinue() - { - LIMITED_METHOD_CONTRACT; - SUPPORTS_DAC; - // don't _ASSERTE(IsEditAndContinueCapable()); - LOG((LF_ENC, LL_INFO100, "DisableEditAndContinue: this:0x%x, %s\n", this, GetDebugName())); - m_dwTransientFlags = m_dwTransientFlags.Load() & (~IS_EDIT_AND_CONTINUE); - } - BOOL IsTenured() { LIMITED_METHOD_CONTRACT; From a25d999765cd7c1f2d11db5425ec748e5f475a41 Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Thu, 25 Feb 2021 12:41:43 -0800 Subject: [PATCH 3/3] More code review feedback --- src/coreclr/vm/ceeload.h | 13 ++++--------- src/coreclr/vm/ceeload.inl | 14 -------------- src/coreclr/vm/encee.h | 2 ++ 3 files changed, 6 insertions(+), 23 deletions(-) diff --git a/src/coreclr/vm/ceeload.h b/src/coreclr/vm/ceeload.h index f372392d44b8cc..073b567e44171c 100644 --- a/src/coreclr/vm/ceeload.h +++ b/src/coreclr/vm/ceeload.h @@ -1769,7 +1769,6 @@ class Module return m_moduleRef; } - BOOL IsResource() const { WRAPPER_NO_CONTRACT; SUPPORTS_DAC; return GetFile()->IsResource(); } BOOL IsPEFile() const { WRAPPER_NO_CONTRACT; return !GetFile()->IsDynamic(); } BOOL IsReflection() const { WRAPPER_NO_CONTRACT; SUPPORTS_DAC; return GetFile()->IsDynamic(); } @@ -1777,19 +1776,15 @@ class Module // Returns true iff the debugger can see this module. BOOL IsVisibleToDebugger(); - BOOL IsEditAndContinueEnabled() { LIMITED_METHOD_CONTRACT; SUPPORTS_DAC; - // We are seeing cases where this flag is set for a module that is not an EditAndContinueModule. This should - // never happen unless the module is EditAndContinueCapable, in which case we would have created an EditAndContinueModule - // not a Module. - //_ASSERTE((m_dwTransientFlags & IS_EDIT_AND_CONTINUE) == 0 || IsEditAndContinueCapable()); - return (IsEditAndContinueCapable()) && ((m_dwTransientFlags & IS_EDIT_AND_CONTINUE) != 0); + _ASSERTE((m_dwTransientFlags & IS_EDIT_AND_CONTINUE) == 0 || IsEditAndContinueCapable()); + return (m_dwTransientFlags & IS_EDIT_AND_CONTINUE) != 0; } - BOOL IsEditAndContinueCapable(); + virtual BOOL IsEditAndContinueCapable() const { return FALSE; } BOOL IsIStream() { LIMITED_METHOD_CONTRACT; return GetFile()->IsIStream(); } @@ -1801,7 +1796,7 @@ class Module { LIMITED_METHOD_CONTRACT; SUPPORTS_DAC; - // _ASSERTE(IsEditAndContinueCapable()); + _ASSERTE(IsEditAndContinueCapable()); LOG((LF_ENC, LL_INFO100, "EnableEditAndContinue: this:0x%x, %s\n", this, GetDebugName())); m_dwTransientFlags |= IS_EDIT_AND_CONTINUE; } diff --git a/src/coreclr/vm/ceeload.inl b/src/coreclr/vm/ceeload.inl index a5094b45e61aa5..d6dbe8e7b40ba7 100644 --- a/src/coreclr/vm/ceeload.inl +++ b/src/coreclr/vm/ceeload.inl @@ -453,20 +453,6 @@ inline mdAssemblyRef Module::FindAssemblyRef(Assembly *targetAssembly) #endif //DACCESS_COMPILE -inline BOOL Module::IsEditAndContinueCapable() -{ - WRAPPER_NO_CONTRACT; - SUPPORTS_DAC; - - BOOL isEnCCapable = IsEditAndContinueCapable(m_pAssembly, m_file); - - // for now, Module::IsReflection is equivalent to m_file->IsDynamic, - // which is checked by IsEditAndContinueCapable(m_pAssembly, m_file) - _ASSERTE(!isEnCCapable || (!this->IsReflection())); - - return isEnCCapable; -} - FORCEINLINE PTR_DomainLocalModule Module::GetDomainLocalModule() { WRAPPER_NO_CONTRACT; diff --git a/src/coreclr/vm/encee.h b/src/coreclr/vm/encee.h index 35e6cb91e491b2..0cd6216b50799c 100644 --- a/src/coreclr/vm/encee.h +++ b/src/coreclr/vm/encee.h @@ -224,6 +224,8 @@ class EditAndContinueModule : public Module virtual void Destruct(); #endif + virtual BOOL IsEditAndContinueCapable() const { return TRUE; } + // Apply an EnC edit HRESULT ApplyEditAndContinue(DWORD cbMetadata, BYTE *pMetadata,