Allow ILCodeVersion to fallback to default IL (#18502)

* Allow ILCodeVersion to fallback to default IL

For compat with profilers that used our APIs in unexpected ways we can allow
the ILCodeVersion to fallback to the default IL code when no IL was explicitly
given.

* Fix incorrect usage of ILCodeVersion::AsNode (issue #18602)

When the debugger is querying the active rejit IL for an IL method that has not been rejitted it incorrectly creates a VMPTR_ILCodeVersionNode for a code version that shouldn't have one.

diff --git a/src/debug/daccess/dacdbiimpl.cpp b/src/debug/daccess/dacdbiimpl.cpp
index 4f15297..faaa7ec 100644 (file)
--- a/src/debug/daccess/dacdbiimpl.cpp
+++ b/src/debug/daccess/dacdbiimpl.cpp
@@ -7169,7 +7169,7 @@ HRESULT DacDbiInterfaceImpl::GetActiveRejitILCodeVersionNode(VMPTR_Module vmModu
     // manager's active IL version hasn't yet asked the profiler for the IL body to use, in which case we want to filter it
     //  out from the return in this method.
     ILCodeVersion activeILVersion = pCodeVersionManager->GetActiveILCodeVersion(pModule, methodTk);
-    if (activeILVersion.IsNull() || activeILVersion.GetRejitState() != ILCodeVersion::kStateActive)
+    if (activeILVersion.IsNull() || activeILVersion.IsDefaultVersion() || activeILVersion.GetRejitState() != ILCodeVersion::kStateActive)
     {
         pVmILCodeVersionNode->SetDacTargetPtr(0);
     }
@@ -7249,11 +7249,11 @@ HRESULT DacDbiInterfaceImpl::GetILCodeVersionNodeData(VMPTR_ILCodeVersionNode vm
 {
     DD_ENTER_MAY_THROW;
 #ifdef FEATURE_REJIT
-    ILCodeVersionNode* pILCodeVersionNode = vmILCodeVersionNode.GetDacPtr();
-    pData->m_state = pILCodeVersionNode->GetRejitState();
-    pData->m_pbIL = PTR_TO_CORDB_ADDRESS(dac_cast<ULONG_PTR>(pILCodeVersionNode->GetIL()));
-    pData->m_dwCodegenFlags = pILCodeVersionNode->GetJitFlags();
-    const InstrumentedILOffsetMapping* pMapping = pILCodeVersionNode->GetInstrumentedILMap();
+    ILCodeVersion ilCode(vmILCodeVersionNode.GetDacPtr());
+    pData->m_state = ilCode.GetRejitState();
+    pData->m_pbIL = PTR_TO_CORDB_ADDRESS(dac_cast<ULONG_PTR>(ilCode.GetIL()));
+    pData->m_dwCodegenFlags = ilCode.GetJitFlags();
+    const InstrumentedILOffsetMapping* pMapping = ilCode.GetInstrumentedILMap();
     if (pMapping)
     {
         pData->m_cInstrumentedMapEntries = (ULONG)pMapping->GetCount();

diff --git a/src/vm/codeversion.cpp b/src/vm/codeversion.cpp
index 01aad66..8c615e4 100644 (file)
--- a/src/vm/codeversion.cpp
+++ b/src/vm/codeversion.cpp
@@ -773,23 +773,38 @@ PTR_COR_ILMETHOD ILCodeVersion::GetIL() const
     }
     CONTRACTL_END
 
+    PTR_COR_ILMETHOD pIL = NULL;
     if (m_storageKind == StorageKind::Explicit)
     {
-        return AsNode()->GetIL();
+        pIL = AsNode()->GetIL();
     }
-    else
+    
+    // For the default code version we always fetch the globally stored default IL for a method
+    //
+    // In the non-default code version we assume NULL is the equivalent of explicitly requesting to
+    // re-use the default IL. Ideally there would be no reason to create a new version that re-uses
+    // the default IL (just use the default code version for that) but we do it here for compat. We've 
+    // got some profilers that use ReJIT to create a new code version and then instead of calling
+    // ICorProfilerFunctionControl::SetILFunctionBody they call ICorProfilerInfo::SetILFunctionBody. 
+    // This mutates the default IL so that it is now correct for their new code version. Of course this
+    // also overwrote the previous default IL so now the default code version GetIL() is out of sync
+    // with the jitted code. In the majority of cases we never re-read the IL after the initial
+    // jitting so this issue goes unnoticed.
+    //
+    // If changing the default IL after it is in use becomes more problematic in the future we would
+    // need to add enforcement that prevents profilers from using ICorProfilerInfo::SetILFunctionBody
+    // that way + coordinate with them because it is a breaking change for any profiler currently doing it.
+    if(pIL == NULL)
     {
         PTR_Module pModule = GetModule();
         PTR_MethodDesc pMethodDesc = dac_cast<PTR_MethodDesc>(pModule->LookupMethodDef(GetMethodDef()));
-        if (pMethodDesc == NULL)
+        if (pMethodDesc != NULL)
         {
-            return NULL;
-        }
-        else
-        {
-            return dac_cast<PTR_COR_ILMETHOD>(pMethodDesc->GetILHeader(TRUE));
+            pIL = dac_cast<PTR_COR_ILMETHOD>(pMethodDesc->GetILHeader(TRUE));
         }
     }
+
+    return pIL;
 }
 
 PTR_COR_ILMETHOD ILCodeVersion::GetILNoThrow() const
@@ -925,6 +940,9 @@ HRESULT ILCodeVersion::SetActiveNativeCodeVersion(NativeCodeVersion activeNative
 ILCodeVersionNode* ILCodeVersion::AsNode()
 {
     LIMITED_METHOD_CONTRACT;
+    //This is dangerous - NativeCodeVersion coerces non-explicit versions to NULL but ILCodeVersion assumes the caller
+    //will never invoke AsNode() on a non-explicit node. Asserting for now as a minimal fix, but we should revisit this.
+    _ASSERTE(m_storageKind == StorageKind::Explicit);
     return m_pVersionNode;
 }
 #endif //DACCESS_COMPILE
@@ -932,6 +950,9 @@ ILCodeVersionNode* ILCodeVersion::AsNode()
 PTR_ILCodeVersionNode ILCodeVersion::AsNode() const
 {
     LIMITED_METHOD_DAC_CONTRACT;
+    //This is dangerous - NativeCodeVersion coerces non-explicit versions to NULL but ILCodeVersion assumes the caller
+    //will never invoke AsNode() on a non-explicit node. Asserting for now as a minimal fix, but we should revisit this.
+    _ASSERTE(m_storageKind == StorageKind::Explicit);
     return m_pVersionNode;
 }