[Java.Interop.Tools.Cecil] fix symbol loading in DirectoryAssemblyResolver

Context: 7d42864d
Context: dotnet/android#8571

While debugging dotnet/android#8571, I found 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:

    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<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 `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,61 +150,96 @@ 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;
+			// Likely 6 entries when symbols exist
+			var disposables = new List<IDisposable> (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<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;
+		}
+
+		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)
 		{
 			return Resolve (Path.GetFileNameWithoutExtension (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);
+		}
+	}
+}