Move RewriteMarshalMethods to the inner (per-RID) build#11066
Move RewriteMarshalMethods to the inner (per-RID) build#11066
Conversation
Move the RewriteMarshalMethods MSBuild task from the outer build (inside _GenerateJavaStubs) to a new _RewriteMarshalMethods target in the inner build, running after _PostTrimmingPipeline but before CreateReadyToRunImages and IlcCompile. This enables marshal methods to work with CoreCLR R2R builds, where Cecil cannot write mixed-mode assemblies produced by R2R. The task is now fully self-contained: it derives the target architecture from RuntimeIdentifier, creates its own XAAssemblyResolver, scans and classifies marshal methods via MarshalMethodsCollection.FromAssemblies, and rewrites assemblies in a single pass — no longer depending on in-process NativeCodeGenState from BuildEngine4. The outer build's GenerateJavaStubs re-classifies the already-rewritten assemblies, detecting _mm_wrapper methods and producing ConvertedMarshalMethodEntry objects. MarshalMethodCecilAdapter now merges ConvertedMarshalMethods into the object model so downstream native code generation (GenerateNativeMarshalMethodSources) produces correct callback tables. ConvertedMarshalMethodEntry sets NativeCallbackWrapper so the base NativeCallback property returns the wrapper method's metadata.
There was a problem hiding this comment.
Pull request overview
Moves marshal method rewriting to the inner (per-RID) publish build so rewritten assemblies are produced before R2R/crossgen2 and NativeAOT, and updates the marshal-method object model to correctly represent already-rewritten (*_mm_wrapper) methods when the outer build re-scans assemblies.
Changes:
- Update
ConvertedMarshalMethodEntrysoNativeCallbackresolves to the rewritten wrapper method. - Extend
MarshalMethodCecilAdapterto includeConvertedMarshalMethodsin the state consumed by downstream native codegen. - Refactor
RewriteMarshalMethodsinto a self-contained inner-build task and wire it via a new_RewriteMarshalMethodstarget after_PostTrimmingPipeline.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsClassifier.cs | Adjusts converted entry behavior so NativeCallback points at the wrapper method. |
| src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodCecilAdapter.cs | Merges ConvertedMarshalMethods into the marshal-method state passed to generators. |
| src/Xamarin.Android.Build.Tasks/Tasks/RewriteMarshalMethods.cs | Refactors task to scan/classify/rewrite assemblies in the inner per-RID build using RuntimeIdentifier + ResolvedFileToPublish. |
| src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.LlvmIr.targets | Removes outer-build invocation and adds _RewriteMarshalMethods inner-build target between trimming and R2R/NativeAOT. |
| // Handle the ordering dependency between special case methods and managed lookup tables | ||
| ManagedMarshalMethodsLookupInfo? managedLookupInfo = null; | ||
| if (!EnableManagedMarshalMethodsLookup) { | ||
| // Standard path: rewrite first, then add special cases | ||
| RewriteAssemblies (arch, classifier, resolver, managedLookupInfo, brokenExceptionTransitionsEnabled); | ||
| classifier.AddSpecialCaseMethods (); | ||
| } else { | ||
| // Managed lookup path: add special cases first so they appear in lookup tables | ||
| // We need to run `AddSpecialCaseMethods` before `RewriteMarshalMethods` so that we can see the special case | ||
| // methods (such as TypeManager.n_Activate_mm) when generating the managed lookup tables. | ||
| classifier.AddSpecialCaseMethods (); | ||
| managedLookupInfo = new ManagedMarshalMethodsLookupInfo (Log); | ||
| RewriteAssemblies (arch, classifier, resolver, managedLookupInfo, brokenExceptionTransitionsEnabled); | ||
| } |
There was a problem hiding this comment.
EnableManagedMarshalMethodsLookup currently builds a ManagedMarshalMethodsLookupInfo instance (managedLookupInfo) but it’s not persisted anywhere after rewriting. Downstream native codegen relies on NativeCodeGenState.ManagedMarshalMethodsLookupInfo to populate AssemblyIndex/ClassIndex/MethodIndex (and will throw if missing), so managed lookup builds can regress. Consider emitting the lookup info (or the computed indexes) as a task output/file and rehydrating it where NativeCodeGenStateObject is created (or recomputing it from the rewritten assemblies).
| // Also include methods that were already rewritten in the inner build. | ||
| // After the inner build rewrites assemblies, the outer build's GenerateJavaStubs | ||
| // re-classifies them and puts already-rewritten methods (with _mm_wrapper + | ||
| // [UnmanagedCallersOnly]) into ConvertedMarshalMethods instead of MarshalMethods. | ||
| // We must include these so that GenerateNativeMarshalMethodSources produces | ||
| // correct native callback tables. | ||
| foreach (var group in state.Classifier.ConvertedMarshalMethods) { | ||
| if (!obj.MarshalMethods.TryGetValue (group.Key, out var methods)) { | ||
| methods = new List<MarshalMethodEntryObject> (group.Value.Count); | ||
| obj.MarshalMethods.Add (group.Key, methods); | ||
| } | ||
|
|
||
| foreach (var method in group.Value) { | ||
| var entry = CreateEntry (method, state.ManagedMarshalMethodsLookupInfo); | ||
| methods.Add (entry); | ||
| } | ||
| } |
There was a problem hiding this comment.
ConvertedMarshalMethods entries are now merged into obj.MarshalMethods, but CreateEntry will only populate AssemblyIndex/ClassIndex/MethodIndex when state.ManagedMarshalMethodsLookupInfo is non-null. With RewriteMarshalMethods moved out of the outer-build state object, this lookup info is likely to be null, which will break managed lookup native codegen (MarshalMethodsNativeAssemblyGenerator throws when indexes are missing). Consider rebuilding ManagedMarshalMethodsLookupInfo from ConvertedMarshalMethods here (or ensuring it is propagated into state before adaptation).
src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsClassifier.cs
Outdated
Show resolved
Hide resolved
…per instead of original Remove NativeCallbackWrapper assignment from ConvertedMarshalMethodEntry so NativeCallback preserves its design contract of returning the original method. Instead, MarshalMethodCecilAdapter.CreateEntry() now explicitly checks for ConvertedMarshalMethodEntry and uses ConvertedNativeCallback for native callback tables, which need the wrapper's MetadataToken and [UnmanagedCallersOnly] attribute.
Move the RewriteMarshalMethods MSBuild task from the outer build (inside _GenerateJavaStubs) to a new _RewriteMarshalMethods target in the inner build, running after _PostTrimmingPipeline but before CreateReadyToRunImages and IlcCompile. This enables marshal methods to work with CoreCLR R2R builds, where Cecil cannot write mixed-mode assemblies produced by R2R.
The task is now fully self-contained: it derives the target architecture from RuntimeIdentifier, creates its own XAAssemblyResolver, scans and classifies marshal methods via MarshalMethodsCollection.FromAssemblies, and rewrites assemblies in a single pass — no longer depending on in-process NativeCodeGenState from BuildEngine4.
The outer build's GenerateJavaStubs re-classifies the already-rewritten assemblies, detecting _mm_wrapper methods and producing ConvertedMarshalMethodEntry objects. MarshalMethodCecilAdapter now merges ConvertedMarshalMethods into the object model so downstream native code generation (GenerateNativeMarshalMethodSources) produces correct callback tables. ConvertedMarshalMethodEntry sets NativeCallbackWrapper so the base NativeCallback property returns the wrapper method's metadata.