Use native code slot for default interface methods (dotnet/coreclr#25770)

- So that `MethodDesc::GetNativeCode()` can retrieve the current native code entry point (instead of returning null as before), and code versioning can find a matching code version from the code start address
- Interface methods currently require having a precode, so the "method entry point" can't be used to directly store the native code entry point
- Reenabled a couple of default interface method tests under GCStress
- Other small cleanup

Fixes https://github.com/dotnet/coreclr/issues/25690

Commit migrated from https://github.com/dotnet/coreclr/commit/98bf56a2cf19645563b41ab01f6972cbe83c9d44

diff --git a/src/coreclr/src/debug/ee/functioninfo.cpp b/src/coreclr/src/debug/ee/functioninfo.cpp
index 9b41d5c..214dee1 100644 (file)
--- a/src/coreclr/src/debug/ee/functioninfo.cpp
+++ b/src/coreclr/src/debug/ee/functioninfo.cpp
@@ -1603,13 +1603,20 @@ DebuggerJitInfo *DebuggerMethodInfo::FindOrCreateInitAndAddJitInfo(MethodDesc* f
     // The DJI may already be populated in the cache, if so CreateInitAndAddJitInfo is a no-op and that is fine.
     // CreateInitAndAddJitInfo takes a lock and checks the list again, which makes this thread-safe.
 
-    CodeVersionManager::TableLockHolder lockHolder(fd->GetCodeVersionManager());
-    CodeVersionManager *pCodeVersionManager = fd->GetCodeVersionManager();
-    NativeCodeVersion nativeCodeVersion = pCodeVersionManager->GetNativeCodeVersion(fd, startAddr);
-
-    // Some day we'll get EnC to use code versioning properly, but until then we'll get the right behavior treating all EnC versions as the default native code version.
-    if (nativeCodeVersion.IsNull())
+    NativeCodeVersion nativeCodeVersion;
+    if (fd->IsVersionable())
+    {
+        CodeVersionManager::TableLockHolder lockHolder(fd->GetCodeVersionManager());
+        CodeVersionManager *pCodeVersionManager = fd->GetCodeVersionManager();
+        nativeCodeVersion = pCodeVersionManager->GetNativeCodeVersion(fd, startAddr);
+        if (nativeCodeVersion.IsNull())
+        {
+            return NULL;
+        }
+    }
+    else
     {
+        // Some day we'll get EnC to use code versioning properly, but until then we'll get the right behavior treating all EnC versions as the default native code version.
         nativeCodeVersion = NativeCodeVersion(fd);
     }
 

diff --git a/src/coreclr/src/vm/codeversion.cpp b/src/coreclr/src/vm/codeversion.cpp
index eb5f15f..e956ad7 100644 (file)
--- a/src/coreclr/src/vm/codeversion.cpp
+++ b/src/coreclr/src/vm/codeversion.cpp
@@ -200,7 +200,7 @@ void NativeCodeVersionNode::SetGCCoverageInfo(PTR_GCCoverageInfo gcCover)
 #endif // HAVE_GCCOVER
 
 NativeCodeVersion::NativeCodeVersion() :
-    m_storageKind(StorageKind::Unknown)
+    m_storageKind(StorageKind::Unknown), m_pVersionNode(PTR_NULL)
 {}
 
 NativeCodeVersion::NativeCodeVersion(const NativeCodeVersion & rhs) :

diff --git a/src/coreclr/src/vm/eventtrace.cpp b/src/coreclr/src/vm/eventtrace.cpp
index 40c007c..05febe7 100644 (file)
--- a/src/coreclr/src/vm/eventtrace.cpp
+++ b/src/coreclr/src/vm/eventtrace.cpp
@@ -6901,7 +6901,7 @@ VOID ETW::MethodLog::SendEventsForJitMethodsHelper(LoaderAllocator *pLoaderAlloc
         ReJITID ilCodeId = 0;
         NativeCodeVersion nativeCodeVersion;
 #ifdef FEATURE_CODE_VERSIONING
-        if (fGetCodeIds)
+        if (fGetCodeIds && pMD->IsVersionable())
         {
             CodeVersionManager *pCodeVersionManager = pMD->GetCodeVersionManager();
             _ASSERTE(pCodeVersionManager->LockOwnedByCurrentThread());

diff --git a/src/coreclr/src/vm/gccover.cpp b/src/coreclr/src/vm/gccover.cpp
index 2ede08d..a455c3d 100644 (file)
--- a/src/coreclr/src/vm/gccover.cpp
+++ b/src/coreclr/src/vm/gccover.cpp
@@ -1315,7 +1315,9 @@ bool IsGcCoverageInterrupt(LPVOID ip)
         return false;
     }
 
-    GCCoverageInfo *gcCover = codeInfo.GetNativeCodeVersion().GetGCCoverageInfo();
+    NativeCodeVersion nativeCodeVersion = codeInfo.GetNativeCodeVersion();
+    _ASSERTE(!nativeCodeVersion.IsNull());
+    GCCoverageInfo *gcCover = nativeCodeVersion.GetGCCoverageInfo();
     if (gcCover == nullptr)
     {
         return false;
@@ -1377,7 +1379,9 @@ BOOL OnGcCoverageInterrupt(PCONTEXT regs)
     forceStack[1] = &pMD;                // This is so I can see it fastchecked
     forceStack[2] = &offset;             // This is so I can see it fastchecked
 
-    GCCoverageInfo* gcCover = codeInfo.GetNativeCodeVersion().GetGCCoverageInfo();
+    NativeCodeVersion nativeCodeVersion = codeInfo.GetNativeCodeVersion();
+    _ASSERTE(!nativeCodeVersion.IsNull());
+    GCCoverageInfo* gcCover = nativeCodeVersion.GetGCCoverageInfo();
     forceStack[3] = &gcCover;            // This is so I can see it fastchecked
     if (gcCover == 0)
         return(FALSE);        // we aren't doing code gcCoverage on this function

diff --git a/src/coreclr/src/vm/jitinterface.cpp b/src/coreclr/src/vm/jitinterface.cpp
index d9a02fd..38f3731 100644 (file)
--- a/src/coreclr/src/vm/jitinterface.cpp
+++ b/src/coreclr/src/vm/jitinterface.cpp
@@ -14283,12 +14283,14 @@ NativeCodeVersion EECodeInfo::GetNativeCodeVersion()
     }
 
 #ifdef FEATURE_CODE_VERSIONING
-    CodeVersionManager *pCodeVersionManager = pMD->GetCodeVersionManager();
-    CodeVersionManager::TableLockHolder lockHolder(pCodeVersionManager);
-    return pCodeVersionManager->GetNativeCodeVersion(pMD, PINSTRToPCODE(GetStartAddress()));
-#else
-    return NativeCodeVersion(pMD);
+    if (pMD->IsVersionable())
+    {
+        CodeVersionManager *pCodeVersionManager = pMD->GetCodeVersionManager();
+        CodeVersionManager::TableLockHolder lockHolder(pCodeVersionManager);
+        return pCodeVersionManager->GetNativeCodeVersion(pMD, PINSTRToPCODE(GetStartAddress()));
+    }
 #endif
+    return NativeCodeVersion(pMD);
 }
 
 #if defined(WIN64EXCEPTIONS)

diff --git a/src/coreclr/src/vm/method.cpp b/src/coreclr/src/vm/method.cpp
index 67bf593..5de254e 100644 (file)
--- a/src/coreclr/src/vm/method.cpp
+++ b/src/coreclr/src/vm/method.cpp
@@ -1022,6 +1022,7 @@ PCODE MethodDesc::GetNativeCode()
 {
     WRAPPER_NO_CONTRACT;
     SUPPORTS_DAC;
+    _ASSERTE(!IsDefaultInterfaceMethod() || HasNativeCodeSlot());
 
     g_IBCLogger.LogMethodDescAccess(this);
 
@@ -2528,7 +2529,7 @@ BOOL MethodDesc::MayHaveNativeCode()
 
     _ASSERTE(IsIL());
 
-    if ((IsInterface() && !IsStatic() && IsVirtual() && IsAbstract()) || IsWrapperStub() || ContainsGenericVariables() || IsAbstract())
+    if (IsWrapperStub() || ContainsGenericVariables() || IsAbstract())
     {
         return FALSE;
     }
@@ -5037,6 +5038,8 @@ BOOL MethodDesc::SetNativeCodeInterlocked(PCODE addr, PCODE pExpected /*=NULL*/)
         GC_NOTRIGGER;
     } CONTRACTL_END;
 
+    _ASSERTE(!IsDefaultInterfaceMethod() || HasNativeCodeSlot());
+
     if (HasNativeCodeSlot())
     {
 #ifdef _TARGET_ARM_
@@ -5060,11 +5063,6 @@ BOOL MethodDesc::SetNativeCodeInterlocked(PCODE addr, PCODE pExpected /*=NULL*/)
             (TADDR&)value, (TADDR&)expected) == (TADDR&)expected;
     }
     
-    if (IsDefaultInterfaceMethod() && HasPrecode())
-    {        
-        return GetPrecode()->SetTargetInterlocked(addr);
-    }
-
     _ASSERTE(pExpected == NULL);
     return SetStableEntryPointInterlocked(addr);
 }

diff --git a/src/coreclr/src/vm/methodtablebuilder.cpp b/src/coreclr/src/vm/methodtablebuilder.cpp
index 5cadcd4..d34f068 100644 (file)
--- a/src/coreclr/src/vm/methodtablebuilder.cpp
+++ b/src/coreclr/src/vm/methodtablebuilder.cpp
@@ -6973,6 +6973,20 @@ MethodTableBuilder::NeedsNativeCodeSlot(bmtMDMethod * pMDMethod)
     }
 #endif
 
+#ifdef FEATURE_DEFAULT_INTERFACES
+    if (IsInterface())
+    {
+        DWORD attrs = pMDMethod->GetDeclAttrs();
+        if (!IsMdStatic(attrs) && IsMdVirtual(attrs) && !IsMdAbstract(attrs))
+        {
+            // Default interface method. Since interface methods currently need to have a precode, the native code slot will be
+            // used to retrieve the native code entry point, instead of getting it from the precode, which is not reliable with
+            // debuggers setting breakpoints.
+            return TRUE;
+        }
+    }
+#endif
+
 #if defined(FEATURE_JIT_PITCHING)
     if ((CLRConfig::GetConfigValue(CLRConfig::INTERNAL_JitPitchEnabled) != 0) &&
         (CLRConfig::GetConfigValue(CLRConfig::INTERNAL_JitPitchMemThreshold) != 0))

diff --git a/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_d.ilproj b/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_d.ilproj
index 330f5d1..1018c78 100644 (file)
--- a/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_d.ilproj
+++ b/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_d.ilproj
@@ -5,8 +5,6 @@
     <OutputType>Exe</OutputType>
     <CLRTestKind>BuildAndRun</CLRTestKind>
     <CLRTestPriority>0</CLRTestPriority>
-    <!-- See https://github.com/dotnet/coreclr/issues/25690 -->
-    <GCStressIncompatible>true</GCStressIncompatible>
   </PropertyGroup>
   <PropertyGroup>
     <DebugType>Full</DebugType>

diff --git a/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_r.ilproj b/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_r.ilproj
index b8c4e3d..44208ba 100644 (file)
--- a/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_r.ilproj
+++ b/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_r.ilproj
@@ -5,8 +5,6 @@
     <OutputType>Exe</OutputType>
     <CLRTestKind>BuildAndRun</CLRTestKind>
     <CLRTestPriority>0</CLRTestPriority>
-    <!-- See https://github.com/dotnet/coreclr/issues/25690 -->
-    <GCStressIncompatible>true</GCStressIncompatible>
   </PropertyGroup>
   <PropertyGroup>
     <DebugType />