From a872d20f38e5cbdfce867ef9564ac50faa7f22a5 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Tue, 12 Dec 2023 16:38:06 -0600 Subject: [PATCH 1/2] [Java.Interop.Tools.Cecil] fix symbol loading in `DirectoryAssemblyResolver` Context: https://github.com/xamarin/java.interop/commit/7d42864d Context: https://github.com/xamarin/xamarin-android/pull/8571 While debugging xamarin/xamarin-android#8571, I found the usage of `MemoryMappedFile` (from 7d42864d) broke `.pdb` symbol loading. This is OK, because we'll likely disable symbol loading anyway, but we should at least address our bugs here for the future. In order for the following code to load symbols: AssemblyDefinition result = ModuleDefinition.ReadModule (viewStream, options).Assembly; You would need the following `ReaderParameters`: * `ReadSymbols=true` * `SymbolStream` containing a valid `Stream` to the `.pdb` file To make this work, I had to: * Create a `List` for bookkeeping. * When successful, we transfer ownership of the `MemoryMappedFile` from the `List` of `MemoryMappedViewStream` to the `viewStreams` collection. * When unsuccessful, we'd just dispose of the `MemoryMappedViewStream`. * If `ReadWrite=true`, we can just use `File.OpenRead()` for symbols, versus `MemoryMappedViewStream`. Other changes: * Added tests to verify we can load a `.dll` and its symbols with appropriate settings. * Stop looking for `.mdb` files. We no longer support these in .NET 6+. * Removed unnecessary `$""` string interpolation. --- .../DirectoryAssemblyResolver.cs | 70 ++++++++++++++----- .../DirectoryAssemblyResolverTests.cs | 53 ++++++++++++++ 2 files changed, 106 insertions(+), 17 deletions(-) create mode 100644 tests/Java.Interop.Tools.JavaCallableWrappers-Tests/Java.Interop.Tools.JavaCallableWrappers/DirectoryAssemblyResolverTests.cs diff --git a/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs b/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs index 67a90ad89..12fc7dc29 100644 --- a/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs +++ b/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs @@ -39,6 +39,7 @@ using Java.Interop.Tools.Diagnostics; using Mono.Cecil; +using Mono.Cecil.Cil; namespace Java.Interop.Tools.Cecil { @@ -149,9 +150,6 @@ public bool AddToCache (AssemblyDefinition assembly) protected virtual AssemblyDefinition ReadAssembly (string file) { - bool haveDebugSymbols = loadDebugSymbols && - (File.Exists (Path.ChangeExtension (file, ".pdb")) || - File.Exists (file + ".mdb")); var reader_parameters = new ReaderParameters () { ApplyWindowsRuntimeProjections = loadReaderParameters.ApplyWindowsRuntimeProjections, AssemblyResolver = this, @@ -159,11 +157,9 @@ protected virtual AssemblyDefinition ReadAssembly (string file) InMemory = loadReaderParameters.InMemory, MetadataResolver = loadReaderParameters.MetadataResolver, ReadingMode = loadReaderParameters.ReadingMode, - ReadSymbols = haveDebugSymbols, ReadWrite = loadReaderParameters.ReadWrite, ReflectionImporterProvider = loadReaderParameters.ReflectionImporterProvider, SymbolReaderProvider = loadReaderParameters.SymbolReaderProvider, - SymbolStream = loadReaderParameters.SymbolStream, }; try { return LoadFromMemoryMappedFile (file, reader_parameters); @@ -171,9 +167,11 @@ protected virtual AssemblyDefinition ReadAssembly (string file) logger ( TraceLevel.Verbose, $"Failed to read '{file}' with debugging symbols. Retrying to load it without it. Error details are logged below."); - logger (TraceLevel.Verbose, $"{ex.ToString ()}"); + logger (TraceLevel.Verbose, ex.ToString ()); reader_parameters.ReadSymbols = false; return LoadFromMemoryMappedFile (file, reader_parameters); + } finally { + reader_parameters.SymbolStream?.Dispose (); } } @@ -181,29 +179,67 @@ AssemblyDefinition LoadFromMemoryMappedFile (string file, ReaderParameters optio { // We can't use MemoryMappedFile when ReadWrite is true if (options.ReadWrite) { + if (loadDebugSymbols) { + LoadSymbols (file, options, File.OpenRead); + } return AssemblyDefinition.ReadAssembly (file, options); } - MemoryMappedViewStream? viewStream = null; + // Likely 6 entries when symbols exist + var disposables = new List (capacity: 6); try { - // Create stream because CreateFromFile(string, ...) uses FileShare.None which is too strict - using var fileStream = new FileStream (file, FileMode.Open, FileAccess.Read, FileShare.Read, 4096, false); - using var mappedFile = MemoryMappedFile.CreateFromFile ( - fileStream, null, fileStream.Length, MemoryMappedFileAccess.Read, HandleInheritability.None, true); - viewStream = mappedFile.CreateViewStream (0, 0, MemoryMappedFileAccess.Read); + if (loadDebugSymbols) { + LoadSymbols (file, options, f => OpenMemoryMappedViewStream (f, disposables)); + } + var viewStream = OpenMemoryMappedViewStream (file, disposables); AssemblyDefinition result = ModuleDefinition.ReadModule (viewStream, options).Assembly; - viewStreams.Add (viewStream); - - // We transferred the ownership of the viewStream to the collection. - viewStream = null; + // Transfer ownership to `viewStreams` collection + viewStreams.Add (viewStream); + disposables.Remove (viewStream); + if (options.SymbolStream is MemoryMappedViewStream m) { + viewStreams.Add (m); + disposables.Remove (m); + options.SymbolStream = null; // Prevents caller from disposing + } return result; } finally { - viewStream?.Dispose (); + for (int i = disposables.Count - 1; i >= 0; i--) { + disposables [i].Dispose (); + } } } + static void LoadSymbols (string assemblyPath, ReaderParameters options, Func getStream) + { + var symbolStream = options.SymbolStream; + if (symbolStream == null) { + var symbolPath = Path.ChangeExtension (assemblyPath, ".pdb"); + if (File.Exists (symbolPath)) { + symbolStream = getStream (symbolPath); + } + } + options.ReadSymbols = symbolStream != null; + options.SymbolStream = symbolStream; + } + + static MemoryMappedViewStream OpenMemoryMappedViewStream (string file, List disposables) + { + // Create stream because CreateFromFile(string, ...) uses FileShare.None which is too strict + var fileStream = new FileStream (file, FileMode.Open, FileAccess.Read, FileShare.Read, 4096, useAsync: false); + disposables.Add (fileStream); + + var mappedFile = MemoryMappedFile.CreateFromFile ( + fileStream, null, fileStream.Length, MemoryMappedFileAccess.Read, HandleInheritability.None, leaveOpen: true); + disposables.Add (mappedFile); + + var viewStream = mappedFile.CreateViewStream (0, 0, MemoryMappedFileAccess.Read); + disposables.Add (viewStream); + + return viewStream; + } + public AssemblyDefinition GetAssembly (string fileName) { return Resolve (Path.GetFileNameWithoutExtension (fileName)); diff --git a/tests/Java.Interop.Tools.JavaCallableWrappers-Tests/Java.Interop.Tools.JavaCallableWrappers/DirectoryAssemblyResolverTests.cs b/tests/Java.Interop.Tools.JavaCallableWrappers-Tests/Java.Interop.Tools.JavaCallableWrappers/DirectoryAssemblyResolverTests.cs new file mode 100644 index 000000000..7053476d7 --- /dev/null +++ b/tests/Java.Interop.Tools.JavaCallableWrappers-Tests/Java.Interop.Tools.JavaCallableWrappers/DirectoryAssemblyResolverTests.cs @@ -0,0 +1,53 @@ +using System.Diagnostics; +using System.IO; +using Java.Interop.Tools.Cecil; +using Mono.Cecil; +using NUnit.Framework; + +namespace Java.Interop.Tools.JavaCallableWrappersTests +{ + [TestFixture] + public class DirectoryAssemblyResolverTests + { + static void Log (TraceLevel level, string message) + { + TestContext.Out.WriteLine ($"{level}: {message}"); + + if (level == TraceLevel.Error) + Assert.Fail (message); + } + + static string assembly_path; + static string symbol_path; + + [OneTimeSetUp] + public static void SetUp() + { + var assembly = typeof (DirectoryAssemblyResolverTests).Assembly; + assembly_path = Path.Combine (Path.GetTempPath (), Path.GetFileName (assembly.Location)); + symbol_path = Path.ChangeExtension (assembly_path, ".pdb"); + + File.Copy (assembly.Location, assembly_path, overwrite: true); + File.Copy (Path.ChangeExtension (assembly.Location, ".pdb"), symbol_path, overwrite: true); + } + + [OneTimeTearDown] + public static void TearDown () + { + File.Delete (assembly_path); + File.Delete (symbol_path); + } + + [Test] + public void LoadSymbols ([Values (true, false)] bool loadDebugSymbols, [Values (true, false)] bool readWrite) + { + using var resolver = new DirectoryAssemblyResolver (Log, loadDebugSymbols: loadDebugSymbols, new ReaderParameters { + ReadWrite = readWrite + }); + + var assembly = resolver.Load (assembly_path); + Assert.IsNotNull (assembly); + Assert.AreEqual (loadDebugSymbols, assembly.MainModule.HasSymbols); + } + } +} From 831b25535a793dd913ee6187fe3170cd9cec3eca Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Tue, 6 Feb 2024 14:46:27 -0600 Subject: [PATCH 2/2] Add `OpenMemoryMappedViewStream_disposables_Add_calls` --- .../DirectoryAssemblyResolver.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs b/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs index 12fc7dc29..aeaaa6113 100644 --- a/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs +++ b/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs @@ -185,8 +185,9 @@ AssemblyDefinition LoadFromMemoryMappedFile (string file, ReaderParameters optio return AssemblyDefinition.ReadAssembly (file, options); } - // Likely 6 entries when symbols exist - var disposables = new List (capacity: 6); + // We know the capacity for disposables + var disposables = new List ( + (1 + (loadDebugSymbols ? 1 : 0)) * OpenMemoryMappedViewStream_disposables_Add_calls); try { if (loadDebugSymbols) { LoadSymbols (file, options, f => OpenMemoryMappedViewStream (f, disposables)); @@ -224,6 +225,11 @@ static void LoadSymbols (string assemblyPath, ReaderParameters options, Func + /// Number of times OpenMemoryMappedViewStream() calls disposables.Add() + /// + const int OpenMemoryMappedViewStream_disposables_Add_calls = 3; + static MemoryMappedViewStream OpenMemoryMappedViewStream (string file, List disposables) { // Create stream because CreateFromFile(string, ...) uses FileShare.None which is too strict