Skip to content

[Java.Interop.Tools.Cecil] Fix DirectoryAssemblyResolver.Dispose() #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 26, 2016
Merged

[Java.Interop.Tools.Cecil] Fix DirectoryAssemblyResolver.Dispose() #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,20 @@ public class DirectoryAssemblyResolver : IAssemblyResolver {
bool loadDebugSymbols;
Action<string, object[]> logWarnings;

public DirectoryAssemblyResolver (Action<string, object[]> logWarnings, bool loadDebugSymbols)
ReaderParameters loadReaderParameters;

static readonly ReaderParameters DefaultLoadReaderParameters = new ReaderParameters {
};

public DirectoryAssemblyResolver (Action<string, object[]> logWarnings, bool loadDebugSymbols, ReaderParameters loadReaderParameters = null)
{
if (logWarnings == null)
throw new ArgumentNullException (nameof (logWarnings));
cache = new Dictionary<string, AssemblyDefinition> ();
this.loadDebugSymbols = loadDebugSymbols;
this.logWarnings = logWarnings;
SearchDirectories = new List<string> ();
this.loadReaderParameters = loadReaderParameters ?? DefaultLoadReaderParameters;
}

public void Dispose ()
Expand All @@ -82,8 +88,15 @@ public void Dispose ()

protected virtual void Dispose (bool disposing)
{
if (!disposing || cache == null)
return;
foreach (var e in cache) {
e.Value.Dispose ();
}
cache = null;
}

[Obsolete ("Should not be used; was required with previous Cecil versions.")]
public IDictionary ToResolverCache ()
{
var resolver_cache = new Hashtable ();
Expand Down Expand Up @@ -114,9 +127,21 @@ public virtual AssemblyDefinition Load (string fileName)

protected virtual AssemblyDefinition ReadAssembly (string file)
{
bool haveDebugSymbols = loadDebugSymbols &&
(File.Exists (file + ".mdb") ||
File.Exists (Path.ChangeExtension (file, ".pdb")));
var reader_parameters = new ReaderParameters () {
AssemblyResolver = this,
ReadSymbols = loadDebugSymbols && (File.Exists(file + ".mdb") || File.Exists(Path.ChangeExtension(file, ".pdb"))),
ApplyWindowsRuntimeProjections = loadReaderParameters.ApplyWindowsRuntimeProjections,
AssemblyResolver = this,
MetadataImporterProvider = loadReaderParameters.MetadataImporterProvider,
InMemory = loadReaderParameters.InMemory,
MetadataResolver = loadReaderParameters.MetadataResolver,
ReadingMode = loadReaderParameters.ReadingMode,
ReadSymbols = haveDebugSymbols,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to set to true and leave it to Cecil to decide which one to load

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can quickly tell, that won't work; it looks like Cecil will throw an exception if ReadSymbols is true and the .mdb file doesn't exist.

To verify:

$ csharp -r:Mono.Cecil.dll
Mono C# Shell, type "help;" for help

Enter statements below.
csharp> using Mono.Cecil;
csharp> var rp = new ReaderParameters { ReadSymbols = true };
csharp> var ad = AssemblyDefinition.ReadAssembly ("Mono.Cecil", rp);
System.IO.FileNotFoundException: Could not find file "/Users/jon/Dropbox/Developer/Java.Interop/bin/Debug/Mono.Cecil".
File name: '/Users/jon/Dropbox/Developer/Java.Interop/bin/Debug/Mono.Cecil'
  at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.Int32 bufferSize, System.Boolean anonymous, System.IO.FileOptions options) [0x0021a] in <94fd79a3b7144c54b4cb162b50fc7761>:0 
  at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00000] in <94fd79a3b7144c54b4cb162b50fc7761>:0 
  at (wrapper remoting-invoke-with-check) System.IO.FileStream:.ctor (string,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare)
  at Mono.Cecil.ModuleDefinition.GetFileStream (System.String fileName, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00006] in <c7a3ce423ee8414bbda752a60dd297fd>:0 
  at Mono.Cecil.ModuleDefinition.ReadModule (System.String fileName, Mono.Cecil.ReaderParameters parameters) [0x00008] in <c7a3ce423ee8414bbda752a60dd297fd>:0 
  at Mono.Cecil.AssemblyDefinition.ReadAssembly (System.String fileName, Mono.Cecil.ReaderParameters parameters) [0x00000] in <c7a3ce423ee8414bbda752a60dd297fd>:0 
  at <InteractiveExpressionClass>.Host (System.Object& $retval) [0x00000] in <1596036310d3473886b65733f6439422>:0 
  at Mono.CSharp.Evaluator.Evaluate (System.String input, System.Object& result, System.Boolean& result_set) [0x0003e] in <984452ea6c444474a99da68d7572368e>:0 
  at Mono.CSharpShell.Evaluate (System.String input) [0x00000] in <693eac6b8b7545318543ea23138aba75>:0

I suppose we could always set it to true anyway, given that we catch the exception, set reader_parameters.ReadSymbols = false, and try again, but that seems wasteful.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot produce pdb on mono without this being fixed. There is PR waiting for approval so any any cecil based tool works properly on mono.

See jbevain/cecil#293

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, but I don't understand the comment; what does the "this" refer to in "without this being fixed"?

My guess is that "this" refers to "ReaderParameters.ReadSymbols always set to true".

Assuming that's the case, I'm still somewhat dubious of that. The current expectation -- if annoying -- is that when ReaderParameters.ReadSymbols is true and the symbol file doesn't exist, a FileNotFoundException is thrown. Changing that semantic would likely annoy anybody who actually wants to ensure that symbol files exist.

There are thus two solutions I can think of:

  1. Always set ReaderParameters.ReadSymbols to true anyway, then catch the exception. (Just because I don't like it doesn't mean it's not the better solution.)
  2. Add a new ReaderParameters.ReadSymbolsWhenPossible property (or something), which would cause Cecil to try to read symbols if they exist, but not fail if they don't.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "this" I meant forced mdb debug symbols reader on Mono. Cecil is missing implementation of pdb/mdb reading logic and I don't know yet which option will jb prefer but ReadSymbolsWhenPossible will probably be the winner. Until this is implemented you can check for mdb file presence and set optionally based on that (although it's still wrong as there could be mdb vs dll mismatch it should cover most cases).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be revisited once cecil got relevant implementation.

ReadWrite = loadReaderParameters.ReadWrite,
ReflectionImporterProvider = loadReaderParameters.ReflectionImporterProvider,
SymbolReaderProvider = loadReaderParameters.SymbolReaderProvider,
SymbolStream = loadReaderParameters.SymbolStream,
};
try {
return AssemblyDefinition.ReadAssembly (file, reader_parameters);
Expand Down
12 changes: 7 additions & 5 deletions ...ools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,15 @@ public TypeNameMapGenerator (IEnumerable<string> assemblies, Action<string, obje

Log = logMessage;
var Assemblies = assemblies.ToList ();
var resolver = new DirectoryAssemblyResolver (Log, loadDebugSymbols: true);
var rp = new ReaderParameters (ReadingMode.Immediate);

foreach (var assembly in Assemblies) {
resolver.Load (Path.GetFullPath (assembly));
}
using (var resolver = new DirectoryAssemblyResolver (Log, loadDebugSymbols: true, loadReaderParameters: rp)) {
foreach (var assembly in Assemblies) {
resolver.Load (Path.GetFullPath (assembly));
}

Types = JavaTypeScanner.GetJavaTypes (Assemblies, resolver, logMessage);
Types = JavaTypeScanner.GetJavaTypes (Assemblies, resolver, logMessage);
}
}

public TypeNameMapGenerator (IEnumerable<TypeDefinition> types, Action<string, object[]> logMessage)
Expand Down
8 changes: 7 additions & 1 deletion tools/generator/CodeGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,13 @@ public static void Run (CodeGeneratorOptions options)
if (options == null)
throw new ArgumentNullException ("options");

using (var resolver = new DirectoryAssemblyResolver (Console.WriteLine, loadDebugSymbols: false)) {
Run (options, resolver);
}
}

static void Run (CodeGeneratorOptions options, DirectoryAssemblyResolver resolver)
{
string assemblyQN = options.AssemblyQualifiedName;
string api_level = options.ApiLevel;
int product_version = options.ProductVersion;
Expand Down Expand Up @@ -239,7 +246,6 @@ public static void Run (CodeGeneratorOptions options)

// Load reference libraries

var resolver = new DirectoryAssemblyResolver (Console.WriteLine, loadDebugSymbols: false);
foreach (var lib in options.LibraryPaths) {
resolver.SearchDirectories.Add (lib);
}
Expand Down
3 changes: 3 additions & 0 deletions tools/jcw-gen/App.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ public static int Main (string [] args)
Console.Error.Write ("jcw-gen: {0}", verbosity > 0 ? e.ToString () : e.Message);
return 1;
}
finally {
resolver.Dispose ();
}
}

static void GenerateJavaCallableWrapper (TypeDefinition type, string outputPath)
Expand Down