Miscellaneous cleanup (#122573)

- Fix comments
- Delete some dead code
- Fix disabled file system test
- Add const
- Avoid redundant virtual calls
- Improve GS cookie
- Use Interlocked.Or/And

Author: jkotas

src/coreclr/debug/ee/controller.cpp

@@ -2039,8 +2039,8 @@ BOOL DebuggerController::AddBindAndActivateILReplicaPatch(DebuggerControllerPatc
         // causing the patch table to grow and move.
         SIZE_T primaryILOffset = primary->offset;
 
-        // Loop through all the native offsets mapped to the given IL offset.  On x86 the mapping
-        // should be 1:1.  On WIN64, because there are funclets, we have a 1:N mapping.
+        // Loop through all the native offsets mapped to the given IL offset.
+        // It is 1:N mapping due to optimizations like funclet inlining or loop cloning.
         DebuggerJitInfo::ILToNativeOffsetIterator it;
         for (dji->InitILToNativeOffsetIterator(it, primaryILOffset); !it.IsAtEnd(); it.Next())
         {
@@ -5294,7 +5294,7 @@ bool DebuggerStepper::IsRangeAppropriate(ControllerStackInfo *info)
     }
 
 #if defined(FEATURE_EH_FUNCLETS)
-    // There are three scenarios which make this function more complicated on WIN64.
+    // There are three scenarios which make this function more complicated with funclets.
     // 1)  We initiate a step in the parent method or a funclet but end up stepping into another funclet closer to the leaf.
     //      a)  start in the parent method
     //      b)  start in a funclet
@@ -6348,7 +6348,7 @@ bool DebuggerStepper::IsAddrWithinFrame(DebuggerJitInfo *dji,
     }
 
 #if defined(FEATURE_EH_FUNCLETS)
-    // On WIN64, we also check whether the targetAddr and the currentAddr is in the same funclet.
+    // Also check whether the targetAddr and the currentAddr is in the same funclet.
     _ASSERTE(currentAddr != NULL);
     if (result)
     {
@@ -6895,7 +6895,7 @@ bool DebuggerStepper::SetRangesFromIL(DebuggerJitInfo *dji, COR_DEBUG_STEP_RANGE
         rToEnd  = rTo + realRangeCount;
 
         // <NOTE>
-        // rTo may also be incremented in the middle of the loop on WIN64 platforms.
+        // rTo may also be incremented in the middle of the loop
         // </NOTE>
         for (/**/; r < rEnd; r++, rTo++)
         {

src/coreclr/debug/ee/rcthread.cpp

@@ -134,34 +134,6 @@ HANDLE CreateWin32EventOrThrow(
     RETURN h;
 }
 
-//-----------------------------------------------------------------------------
-// Open an event. Another helper for DebuggerRCThread::Init
-//-----------------------------------------------------------------------------
-HANDLE OpenWin32EventOrThrow(
-    DWORD dwDesiredAccess,
-    BOOL bInheritHandle,
-    LPCWSTR lpName
-)
-{
-    CONTRACT(HANDLE)
-    {
-        THROWS;
-        GC_NOTRIGGER;
-        POSTCONDITION(RETVAL != NULL);
-    }
-    CONTRACT_END;
-
-    HANDLE h = OpenEvent(
-        dwDesiredAccess,
-        bInheritHandle,
-        lpName
-    );
-    if (h == NULL)
-        ThrowLastError();
-
-    RETURN h;
-}
-
 //---------------------------------------------------------------------------------------
 //
 // Init

src/coreclr/nativeaot/Runtime/startup.cpp

@@ -25,6 +25,7 @@
 #include "yieldprocessornormalized.h"
 #include <minipal/cpufeatures.h>
 #include <minipal/time.h>
+#include <minipal/random.h>
 
 #ifdef FEATURE_PERFTRACING
 #include "EventPipeInterface.h"
@@ -224,8 +225,9 @@ bool InitGSCookie()
     }
 #endif
 
-    // REVIEW: Need something better for PAL...
-    GSCookie val = (GSCookie)minipal_lowres_ticks();
+    GSCookie val;
+
+    minipal_get_non_cryptographically_secure_random_bytes((uint8_t*)&val, sizeof(val));
 
 #ifdef _DEBUG
     // In _DEBUG, always use the same value to make it easier to search for the cookie

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Thread.NativeAot.cs

@@ -272,24 +272,12 @@ private bool GetThreadStateBit(ThreadState bit)
 
         private int SetThreadStateBit(ThreadState bit)
         {
-            int oldState, newState;
-            do
-            {
-                oldState = _threadState;
-                newState = oldState | (int)bit;
-            } while (Interlocked.CompareExchange(ref _threadState, newState, oldState) != oldState);
-            return oldState;
+            return Interlocked.Or(ref _threadState, (int)bit);
         }
 
         private int ClearThreadStateBit(ThreadState bit)
         {
-            int oldState, newState;
-            do
-            {
-                oldState = _threadState;
-                newState = oldState & ~(int)bit;
-            } while (Interlocked.CompareExchange(ref _threadState, newState, oldState) != oldState);
-            return oldState;
+            return Interlocked.And(ref _threadState, ~(int)bit);
         }
 
         internal void SetWaitSleepJoinState()

src/coreclr/pal/inc/pal.h

@@ -266,27 +266,6 @@ PAL_GenerateCoreDump(
     LPSTR errorMessageBuffer,
     INT cbErrorMessageBuffer);
 
-typedef VOID (*PPAL_STARTUP_CALLBACK)(
-    char *modulePath,
-    HMODULE hModule,
-    PVOID parameter);
-
-PALIMPORT
-DWORD
-PALAPI
-PAL_RegisterForRuntimeStartup(
-    IN DWORD dwProcessId,
-    IN LPCWSTR lpApplicationGroupId,
-    IN PPAL_STARTUP_CALLBACK pfnCallback,
-    IN PVOID parameter,
-    OUT PVOID *ppUnregisterToken);
-
-PALIMPORT
-DWORD
-PALAPI
-PAL_UnregisterForRuntimeStartup(
-    IN PVOID pUnregisterToken);
-
 PALIMPORT
 BOOL
 PALAPI

src/coreclr/runtime/CachedInterfaceDispatch.cpp

@@ -129,13 +129,13 @@ static bool UpdateCacheEntryAtomically(InterfaceDispatchCacheEntry *pEntry,
 // supplied (in the case where another thread raced with us for the update and won). In any case, if the
 // returned pointer is non-NULL it represents a cache that should be scheduled for release.
 static InterfaceDispatchCache * UpdateCellStubAndCache(InterfaceDispatchCell * pCell,
-                                                       void * pStub,
+                                                       PCODE pStub,
                                                        uintptr_t newCacheValue)
 {
     static_assert(offsetof(InterfaceDispatchCell, m_pStub) == 0);
     static_assert(offsetof(InterfaceDispatchCell, m_pCache) == sizeof(void*));
 
-    uintptr_t oldCacheValue = (uintptr_t)UpdatePointerPairAtomically(pCell, pStub, (void*)newCacheValue, false);
+    uintptr_t oldCacheValue = (uintptr_t)UpdatePointerPairAtomically(pCell, (void*)pStub, (void*)newCacheValue, false);
 
     if (InterfaceDispatchCell::IsCache(oldCacheValue))
     {
@@ -204,14 +204,14 @@ extern "C" void RhpVTableOffsetDispatch();
 
 typedef void (*InterfaceDispatchStub)();
 
-static void * g_rgDispatchStubs[CID_MAX_CACHE_SIZE_LOG2 + 1] = {
-    (void *)&RhpInterfaceDispatch1,
-    (void *)&RhpInterfaceDispatch2,
-    (void *)&RhpInterfaceDispatch4,
-    (void *)&RhpInterfaceDispatch8,
-    (void *)&RhpInterfaceDispatch16,
-    (void *)&RhpInterfaceDispatch32,
-    (void *)&RhpInterfaceDispatch64,
+static const PCODE g_rgDispatchStubs[CID_MAX_CACHE_SIZE_LOG2 + 1] = {
+    (PCODE)&RhpInterfaceDispatch1,
+    (PCODE)&RhpInterfaceDispatch2,
+    (PCODE)&RhpInterfaceDispatch4,
+    (PCODE)&RhpInterfaceDispatch8,
+    (PCODE)&RhpInterfaceDispatch16,
+    (PCODE)&RhpInterfaceDispatch32,
+    (PCODE)&RhpInterfaceDispatch64,
 };
 
 // Map a cache size into a linear index.
@@ -241,12 +241,12 @@ static uint32_t CacheSizeToIndex(uint32_t cCacheEntries)
 // Allocates and initializes new cache of the given size. If given a previous version of the cache (guaranteed
 // to be smaller) it will also pre-populate the new cache with the contents of the old. Additionally the
 // address of the interface dispatch stub associated with this size of cache is returned.
-static uintptr_t AllocateCache(uint32_t cCacheEntries, InterfaceDispatchCache * pExistingCache, const DispatchCellInfo *pNewCellInfo, void ** ppStub)
+static uintptr_t AllocateCache(uint32_t cCacheEntries, InterfaceDispatchCache * pExistingCache, const DispatchCellInfo *pNewCellInfo, PCODE * ppStub)
 {
 #ifndef FEATURE_NATIVEAOT
     if (pNewCellInfo->CellType == DispatchCellType::VTableOffset)
     {
-        *ppStub = (void *)&RhpVTableOffsetDispatch;
+        *ppStub = (PCODE)&RhpVTableOffsetDispatch;
         ASSERT(!InterfaceDispatchCell::IsCache(pNewCellInfo->GetVTableOffset()));
         return pNewCellInfo->GetVTableOffset();
     }
@@ -455,7 +455,7 @@ PCODE InterfaceDispatch_UpdateDispatchCellCache(InterfaceDispatchCell * pCell, P
     }
 
     uint32_t cNewCacheEntries = cOldCacheEntries ? cOldCacheEntries * 2 : 1;
-    void *pStub;
+    PCODE pStub;
     uintptr_t newCacheValue = AllocateCache(cNewCacheEntries, pCache, pNewCellInfo, &pStub);
     if (newCacheValue == 0)
     {

src/coreclr/tools/Common/TypeSystem/IL/Stubs/DelegateMethodILEmitter.cs

@@ -55,19 +55,20 @@ public static MethodIL EmitIL(MethodDesc method)
                 // Store the function pointer into local variable to avoid unnecessary register usage by JIT
                 ILLocalVariable functionPointer = emit.NewLocal(context.GetWellKnownType(WellKnownType.IntPtr));
 
+                MethodSignature signature = method.Signature;
+
                 codeStream.EmitLdArg(0);
                 codeStream.Emit(ILOpcode.ldfld, emit.NewToken(functionPointerField.InstantiateAsOpen()));
                 codeStream.EmitStLoc(functionPointer);
 
                 codeStream.EmitLdArg(0);
                 codeStream.Emit(ILOpcode.ldfld, emit.NewToken(firstParameterField.InstantiateAsOpen()));
-                for (int i = 0; i < method.Signature.Length; i++)
+                for (int i = 0; i < signature.Length; i++)
                 {
                     codeStream.EmitLdArg(i + 1);
                 }
                 codeStream.EmitLdLoc(functionPointer);
 
-                MethodSignature signature = method.Signature;
                 if (method.OwningType.HasInstantiation)
                 {
                     // If the owning type is generic, the signature will contain T's and U's.