From 8ebc6f96edaed557cdc988efce0642f8dc3a5995 Mon Sep 17 00:00:00 2001 From: Kirill Osenkov Date: Wed, 27 Dec 2023 19:18:35 -0800 Subject: [PATCH] Update Cecil submodule to latest Read embedded pdbs Copy debug information for scopes and imports Taken from https://github.com/Alexx999/il-repack/pull/3 Make the output assembly MVID deterministic Fix test locking dlls Add .ToArray() to flush the Linq expression so that it is not evaluated more than once. Better error if the pdb doesn't exist Use cecil from https://github.com/KirillOsenkov/cecil/tree/ilrepack --- .gitmodules | 3 +- Directory.Build.props | 4 + .../NuGet/RepackNuGetTests.cs | 2 +- .../NuGet/TestHelpers.cs | 12 +- ILRepack/IKVMLineIndexer.cs | 32 +++--- ILRepack/ILRepack.cs | 38 +++++-- ILRepack/Mixins/Mixins.cs | 16 +++ ILRepack/PlatformFixer.cs | 7 +- ILRepack/ReferenceFixator.cs | 17 ++- ILRepack/RepackAssemblyResolver.cs | 8 +- ILRepack/RepackImporter.cs | 106 ++++++++++++++++-- ILRepack/Steps/SourceServerData/PdbStr.cs | 5 + ILRepack/Steps/XamlResourcePathPatcherStep.cs | 2 +- cecil | 2 +- 14 files changed, 205 insertions(+), 49 deletions(-) create mode 100644 ILRepack/Mixins/Mixins.cs diff --git a/.gitmodules b/.gitmodules index b9c2ef54..db870ce2 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,3 +1,4 @@ [submodule "cecil"] path = cecil - url = https://github.com/gluck/cecil.git \ No newline at end of file + url = https://github.com/KirillOsenkov/cecil + branch = ilrepack diff --git a/Directory.Build.props b/Directory.Build.props index 027badb7..4a0180a6 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -1,5 +1,9 @@ + + latest + + diff --git a/ILRepack.IntegrationTests/NuGet/RepackNuGetTests.cs b/ILRepack.IntegrationTests/NuGet/RepackNuGetTests.cs index 24b23bd1..9ac41853 100644 --- a/ILRepack.IntegrationTests/NuGet/RepackNuGetTests.cs +++ b/ILRepack.IntegrationTests/NuGet/RepackNuGetTests.cs @@ -136,7 +136,7 @@ public void VerifiesMergedPdbUnchangedSourceIndexationForTfsIndexation() { // This test requires Mono.Cecil.Pdb.dll. Indicate a dependency such that // the reference is not accidentally removed. - _ = typeof(Mono.Cecil.Pdb.PdbReader); + _ = typeof(Mono.Cecil.Pdb.PdbReaderProvider); var platform = Platform.From( Package.From("TfsIndexer", "1.2.4"), diff --git a/ILRepack.IntegrationTests/NuGet/TestHelpers.cs b/ILRepack.IntegrationTests/NuGet/TestHelpers.cs index 3db2908f..192e3925 100644 --- a/ILRepack.IntegrationTests/NuGet/TestHelpers.cs +++ b/ILRepack.IntegrationTests/NuGet/TestHelpers.cs @@ -38,12 +38,20 @@ public static void DoRepackForCmd(IEnumerable args) private static void ReloadAndCheckReferences(RepackOptions repackOptions) { - var outputFile = AssemblyDefinition.ReadAssembly(repackOptions.OutputFile, new ReaderParameters(ReadingMode.Immediate)); - var mergedFiles = repackOptions.ResolveFiles().Select(f => AssemblyDefinition.ReadAssembly(f, new ReaderParameters(ReadingMode.Deferred))); + using var ar = new DefaultAssemblyResolver(); + using var outputFile = AssemblyDefinition.ReadAssembly(repackOptions.OutputFile, new ReaderParameters(ReadingMode.Immediate)); + var mergedFiles = repackOptions + .ResolveFiles() + .Select(f => AssemblyDefinition.ReadAssembly(f, new ReaderParameters(ReadingMode.Deferred))) + .ToArray(); foreach (var a in outputFile.MainModule.AssemblyReferences.Where(x => mergedFiles.Any(y => repackOptions.KeepOtherVersionReferences ? x.FullName == y.FullName : x.Name == y.Name.Name))) { Assert.Fail($"Merged assembly retains a reference to one (or more) of the merged files: {a.FullName}"); } + foreach (var a in mergedFiles) + { + a.Dispose(); + } } public static void SaveAs(Stream input, string directory, string fileName) diff --git a/ILRepack/IKVMLineIndexer.cs b/ILRepack/IKVMLineIndexer.cs index 5f14c285..5155642b 100644 --- a/ILRepack/IKVMLineIndexer.cs +++ b/ILRepack/IKVMLineIndexer.cs @@ -17,7 +17,6 @@ internal class IKVMLineIndexer { private readonly IRepackContext repack; private bool enabled; - private LineNumberWriter lineNumberWriter; private string fileName; private TypeReference sourceFileAttributeTypeReference; private TypeReference lineNumberTableAttributeTypeReference; @@ -41,29 +40,29 @@ public IKVMLineIndexer(IRepackContext ilRepack, bool doLineIndexing) public void Reset() { - lineNumberWriter = null; fileName = null; } public void PreMethodBodyRepack(MethodBody body, MethodDefinition parent) { - if (!enabled) + if (!enabled || !parent.DebugInformation.HasSequencePoints) return; Reset(); if (!parent.CustomAttributes.Any(x => x.Constructor.DeclaringType.Name == "LineNumberTableAttribute")) { - lineNumberWriter = new LineNumberWriter(body.Instructions.Count / 4); + var lineNumberWriter = new LineNumberWriter(body.Instructions.Count / 4); + foreach (var sp in parent.DebugInformation.SequencePoints) + { + AddSeqPoint(sp, lineNumberWriter); + } + PostMethodBodyRepack(parent, lineNumberWriter); } } - public void ProcessMethodBodyInstruction(Instruction instr) + private void AddSeqPoint(SequencePoint currentSeqPoint, LineNumberWriter lineNumberWriter) { - if (!enabled) - return; - - var currentSeqPoint = instr.SequencePoint; - if (lineNumberWriter != null && currentSeqPoint != null) + if (currentSeqPoint != null) { if (fileName == null && currentSeqPoint.Document != null) { @@ -84,25 +83,22 @@ public void ProcessMethodBodyInstruction(Instruction instr) { if (lineNumberWriter.LineNo > 0) { - lineNumberWriter.AddMapping(instr.Offset, -1); + lineNumberWriter.AddMapping(currentSeqPoint.Offset, -1); } } else { if (lineNumberWriter.LineNo != currentSeqPoint.StartLine) { - lineNumberWriter.AddMapping(instr.Offset, currentSeqPoint.StartLine); + lineNumberWriter.AddMapping(currentSeqPoint.Offset, currentSeqPoint.StartLine); } } } } - public void PostMethodBodyRepack(MethodDefinition parent) + private void PostMethodBodyRepack(MethodDefinition parent, LineNumberWriter lineNumberWriter) { - if (!enabled) - return; - - if (lineNumberWriter != null && lineNumberWriter.Count > 0) + if (lineNumberWriter.Count > 0) { CustomAttribute ca; if (lineNumberWriter.Count == 1) @@ -155,7 +151,7 @@ public void PostRepackReferences() IMetadataScope ikvmRuntimeReference = repack.TargetAssemblyMainModule.AssemblyReferences.FirstOrDefault(r => r.Name == "IKVM.Runtime"); if (ikvmRuntimeReference == null) { - ikvmRuntimeReference = repack.MergeScope(repack.GlobalAssemblyResolver.Resolve("IKVM.Runtime").Name); + ikvmRuntimeReference = repack.MergeScope(repack.GlobalAssemblyResolver.Resolve(new AssemblyNameReference("IKVM.Runtime", null)).Name); } if (ikvmRuntimeReference == null) { diff --git a/ILRepack/ILRepack.cs b/ILRepack/ILRepack.cs index 055acb9e..704e29eb 100644 --- a/ILRepack/ILRepack.cs +++ b/ILRepack/ILRepack.cs @@ -21,6 +21,7 @@ using System.Text.RegularExpressions; using ILRepacking.Steps; using Mono.Cecil; +using Mono.Cecil.Cil; using Mono.Cecil.PE; using Mono.Unix.Native; using ILRepacking.Mixins; @@ -112,9 +113,10 @@ private AssemblyDefinitionContainer ReadInputAssembly(string assembly, bool isPr { ReaderParameters rp = new ReaderParameters(ReadingMode.Immediate) { AssemblyResolver = GlobalAssemblyResolver }; // read PDB/MDB? - if (Options.DebugInfo && (File.Exists(Path.ChangeExtension(assembly, "pdb")) || File.Exists(assembly + ".mdb"))) + if (Options.DebugInfo) { rp.ReadSymbols = true; + rp.SymbolReaderProvider = new DefaultSymbolReaderProvider(throwIfNoSymbol: false); } AssemblyDefinition mergeAsm; try @@ -144,6 +146,7 @@ private AssemblyDefinitionContainer ReadInputAssembly(string assembly, bool isPr if (!Options.AllowZeroPeKind && (mergeAsm.MainModule.Attributes & ModuleAttributes.ILOnly) == 0) throw new ArgumentException("Failed to load assembly with Zero PeKind: " + assembly); + GlobalAssemblyResolver.RegisterAssembly(mergeAsm); return new AssemblyDefinitionContainer { @@ -264,7 +267,6 @@ public void Repack() // Read input assemblies only after all properties are set. ReadInputAssemblies(); - GlobalAssemblyResolver.RegisterAssemblies(MergedAssemblies); _platformFixer = new PlatformFixer(this, PrimaryAssemblyMainModule.Runtime); _mappingHandler = new MappingHandler(); @@ -309,7 +311,7 @@ public void Repack() } // set the main module attributes TargetAssemblyMainModule.Attributes = PrimaryAssemblyMainModule.Attributes; - TargetAssemblyMainModule.Win32ResourceDirectory = MergeWin32Resources(PrimaryAssemblyMainModule.Win32ResourceDirectory); + TargetAssemblyMainModule.Win32ResourceDirectory = MergeWin32Resources(); if (Options.Version != null) TargetAssemblyDefinition.Name.Version = Options.Version; @@ -337,10 +339,16 @@ public void Repack() step.Perform(); } + var anySymbolReader = MergedAssemblies + .Select(m => m.MainModule.SymbolReader) + .Where(r => r != null) + .FirstOrDefault(); var parameters = new WriterParameters { StrongNameKeyPair = signingStep.KeyPair, - WriteSymbols = Options.DebugInfo + WriteSymbols = Options.DebugInfo, + SymbolWriterProvider = anySymbolReader?.GetWriterProvider(), + DeterministicMvid = true }; // create output directory if it does not exist var outputDir = Path.GetDirectoryName(Options.OutputFile); @@ -350,11 +358,19 @@ public void Repack() Directory.CreateDirectory(outputDir); } + Logger.Info("Writing output assembly to disk"); TargetAssemblyDefinition.Write(Options.OutputFile, parameters); sourceServerDataStep.Write(); - Logger.Info("Writing output assembly to disk"); + foreach (var assembly in MergedAssemblies) + { + assembly.Dispose(); + } + + TargetAssemblyDefinition.Dispose(); + GlobalAssemblyResolver.Dispose(); + // If this is an executable and we are on linux/osx we should copy file permissions from // the primary assembly if (isUnixEnvironment && (kind == ModuleKind.Console || kind == ModuleKind.Windows)) @@ -405,13 +421,17 @@ private void ResolveSearchDirectories() } } - private ResourceDirectory MergeWin32Resources(ResourceDirectory primary) + private ResourceDirectory MergeWin32Resources() { - if (primary == null) - return null; + var primary = PrimaryAssemblyMainModule.ReadWin32ResourceDirectory() ?? new ResourceDirectory(); + foreach (var ass in OtherAssemblies) { - MergeDirectory(new List(), primary, ass, ass.MainModule.Win32ResourceDirectory); + var directory = ass.MainModule.ReadWin32ResourceDirectory(); + if (directory != null) + { + MergeDirectory(new List(), primary, ass, directory); + } } return primary; } diff --git a/ILRepack/Mixins/Mixins.cs b/ILRepack/Mixins/Mixins.cs new file mode 100644 index 00000000..b95c27e2 --- /dev/null +++ b/ILRepack/Mixins/Mixins.cs @@ -0,0 +1,16 @@ +using System.Collections.Generic; +using Mono.Collections.Generic; + +namespace ILRepacking.Mixins +{ + static class CollectionMixins + { + public static void AddRange(this Collection destination, IEnumerable source) + { + foreach (var item in source) + { + destination.Add(item); + } + } + } +} \ No newline at end of file diff --git a/ILRepack/PlatformFixer.cs b/ILRepack/PlatformFixer.cs index 66b24afc..f494f2c5 100644 --- a/ILRepack/PlatformFixer.cs +++ b/ILRepack/PlatformFixer.cs @@ -192,7 +192,7 @@ private void FixPlatformVersion(ParameterDefinition pd) private void FixPlatformVersion(GenericParameter gp) { if (gp.HasConstraints) - foreach (TypeReference tr in gp.Constraints) + foreach (var tr in gp.Constraints) FixPlatformVersion(tr); if (gp.HasCustomAttributes) foreach (CustomAttribute ca in gp.CustomAttributes) @@ -202,6 +202,11 @@ private void FixPlatformVersion(GenericParameter gp) FixPlatformVersion(gp1); } + private void FixPlatformVersion(GenericParameterConstraint gp) + { + FixPlatformVersion(gp.ConstraintType); + } + private void FixPlatformVersion(CustomAttribute ca) { FixPlatformVersion(ca.Constructor); diff --git a/ILRepack/ReferenceFixator.cs b/ILRepack/ReferenceFixator.cs index f324825f..94ec77fd 100755 --- a/ILRepack/ReferenceFixator.cs +++ b/ILRepack/ReferenceFixator.cs @@ -111,6 +111,15 @@ internal void FixMethodVisibility(TypeDefinition type) FixOverridenMethodDef(meth); } + private void FixReferences(Collection constraints) + { + foreach (var constraint in constraints) + { + constraint.ConstraintType = Fix(constraint.ConstraintType); + FixReferences(constraint.CustomAttributes); + } + } + internal void FixReferences(TypeDefinition type) { FixReferences(type.GenericParameters); @@ -118,7 +127,11 @@ internal void FixReferences(TypeDefinition type) type.BaseType = Fix(type.BaseType); // interfaces before methods, because methods will have to go through them - FixReferences(type.Interfaces); + foreach (InterfaceImplementation nested in type.Interfaces) + { + nested.InterfaceType = Fix(nested.InterfaceType); + FixReferences(nested.CustomAttributes); + } // nested types first foreach (TypeDefinition nested in type.NestedTypes) @@ -332,7 +345,7 @@ private bool IsAnnotation(TypeDefinition typeAttribute) { if (typeAttribute == null) return false; - if (typeAttribute.Interfaces.Any(@interface => @interface.FullName == "java.lang.annotation.Annotation")) + if (typeAttribute.Interfaces.Any(@interface => @interface.InterfaceType.FullName == "java.lang.annotation.Annotation")) return true; return typeAttribute.BaseType != null && IsAnnotation(typeAttribute.BaseType.Resolve()); } diff --git a/ILRepack/RepackAssemblyResolver.cs b/ILRepack/RepackAssemblyResolver.cs index 9af448c8..27d27fb3 100644 --- a/ILRepack/RepackAssemblyResolver.cs +++ b/ILRepack/RepackAssemblyResolver.cs @@ -1,16 +1,12 @@ using Mono.Cecil; -using System.Collections.Generic; namespace ILRepacking { public class RepackAssemblyResolver : DefaultAssemblyResolver { - public void RegisterAssemblies(IList mergedAssemblies) + public new void RegisterAssembly(AssemblyDefinition assembly) { - foreach (var assemblyDefinition in mergedAssemblies) - { - RegisterAssembly(assemblyDefinition); - } + base.RegisterAssembly(assembly); } } } diff --git a/ILRepack/RepackImporter.cs b/ILRepack/RepackImporter.cs index 50344981..54158d2c 100644 --- a/ILRepack/RepackImporter.cs +++ b/ILRepack/RepackImporter.cs @@ -14,6 +14,7 @@ // See the License for the specific language governing permissions and // limitations under the License. // +using ILRepacking.Mixins; using Mono.Cecil; using Mono.Cecil.Cil; using Mono.Collections.Generic; @@ -29,6 +30,8 @@ internal class RepackImporter : IRepackImporter, IRepackCopier private readonly IRepackContext _repackContext; private readonly RepackOptions _options; private readonly Dictionary _aspOffsets; + private readonly Dictionary _importDebugInformations = new(); + private readonly static Instruction _dummyInstruction = Instruction.Create(OpCodes.Nop); const string ExcludeInternalizeAttName = "RepackExcludeInternalizeAttribute"; @@ -388,6 +391,10 @@ private void CloneTo(MethodDefinition meth, TypeDefinition type, bool typeJustCr // use void placeholder as we'll do the return type import later on (after generic parameters) MethodDefinition nm = new MethodDefinition(meth.Name, meth.Attributes, _repackContext.TargetAssemblyMainModule.TypeSystem.Void); nm.ImplAttributes = meth.ImplAttributes; + if (meth.DebugInformation.HasCustomDebugInformations) + nm.DebugInformation.CustomDebugInformations.AddRange(meth.DebugInformation.CustomDebugInformations); + if (meth.DebugInformation.HasSequencePoints) + nm.DebugInformation.SequencePoints.AddRange(meth.DebugInformation.SequencePoints); type.Methods.Add(nm); @@ -426,6 +433,9 @@ private void CloneTo(MethodDefinition meth, TypeDefinition type, bool typeJustCr if (meth.HasBody) CloneTo(meth.Body, nm); + + nm.DebugInformation.Scope = CopyScope(meth.DebugInformation.Scope, nm, out _); + meth.Body = null; // frees memory nm.IsAddOn = meth.IsAddOn; @@ -435,6 +445,72 @@ private void CloneTo(MethodDefinition meth, TypeDefinition type, bool typeJustCr nm.CallingConvention = meth.CallingConvention; } + private ScopeDebugInformation CopyScope(ScopeDebugInformation scope, MethodDefinition nm, out bool copied) + { + copied = false; + if (scope is null || scope.Import is null && !scope.HasConstants && !scope.HasScopes) + return scope; + + var ns = new ScopeDebugInformation(_dummyInstruction, null); + ns.Start = new InstructionOffset(scope.Start.Offset); + ns.End = scope.End.IsEndOfMethod ? default : new InstructionOffset(scope.End.Offset); + if (scope.HasCustomDebugInformations) + ns.CustomDebugInformations.AddRange(scope.CustomDebugInformations); + if (scope.HasVariables) + ns.Variables.AddRange(scope.Variables); + if (scope.HasScopes) + foreach (var ps in scope.Scopes) + { + ns.Scopes.Add(CopyScope(ps, nm, out var nc)); + copied |= nc; + } + if (scope.HasConstants) + { + copied = true; + foreach (var pc in scope.Constants) + { + var nc = new ConstantDebugInformation(pc.Name, Import(pc.ConstantType, nm), pc.Value); + if (pc.HasCustomDebugInformations) + nc.CustomDebugInformations.AddRange(pc.CustomDebugInformations); + ns.Constants.Add(nc); + } + } + if (scope.Import is not null) + { + copied = true; + ns.Import = CopyImport(scope.Import, nm); + } + + return copied ? ns : scope; + } + + private ImportDebugInformation CopyImport(ImportDebugInformation import, MethodDefinition nm) + { + if (import is null) + return null; + if (_importDebugInformations.TryGetValue(import, out var ni)) + return ni; + + ni = new ImportDebugInformation(); + ni.Parent = CopyImport(import.Parent, nm); + if (import.HasCustomDebugInformations) + ni.CustomDebugInformations.AddRange(import.CustomDebugInformations); + if (import.HasTargets) + foreach (var pt in import.Targets) + { + var nt = new ImportTarget(pt.Kind); + nt.Alias = pt.Alias; + nt.Namespace = pt.Namespace; + if (pt.Type is not null) + nt.Type = Import(pt.Type, nm); + if (pt.AssemblyReference is not null) + nt.AssemblyReference = _repackContext.PlatformFixer.FixPlatformVersion(pt.AssemblyReference) as AssemblyNameReference; + ni.Targets.Add(nt); + } + _importDebugInformations.Add(import, ni); + return ni; + } + private void CloneTo(MethodBody body, MethodDefinition parent) { MethodBody nb = new MethodBody(parent); @@ -445,15 +521,13 @@ private void CloneTo(MethodBody body, MethodDefinition parent) nb.LocalVarToken = body.LocalVarToken; foreach (VariableDefinition var in body.Variables) - nb.Variables.Add(new VariableDefinition(var.Name, + nb.Variables.Add(new VariableDefinition( Import(var.VariableType, parent))); - nb.Instructions.SetCapacity(body.Instructions.Count); + nb.Instructions.Capacity = Math.Max(nb.Instructions.Capacity, body.Instructions.Count); _repackContext.LineIndexer.PreMethodBodyRepack(body, parent); foreach (Instruction instr in body.Instructions) { - _repackContext.LineIndexer.ProcessMethodBodyInstruction(instr); - Instruction ni; if (instr.OpCode.Code == Code.Calli) @@ -542,10 +616,8 @@ private void CloneTo(MethodBody body, MethodDefinition parent) default: throw new InvalidOperationException(); } - ni.SequencePoint = instr.SequencePoint; nb.Instructions.Add(ni); } - _repackContext.LineIndexer.PostMethodBodyRepack(parent); for (int i = 0; i < body.Instructions.Count; i++) { @@ -607,11 +679,21 @@ private TypeDefinition CreateType(TypeDefinition type, Collection interfaces1, Collection interfaces2, TypeDefinition nt) + { + foreach (var iface in interfaces1) + { + var newIface = new InterfaceImplementation(Import(iface.InterfaceType, nt)); + CopyCustomAttributes(iface.CustomAttributes, newIface.CustomAttributes, nt); + interfaces2.Add(newIface); + } + } + private MethodDefinition FindMethodInNewType(TypeDefinition nt, MethodDefinition methodDefinition) { var ret = _repackContext.ReflectionHelper.FindMethodDefinitionInType(nt, methodDefinition); @@ -842,6 +924,16 @@ public void CopyTypeReferences(Collection input, Collection input, Collection output, IGenericParameterProvider context) + { + foreach (var gpc in input) + { + var result = new GenericParameterConstraint(Import(gpc.ConstraintType, context)); + CopyCustomAttributes(gpc.CustomAttributes, result.CustomAttributes, context); + output.Add(result); + } + } + public CustomAttributeArgument Copy(CustomAttributeArgument arg, IGenericParameterProvider context) { return new CustomAttributeArgument(Import(arg.Type, context), ImportCustomAttributeValue(arg.Value, context)); diff --git a/ILRepack/Steps/SourceServerData/PdbStr.cs b/ILRepack/Steps/SourceServerData/PdbStr.cs index 0f9d20b8..1ff8cd6a 100644 --- a/ILRepack/Steps/SourceServerData/PdbStr.cs +++ b/ILRepack/Steps/SourceServerData/PdbStr.cs @@ -19,6 +19,11 @@ public PdbStr() public string Read(string pdb) { + if (!File.Exists(pdb)) + { + return $"File doesn't exist: {pdb}"; + } + return Execute($"-r -p:{pdb} -s:srcsrv"); } diff --git a/ILRepack/Steps/XamlResourcePathPatcherStep.cs b/ILRepack/Steps/XamlResourcePathPatcherStep.cs index 1d7dff5c..c8a91b1c 100644 --- a/ILRepack/Steps/XamlResourcePathPatcherStep.cs +++ b/ILRepack/Steps/XamlResourcePathPatcherStep.cs @@ -69,7 +69,7 @@ private void PatchWpfToolkitVersionResourceDictionary(TypeDefinition type) private void PatchIComponentConnector(TypeDefinition type) { - if (!type.Interfaces.Any(t => t.FullName == "System.Windows.Markup.IComponentConnector")) + if (!type.Interfaces.Any(t => t.InterfaceType.FullName == "System.Windows.Markup.IComponentConnector")) return; var initializeMethod = type.Methods.FirstOrDefault(m => diff --git a/cecil b/cecil index c854cad1..ebbba77e 160000 --- a/cecil +++ b/cecil @@ -1 +1 @@ -Subproject commit c854cad1a1da59ea94e5e87cbcc0b0a1416c9b62 +Subproject commit ebbba77e4da9e9d21e925c51c7b5317f9a58b591