From 9dfda4fd9e72d979f7a8c596544b5d805f4f8537 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 8 Oct 2021 09:52:27 -0700 Subject: [PATCH] Fix issue with incorrect handling of optimized method layouts - The sort order of the RuntimeFunctions table (among others) was not properly handled when an optimized method layout was applied - While this bug applied to non-composite builds, it was easily reproduced with a composite build - Fix lack of test infrastructure around non-standard method layouts. - Add new random method sorting order - Use new random sorting order in crossgen2 test passes (#60109) Fixes #59089 Co-authored-by: David Wrighton --- .../Compiler/ReadyToRunFileLayoutOptimizer.cs | 12 ++++++++++++ .../Compiler/ReadyToRunTableManager.cs | 22 ++++++++++++++++++---- src/coreclr/tools/aot/crossgen2/Program.cs | 1 + .../tools/aot/crossgen2/Properties/Resources.resx | 2 +- src/tests/Common/CLRTest.CrossGen.targets | 1 + 5 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunFileLayoutOptimizer.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunFileLayoutOptimizer.cs index d8d6d36..4ef5443 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunFileLayoutOptimizer.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunFileLayoutOptimizer.cs @@ -26,6 +26,7 @@ namespace ILCompiler HotWarmCold, CallFrequency, PettisHansen, + Random, } public enum ReadyToRunFileLayoutAlgorithm @@ -166,6 +167,17 @@ namespace ILCompiler methods = PettisHansenSort(methods); break; + case ReadyToRunMethodLayoutAlgorithm.Random: + Random rand = new Random(0); + for (int i = 0; i < methods.Count - 1; i++) + { + int j = rand.Next(i, methods.Count); + MethodWithGCInfo temp = methods[i]; + methods[i] = methods[j]; + methods[j] = temp; + } + break; + default: throw new NotImplementedException(_methodLayoutAlgorithm.ToString()); } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunTableManager.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunTableManager.cs index 8f37f8e..e8b8f0a 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunTableManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunTableManager.cs @@ -101,16 +101,30 @@ namespace ILCompiler { if (!_sortedMethods) { - TypeSystemComparer comparer = new TypeSystemComparer(); - Comparison sortHelper = (x, y) => comparer.Compare(x.Method, y.Method); + CompilerComparer comparer = new CompilerComparer(); + SortableDependencyNode.ObjectNodeComparer objectNodeComparer = new SortableDependencyNode.ObjectNodeComparer(comparer); + Comparison sortHelper = (x, y) => + { + int nodeComparerResult = objectNodeComparer.Compare((SortableDependencyNode)x, (SortableDependencyNode)y); +#if DEBUG + int methodOnlyResult = comparer.Compare(x.Method, y.Method); + + // Assert the two sorting techniques produce the same result unless there is a CustomSort applied + Debug.Assert((nodeComparerResult == methodOnlyResult) || + ((x is SortableDependencyNode sortableX && sortableX.CustomSort != Int32.MaxValue) || + (y is SortableDependencyNode sortableY && sortableY.CustomSort != Int32.MaxValue))); +#endif + return nodeComparerResult; + }; + Comparison sortHelperNoCustomSort = (x, y) => comparer.Compare(x, y); List perModuleDatas = new List(_methodsGenerated.Values); perModuleDatas.Sort((x, y) => x.Module.CompareTo(y.Module)); foreach (var perModuleData in perModuleDatas) { - perModuleData.MethodsGenerated.MergeSort(sortHelper); - perModuleData.GenericMethodsGenerated.MergeSort(sortHelper); + perModuleData.MethodsGenerated.MergeSort(sortHelperNoCustomSort); + perModuleData.GenericMethodsGenerated.MergeSort(sortHelperNoCustomSort); _completeSortedMethods.AddRange(perModuleData.MethodsGenerated); _completeSortedMethods.AddRange(perModuleData.GenericMethodsGenerated); _completeSortedGenericMethods.AddRange(perModuleData.GenericMethodsGenerated); diff --git a/src/coreclr/tools/aot/crossgen2/Program.cs b/src/coreclr/tools/aot/crossgen2/Program.cs index afae84f..e9eb592 100644 --- a/src/coreclr/tools/aot/crossgen2/Program.cs +++ b/src/coreclr/tools/aot/crossgen2/Program.cs @@ -156,6 +156,7 @@ namespace ILCompiler "hotwarmcold" => ReadyToRunMethodLayoutAlgorithm.HotWarmCold, "callfrequency" => ReadyToRunMethodLayoutAlgorithm.CallFrequency, "pettishansen" => ReadyToRunMethodLayoutAlgorithm.PettisHansen, + "random" => ReadyToRunMethodLayoutAlgorithm.Random, _ => throw new CommandLineException(SR.InvalidMethodLayout) }; } diff --git a/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx b/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx index c122cd6..85ba1a7 100644 --- a/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx +++ b/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx @@ -148,7 +148,7 @@ Method layout must be either DefaultSort or MethodOrder. - Method layout must be either DefaultSort, ExclusiveWeight, HotCold, HotWarmCold, CallFrequency or PettisHansen. + Method layout must be either DefaultSort, ExclusiveWeight, HotCold, HotWarmCold, CallFrequency, PettisHansen, or Random. True to skip compiling methods into the R2R image (default = false) diff --git a/src/tests/Common/CLRTest.CrossGen.targets b/src/tests/Common/CLRTest.CrossGen.targets index 94c6c40..4d3e8ea 100644 --- a/src/tests/Common/CLRTest.CrossGen.targets +++ b/src/tests/Common/CLRTest.CrossGen.targets @@ -225,6 +225,7 @@ if defined RunCrossGen2 ( echo -o:!__OutputFile!>>!__ResponseFile! echo --targetarch:$(TargetArchitecture)>>!__ResponseFile! echo --verify-type-and-field-layout>>!__ResponseFile! + echo --method-layout:random>>!__ResponseFile! echo -r:!CORE_ROOT!\System.*.dll>>!__ResponseFile! echo -r:!CORE_ROOT!\Microsoft.*.dll>>!__ResponseFile! echo -r:!CORE_ROOT!\mscorlib.dll>>!__ResponseFile! -- 2.7.4