[Java.Interop.Tools.Cecil] DirectoryAssemblyResolver symbol loads (#1176)

Context: 7d42864
Context: dotnet/android#8571

While debugging dotnet/android#8571, I found that the usage
of `MemoryMappedFile` (from 7d42864) 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:

	var options = new ReaderParameters {
	    // …
	};
	AssemblyDefinition result = ModuleDefinition.ReadModule(viewStream, options)
	    .Assembly;

You would need the following in `ReaderParameters options`:

  * `options.ReadSymbols=true`

  * `options.SymbolStream` containing a valid `Stream` to the
    `.pdb` file

To make this work, I had to:

  * Create a `List<IDisposable>` for bookkeeping.

  * When successful, we transfer ownership of the `MemoryMappedFile`
    from the `List<IDisposable>` of `MemoryMappedViewStream` to the
    `viewStreams` collection.

  * When unsuccessful, we'd just dispose of the
    `MemoryMappedViewStream`.

  * If `options.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.

Author: jonathanpeppers

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,59 +150,100 @@ 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,
 				MetadataImporterProvider        = loadReaderParameters.MetadataImporterProvider,
 				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);
 			} catch (Exception ex) {
 				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 ();
 			}
 		}
 
 		AssemblyDefinition LoadFromMemoryMappedFile (string file, ReaderParameters options)
 		{
 			// 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;
+			// We know the capacity for disposables
+			var disposables = new List<IDisposable> (
+				(1 + (loadDebugSymbols ? 1 : 0)) * OpenMemoryMappedViewStream_disposables_Add_calls);
 			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<string, Stream> 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;
+		}
+
+		/// <summary>
+		/// Number of times OpenMemoryMappedViewStream() calls disposables.Add()
+		/// </summary>
+		const int OpenMemoryMappedViewStream_disposables_Add_calls = 3;
+
+		static MemoryMappedViewStream OpenMemoryMappedViewStream (string file, List<IDisposable> 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)

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);
+		}
+	}
+}