From eb2451cef000bc93b6b1e23dbe5f7789af590467 Mon Sep 17 00:00:00 2001 From: Anubhav Srivastava Date: Tue, 3 Dec 2019 09:30:14 -0800 Subject: [PATCH] Determinism fixes (#306) * More determinism fixes. * Address PR comments. Replace lock in GetILBytes with Interlocked.CompareExchange. Remove comment saying that ILCache would eventually be removed. Fix ModuleToIndex not adding all assemblies. * Add determinism test that compiles CORE_ROOT with two seeds. Fix nit in ManifestMetadataTableNode. --- .../Internal/NativeFormat/NativeFormatWriter.cs | 11 +-- .../src/tools/Common/TypeSystem/IL/EcmaMethodIL.cs | 13 ++-- .../ReadyToRun/ManifestMetadataTableNode.cs | 37 ++++++---- .../ReadyToRun/NewArrayFixupSignature.cs | 12 ++- .../ReadyToRun/StringImportSignature.cs | 10 ++- .../Compiler/ReadyToRunCodegenCompilation.cs | 15 ++-- .../src/readytorun/coreroot_determinism/Program.cs | 86 ++++++++++++++++++++++ .../coreroot_determinism.csproj | 17 +++++ 8 files changed, 162 insertions(+), 39 deletions(-) create mode 100644 src/coreclr/tests/src/readytorun/coreroot_determinism/Program.cs create mode 100644 src/coreclr/tests/src/readytorun/coreroot_determinism/coreroot_determinism.csproj diff --git a/src/coreclr/src/tools/Common/Internal/NativeFormat/NativeFormatWriter.cs b/src/coreclr/src/tools/Common/Internal/NativeFormat/NativeFormatWriter.cs index c170e37..09f5555 100644 --- a/src/coreclr/src/tools/Common/Internal/NativeFormat/NativeFormatWriter.cs +++ b/src/coreclr/src/tools/Common/Internal/NativeFormat/NativeFormatWriter.cs @@ -6,6 +6,7 @@ using System; using System.IO; using System.Diagnostics; using System.Collections.Generic; +using System.Linq; using System.Text; // Managed mirror of NativeFormatWriter.h/.cpp @@ -2041,13 +2042,9 @@ namespace Internal.NativeFormat // we can use the ordering to terminate the lookup prematurely. uint mask = ((_nBuckets - 1) << 8) | 0xFF; - // sort it by hashcode - _Entries.Sort( - (a, b) => - { - return (int)(a.Hashcode & mask) - (int)(b.Hashcode & mask); - } - ); + // Sort by hashcode. This sort must be stable since we need determinism even if two entries have + // the same hashcode. This is deterministic if entries are added in a deterministic order. + _Entries = _Entries.OrderBy(entry => entry.Hashcode & mask).ToList(); // Start with maximum size entries _entryIndexSize = 2; diff --git a/src/coreclr/src/tools/Common/TypeSystem/IL/EcmaMethodIL.cs b/src/coreclr/src/tools/Common/TypeSystem/IL/EcmaMethodIL.cs index f15b867..db2443a 100644 --- a/src/coreclr/src/tools/Common/TypeSystem/IL/EcmaMethodIL.cs +++ b/src/coreclr/src/tools/Common/TypeSystem/IL/EcmaMethodIL.cs @@ -6,7 +6,7 @@ using System; using System.Collections.Immutable; using System.Reflection.Metadata; using System.Reflection.Metadata.Ecma335; - +using System.Threading; using Internal.TypeSystem; using Internal.TypeSystem.Ecma; @@ -64,8 +64,8 @@ namespace Internal.IL 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 @@ -97,7 +97,9 @@ namespace Internal.IL EcmaSignatureParser parser = new EcmaSignatureParser(_module, signatureReader); LocalVariableDefinition[] locals = parser.ParseLocalsSignature(); - return (_locals = locals); + + Interlocked.CompareExchange(ref _locals, locals, null); + return _locals; } public override ILExceptionRegion[] GetExceptionRegions() @@ -131,7 +133,8 @@ namespace Internal.IL } } - return (_ilExceptionRegions = ilExceptionRegions); + Interlocked.CompareExchange(ref _ilExceptionRegions, ilExceptionRegions, null); + return _ilExceptionRegions; } public override object GetObject(int token) diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ManifestMetadataTableNode.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ManifestMetadataTableNode.cs index 933414b..59be18f 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ManifestMetadataTableNode.cs +++ b/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 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun private readonly Dictionary _assemblyRefToModuleIdMap; /// - /// 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. /// - private readonly List _manifestAssemblies; + private readonly Dictionary _moduleIdToAssemblyNameMap; + /// /// Registered signature emitters. @@ -69,7 +71,7 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun : base(inputModule.Context.Target) { _assemblyRefToModuleIdMap = new Dictionary(); - _manifestAssemblies = new List(); + _moduleIdToAssemblyNameMap = new Dictionary(); _signatureEmitters = new List(); _nodeFactory = nodeFactory; @@ -95,9 +97,22 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun 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 (!_manifestAssemblies.Contains(assemblyName)) + if (!_moduleIdToAssemblyNameMap.ContainsKey(assemblyRefIndex)) { if (_emissionCompleted) { @@ -108,14 +123,9 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun // the verification logic would be broken at runtime. Debug.Assert(_nodeFactory.CompilationModuleGroup.VersionsWithModule(module)); - _manifestAssemblies.Add(assemblyName); - if (!_assemblyRefToModuleIdMap.ContainsKey(assemblyName.Name)) - _assemblyRefToModuleIdMap.Add(assemblyName.Name, _nextModuleId); - - _nextModuleId++; + _moduleIdToAssemblyNameMap.Add(assemblyRefIndex, assemblyName); } - - return _assemblyRefToModuleIdMap[assemblyName.Name]; + return assemblyRefIndex; } public override int ClassCode => 791828335; @@ -166,8 +176,9 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun fieldList: MetadataTokens.FieldDefinitionHandle(1), methodList: MetadataTokens.MethodDefinitionHandle(1)); - foreach (AssemblyName assemblyName in _manifestAssemblies) + foreach (var idAndAssemblyName in _moduleIdToAssemblyNameMap.OrderBy(x => x.Key)) { + AssemblyName assemblyName = idAndAssemblyName.Value; AssemblyFlags assemblyFlags = 0; byte[] publicKeyOrToken; if ((assemblyName.Flags & AssemblyNameFlags.PublicKey) != 0) diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewArrayFixupSignature.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewArrayFixupSignature.cs index bff6c6c..99107cc 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewArrayFixupSignature.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewArrayFixupSignature.cs @@ -26,11 +26,15 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun { ReadyToRunCodegenNodeFactory r2rFactory = (ReadyToRunCodegenNodeFactory)factory; ObjectDataSignatureBuilder dataBuilder = new ObjectDataSignatureBuilder(); - dataBuilder.AddSymbol(this); - EcmaModule targetModule = _signatureContext.GetTargetModule(_arrayType); - SignatureContext innerContext = dataBuilder.EmitFixup(r2rFactory, ReadyToRunFixupKind.NewArray, targetModule, _signatureContext); - dataBuilder.EmitTypeSignature(_arrayType, innerContext); + if (!relocsOnly) + { + dataBuilder.AddSymbol(this); + + EcmaModule targetModule = _signatureContext.GetTargetModule(_arrayType); + SignatureContext innerContext = dataBuilder.EmitFixup(r2rFactory, ReadyToRunFixupKind.NewArray, targetModule, _signatureContext); + dataBuilder.EmitTypeSignature(_arrayType, innerContext); + } return dataBuilder.ToObjectData(); } diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/StringImportSignature.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/StringImportSignature.cs index 609f794..382c566 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/StringImportSignature.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/StringImportSignature.cs @@ -25,10 +25,14 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun { ReadyToRunCodegenNodeFactory r2rFactory = (ReadyToRunCodegenNodeFactory)factory; ObjectDataSignatureBuilder dataBuilder = new ObjectDataSignatureBuilder(); - dataBuilder.AddSymbol(this); - dataBuilder.EmitFixup(r2rFactory, ReadyToRunFixupKind.StringHandle, _token.Module, _signatureContext); - dataBuilder.EmitUInt(_token.TokenRid); + if (!relocsOnly) + { + dataBuilder.AddSymbol(this); + + dataBuilder.EmitFixup(r2rFactory, ReadyToRunFixupKind.StringHandle, _token.Module, _signatureContext); + dataBuilder.EmitUInt(_token.TokenRid); + } return dataBuilder.ToObjectData(); } diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs index 484ed78..25c0650 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs @@ -26,7 +26,7 @@ namespace ILCompiler protected readonly NodeFactory _nodeFactory; protected readonly Logger _logger; private readonly DevirtualizationManager _devirtualizationManager; - private ILCache _methodILCache; + protected ILCache _methodILCache; private readonly HashSet _modulesBeingInstrumented; @@ -79,10 +79,6 @@ namespace ILCompiler public virtual MethodIL GetMethodIL(MethodDesc method) { - // Flush the cache when it grows too big - if (_methodILCache.Count > 1000) - _methodILCache = new ILCache(_methodILCache.ILProvider, NodeFactory.CompilationModuleGroup); - return _methodILCache.GetOrCreateValue(method).MethodIL; } @@ -106,7 +102,7 @@ namespace ILCompiler return _modulesBeingInstrumented.Contains(module); } - private sealed class ILCache : LockFreeReaderHashtable + public sealed class ILCache : LockFreeReaderHashtable { public ILProvider ILProvider { get; } private readonly CompilationModuleGroup _compilationModuleGroup; @@ -146,7 +142,7 @@ namespace ILCompiler return new MethodILData() { Method = key, MethodIL = methodIL }; } - internal class MethodILData + public class MethodILData { public MethodDesc Method; public MethodIL MethodIL; @@ -303,6 +299,11 @@ namespace ILCompiler } } } + + if (_methodILCache.Count > 1000) + { + _methodILCache = new ILCache(_methodILCache.ILProvider, NodeFactory.CompilationModuleGroup); + } } public ISymbolNode GetFieldRvaData(FieldDesc field) => NodeFactory.CopiedFieldRva(field); diff --git a/src/coreclr/tests/src/readytorun/coreroot_determinism/Program.cs b/src/coreclr/tests/src/readytorun/coreroot_determinism/Program.cs new file mode 100644 index 0000000..5955bda --- /dev/null +++ b/src/coreclr/tests/src/readytorun/coreroot_determinism/Program.cs @@ -0,0 +1,86 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.IO; +using System.Linq; +using System.Runtime.InteropServices; + +internal class Program +{ + public static bool IsTimestampByte(int i) + { + return i >= 136 && i < 140; + } + + public static int CompareDLLs(string folder1, string folder2) + { + int result = 100; + + // Check for files that failed compilation with one of the seeds but not the other + HashSet uniqueFilenames = new HashSet(Directory.GetFiles(folder1, "*.dll").Select(Path.GetFileName)); + uniqueFilenames.SymmetricExceptWith(Directory.GetFiles(folder2, "*.dll").Select(Path.GetFileName)); + foreach (string uniqueFilename in uniqueFilenames) + { + Console.WriteLine($"{uniqueFilename} was found in only one of the output folders."); + result = 1; + } + + foreach (string filename in Directory.GetFiles(folder1, "*.dll").Select(Path.GetFileName)) + { + if (uniqueFilenames.Contains(filename)) + continue; + + byte[] file1 = File.ReadAllBytes(Path.Combine(folder1, Path.GetFileName(filename))); + byte[] file2 = File.ReadAllBytes(Path.Combine(folder2, Path.GetFileName(filename))); + + if (file1.Length != file2.Length) + { + Console.WriteLine(filename); + Console.WriteLine($"Expected ReadyToRun'd files to be identical but they have different sizes ({file1.Length} and {file2.Length})"); + result = 1; + } + + for (int i = 0; i < file1.Length; ++i) + { + if (file1[i] != file2[i] && !IsTimestampByte(i)) + { + Console.WriteLine(filename); + Console.WriteLine($"Difference at non-timestamp byte {i}"); + result = 1; + } + } + + Console.WriteLine($"Files of length {file1.Length} were identical."); + } + return result; + } + + public static string OSExeSuffix(string path) => (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? path + ".exe" : path); + + public static void CompileWithSeed(int seed, string outDir) + { + string coreRootPath = Environment.GetEnvironmentVariable("CORE_ROOT"); + string superIlcPath = Path.Combine(coreRootPath, "ReadyToRun.SuperIlc", OSExeSuffix("ReadyToRun.SuperIlc")); + + Console.WriteLine($"================================== Compiling with seed {seed} =================================="); + Environment.SetEnvironmentVariable("CoreRT_DeterminismSeed", seed.ToString()); + if (Directory.Exists(outDir)) + { + Directory.Delete(outDir, true); + } + Directory.CreateDirectory(outDir); + ProcessStartInfo processStartInfo = new ProcessStartInfo(superIlcPath, $"compile-directory -cr {coreRootPath} -in {coreRootPath} --nojit --noexe --large-bubble --release --nocleanup -out {outDir}"); + Process.Start(processStartInfo).WaitForExit(); + } + + public static int Main() + { + CompileWithSeed(1, "seed1"); + CompileWithSeed(2, "seed2"); + return CompareDLLs("seed1", "seed2"); + } +} diff --git a/src/coreclr/tests/src/readytorun/coreroot_determinism/coreroot_determinism.csproj b/src/coreclr/tests/src/readytorun/coreroot_determinism/coreroot_determinism.csproj new file mode 100644 index 0000000..3133e13 --- /dev/null +++ b/src/coreclr/tests/src/readytorun/coreroot_determinism/coreroot_determinism.csproj @@ -0,0 +1,17 @@ + + + exe + BuildAndRun + 0 + + true + + true + + false + 2.0 + + + + + -- 2.7.4