Crossgen2 determinism fixes

src/coreclr/src/tools/crossgen2/Common/TypeSystem/IL/EcmaMethodIL.cs

@@ -6,7 +6,7 @@
 using System.Collections.Immutable;
 using System.Reflection.Metadata;
 using System.Reflection.Metadata.Ecma335;
-
+using System.Threading;
 using Internal.TypeSystem;
 using Internal.TypeSystem.Ecma;
 
@@ -64,20 +64,8 @@ public override byte[] GetILBytes()
             if (_ilBytes != null)
                 return _ilBytes;
 
-            // This lock is necessary since the JIT requires that we provide the same address
-            // for the IL buffer each time (it uses the address to detect recursive calls).
-            // Since MethodBodyBlock.GetAllBytes creates a new array each time, if two threads
-            // call this on the same function then it is possible for both to set _ilBytes.
-            // Then, one of the threads will receive the first value on the first call and the
-            // second value on all subsequent calls, and we will not detect the recursive call.
-            lock (this)
-            {
-                if (_ilBytes != null)
-                    return _ilBytes;
-
-                byte[] ilBytes = _methodBody.GetILBytes();
-                return (_ilBytes = ilBytes);
-            }
+            Interlocked.CompareExchange(ref _ilBytes, _methodBody.GetILBytes(), null);
+            return _ilBytes;
         }
 
         public override bool IsInitLocals

src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ManifestMetadataTableNode.cs

@@ -4,12 +4,12 @@
 
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using System.Reflection;
 using System.Reflection.Metadata;
 using System.Reflection.Metadata.Ecma335;
 
 using Internal.Text;
-using Internal.TypeSystem;
 using Internal.TypeSystem.Ecma;
 
 using Debug = System.Diagnostics.Debug;
@@ -36,9 +36,11 @@ public class ManifestMetadataTableNode : HeaderTableNode
         private readonly Dictionary<string, int> _assemblyRefToModuleIdMap;
 
         /// <summary>
-        /// Assembly references to store in the manifest metadata.
+        /// Map from module index to the AssemblyName for the module. This only contains modules
+        /// that were actually loaded and is populated by ModuleToIndex.
         /// </summary>
-        private readonly List<AssemblyName> _manifestAssemblies;
+        private readonly Dictionary<int, AssemblyName> _moduleIdToAssemblyNameMap;
+
 
         /// <summary>
         /// Registered signature emitters.
@@ -69,7 +71,7 @@ public ManifestMetadataTableNode(EcmaModule inputModule, NodeFactory nodeFactory
             : base(inputModule.Context.Target)
         {
             _assemblyRefToModuleIdMap = new Dictionary<string, int>();
-            _manifestAssemblies = new List<AssemblyName>();
+            _moduleIdToAssemblyNameMap = new Dictionary<int, AssemblyName>();
             _signatureEmitters = new List<ISignatureEmitter>();
             _nodeFactory = nodeFactory;
 
@@ -95,9 +97,22 @@ public void RegisterEmitter(ISignatureEmitter emitter)
 
         public int ModuleToIndex(EcmaModule module)
         {
+            if (!_nodeFactory.MarkingComplete)
+            {
+                // If we call this function before sorting is complete, we might have a determinism bug caused by
+                // compiling two functions in an arbitrary order and hence getting different module IDs.
+                throw new InvalidOperationException("Cannot get ModuleToIndex mapping until marking is complete.");
+            }
+
             AssemblyName assemblyName = module.Assembly.GetName();
             int assemblyRefIndex;
             if (!_assemblyRefToModuleIdMap.TryGetValue(assemblyName.Name, out assemblyRefIndex))
+            {
+                assemblyRefIndex = _nextModuleId++;
+                _assemblyRefToModuleIdMap.Add(assemblyName.Name, assemblyRefIndex);
+            }
+
+            if (!_moduleIdToAssemblyNameMap.ContainsKey(assemblyRefIndex))
             {
                 if (_emissionCompleted)
                 {
@@ -108,9 +123,7 @@ public int ModuleToIndex(EcmaModule module)
                 // the verification logic would be broken at runtime.
                 Debug.Assert(_nodeFactory.CompilationModuleGroup.VersionsWithModule(module));
 
-                assemblyRefIndex = _nextModuleId++;
-                _manifestAssemblies.Add(assemblyName);
-                _assemblyRefToModuleIdMap.Add(assemblyName.Name, assemblyRefIndex);
+                _moduleIdToAssemblyNameMap.Add(assemblyRefIndex, assemblyName);
             }
             return assemblyRefIndex;
         }
@@ -163,7 +176,8 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
                fieldList: MetadataTokens.FieldDefinitionHandle(1),
                methodList: MetadataTokens.MethodDefinitionHandle(1));
 
-            foreach (AssemblyName assemblyName in _manifestAssemblies)
+            IEnumerable<AssemblyName> manifestAssemblies = _moduleIdToAssemblyNameMap.OrderBy(x => x.Key).Select(x => x.Value);
+            foreach (AssemblyName assemblyName in manifestAssemblies)
             {
                 AssemblyFlags assemblyFlags = 0;
                 byte[] publicKeyOrToken;

src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs

@@ -78,8 +78,6 @@ public bool CanInline(MethodDesc caller, MethodDesc callee)
 
         public virtual MethodIL GetMethodIL(MethodDesc method)
         {
-            // TODO: Remove the ILCache and use the pinned heap for the IL stream once this GC
-            // feature ships (in dotnet5)
             return _methodILCache.GetOrCreateValue(method).MethodIL;
         }