[marshal methods] Part 2 of ? #7123
There are no files selected for viewing
Submodule Java.Interop
updated
from c942ab to fadbb8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| #if ENABLE_MARSHAL_METHODS | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.IO; | ||
|
|
||
| using Java.Interop.Tools.Cecil; | ||
| using Microsoft.Android.Build.Tasks; | ||
| using Microsoft.Build.Utilities; | ||
| using Mono.Cecil; | ||
| using Xamarin.Android.Tools; | ||
|
|
||
| namespace Xamarin.Android.Tasks | ||
| { | ||
| class MarshalMethodsAssemblyRewriter | ||
| { | ||
| IDictionary<string, IList<MarshalMethodEntry>> methods; | ||
| ICollection<AssemblyDefinition> uniqueAssemblies; | ||
| IDictionary <string, HashSet<string>> assemblyPaths; | ||
| TaskLoggingHelper log; | ||
|
|
||
| public MarshalMethodsAssemblyRewriter (IDictionary<string, IList<MarshalMethodEntry>> methods, ICollection<AssemblyDefinition> uniqueAssemblies, IDictionary <string, HashSet<string>> assemblyPaths, TaskLoggingHelper log) | ||
| { | ||
| this.methods = methods ?? throw new ArgumentNullException (nameof (methods)); | ||
| this.uniqueAssemblies = uniqueAssemblies ?? throw new ArgumentNullException (nameof (uniqueAssemblies)); | ||
| this.assemblyPaths = assemblyPaths ?? throw new ArgumentNullException (nameof (assemblyPaths)); | ||
| this.log = log ?? throw new ArgumentNullException (nameof (log)); | ||
| } | ||
|
|
||
| public void Rewrite (DirectoryAssemblyResolver resolver) | ||
| { | ||
| MethodDefinition unmanagedCallersOnlyAttributeCtor = GetUnmanagedCallersOnlyAttributeConstructor (resolver); | ||
| var unmanagedCallersOnlyAttributes = new Dictionary<AssemblyDefinition, CustomAttribute> (); | ||
| foreach (AssemblyDefinition asm in uniqueAssemblies) { | ||
| unmanagedCallersOnlyAttributes.Add (asm, CreateImportedUnmanagedCallersOnlyAttribute (asm, unmanagedCallersOnlyAttributeCtor)); | ||
| } | ||
|
|
||
| Console.WriteLine ("Adding the [UnmanagedCallersOnly] attribute to native callback methods and removing unneeded fields+methods"); | ||
| foreach (IList<MarshalMethodEntry> methodList in methods.Values) { | ||
| foreach (MarshalMethodEntry method in methodList) { | ||
| Console.WriteLine ($"\t{method.NativeCallback.FullName} (token: 0x{method.NativeCallback.MetadataToken.RID:x})"); | ||
| method.NativeCallback.CustomAttributes.Add (unmanagedCallersOnlyAttributes [method.NativeCallback.Module.Assembly]); | ||
| method.Connector.DeclaringType.Methods.Remove (method.Connector); | ||
| method.CallbackField?.DeclaringType.Fields.Remove (method.CallbackField); | ||
| } | ||
| } | ||
|
|
||
| Console.WriteLine (); | ||
|
Contributor
There was a problem hiding this comment. I think we'll be wanting
Contributor
Author
There was a problem hiding this comment. The CWLs will be gone eventually, I will remove them in the last PR in the series |
||
| Console.WriteLine ("Rewriting assemblies"); | ||
|
|
||
| var newAssemblyPaths = new List<string> (); | ||
| foreach (AssemblyDefinition asm in uniqueAssemblies) { | ||
| foreach (string path in GetAssemblyPaths (asm)) { | ||
| var writerParams = new WriterParameters { | ||
| WriteSymbols = (File.Exists (path + ".mdb") || File.Exists (Path.ChangeExtension (path, ".pdb"))), | ||
| }; | ||
|
|
||
| string output = $"{path}.new"; | ||
| Console.WriteLine ($"\t{asm.Name} => {output}"); | ||
| asm.Write (output, writerParams); | ||
| newAssemblyPaths.Add (output); | ||
| } | ||
| } | ||
|
|
||
| // Replace old versions of the assemblies only after we've finished rewriting without issues, otherwise leave the new | ||
| // versions around. | ||
| foreach (string path in newAssemblyPaths) { | ||
| string target = Path.Combine (Path.GetDirectoryName (path), Path.GetFileNameWithoutExtension (path)); | ||
| MoveFile (path, target); | ||
|
Contributor
There was a problem hiding this comment. We may want/need to rework the build system so that we write files into new/separate directories instead of modifying files in-place, otherwise we'll provoke the incremental build + file sharing gods.
Contributor
Author
There was a problem hiding this comment. I asked @jonathanpeppers about this and he thought it was better to write them in place. We can optimize this in the last PR in the series though |
||
|
|
||
| string source = Path.ChangeExtension (path, ".pdb"); | ||
| if (File.Exists (source)) { | ||
| target = Path.ChangeExtension (Path.Combine (Path.GetDirectoryName (source), Path.GetFileNameWithoutExtension (source)), ".pdb"); | ||
|
|
||
| MoveFile (source, target); | ||
| } | ||
|
|
||
| source = $"{path}.mdb"; | ||
| if (File.Exists (source)) { | ||
| target = Path.ChangeExtension (path, ".mdb"); | ||
| MoveFile (source, target); | ||
| } | ||
| } | ||
|
|
||
| Console.WriteLine (); | ||
| Console.WriteLine ("Method tokens:"); | ||
| foreach (IList<MarshalMethodEntry> methodList in methods.Values) { | ||
| foreach (MarshalMethodEntry method in methodList) { | ||
| Console.WriteLine ($"\t{method.NativeCallback.FullName} (token: 0x{method.NativeCallback.MetadataToken.RID:x})"); | ||
| } | ||
| } | ||
|
|
||
| void MoveFile (string source, string target) | ||
| { | ||
| Console.WriteLine ($"Moving '{source}' => '{target}'"); | ||
dellis1972 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Files.CopyIfChanged (source, target); | ||
| try { | ||
| File.Delete (source); | ||
| } catch (Exception ex) { | ||
| log.LogWarning ($"Unable to delete source file '{source}' when moving it to '{target}'"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ICollection<string> GetAssemblyPaths (AssemblyDefinition asm) | ||
| { | ||
| if (!assemblyPaths.TryGetValue (asm.Name.Name, out HashSet<string> paths)) { | ||
| throw new InvalidOperationException ($"Unable to determine file path for assembly '{asm.Name.Name}'"); | ||
| } | ||
|
|
||
| return paths; | ||
| } | ||
|
|
||
| MethodDefinition GetUnmanagedCallersOnlyAttributeConstructor (DirectoryAssemblyResolver resolver) | ||
| { | ||
| AssemblyDefinition asm = resolver.Resolve ("System.Runtime.InteropServices"); | ||
| TypeDefinition unmanagedCallersOnlyAttribute = null; | ||
| foreach (ModuleDefinition md in asm.Modules) { | ||
| foreach (ExportedType et in md.ExportedTypes) { | ||
| if (!et.IsForwarder) { | ||
| continue; | ||
| } | ||
|
|
||
| if (String.Compare ("System.Runtime.InteropServices.UnmanagedCallersOnlyAttribute", et.FullName, StringComparison.Ordinal) != 0) { | ||
| continue; | ||
| } | ||
|
|
||
| unmanagedCallersOnlyAttribute = et.Resolve (); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (unmanagedCallersOnlyAttribute == null) { | ||
| throw new InvalidOperationException ("Unable to find the System.Runtime.InteropServices.UnmanagedCallersOnlyAttribute type"); | ||
| } | ||
|
|
||
| foreach (MethodDefinition md in unmanagedCallersOnlyAttribute.Methods) { | ||
| if (!md.IsConstructor) { | ||
| continue; | ||
| } | ||
|
|
||
| return md; | ||
| } | ||
|
|
||
| throw new InvalidOperationException ("Unable to find the System.Runtime.InteropServices.UnmanagedCallersOnlyAttribute type constructor"); | ||
| } | ||
|
|
||
| CustomAttribute CreateImportedUnmanagedCallersOnlyAttribute (AssemblyDefinition targetAssembly, MethodDefinition unmanagedCallersOnlyAtributeCtor) | ||
| { | ||
| return new CustomAttribute (targetAssembly.MainModule.ImportReference (unmanagedCallersOnlyAtributeCtor)); | ||
| } | ||
| } | ||
| } | ||
| #endif | ||
|
Contributor
There was a problem hiding this comment. At this point I suspect I'm the only one who remembers this convention, but please place a comment on #endif // ENABLE_MARSHAL_METHODS
Contributor
Author
There was a problem hiding this comment. I apply the convention when it's there to stay :) This macro is going to go away in the last PR of the series, it's just a measure to not "poison" current code until marshal methods are finished |
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not skip these
if (!Debug) StoreMarshalAssemblyPath()checks and just callStoreMarshalAssemblyPath())?StoreMarshalAssemblyPath()checksDebuganyway, so this is just adding extra levels of complexity & indentation, for no observable benefit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the first case we save one string allocation, in the second instance yes, it's not necessary