-
Notifications
You must be signed in to change notification settings - Fork 544
[XABT] Move JLO scanning needed for typemap generation to a linker step. #10015
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.
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
base: main
Are you sure you want to change the base?
Conversation
e187164
to
4a00982
Compare
A potential issue here is with cross-assembly duplicate detection. Today, we are scanning all JLO derived types from all assemblies in a single loop, so duplicate detection logic like the following finds duplicates even if they are in different assemblies: android/src/Xamarin.Android.Build.Tasks/Utilities/TypeMapCecilAdapter.cs Lines 141 to 152 in 8c74126
Because the new process only scans a single assembly at a time with Cecil, it will find duplicates in the same assembly, but not duplicates that may exist in other assemblies. An example is
It looks like we have 2 ordering semantics we have to preserve in replicating this logic:
Aside from these 2 cases, which type is considered the "primary" and which type(s) are considered the "duplicates" seems to be luck of the draw. |
This is correct and incomplete. If you're only going by However, most codepaths are not hitting Thus, even if there are duplicate types (because bindings for a |
9b84897
to
faaad5a
Compare
0d66bf2
to
f0dea44
Compare
8650e83
to
f8100ea
Compare
f8100ea
to
eae6b50
Compare
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.
Is the new trimmer step not setup for Release mode? I thought it would be listed here:
The PR description mentions it won't work in combination with $(AndroidEnableMarshalMethods)=true
, but would it work for a Release
build with marshal methods off?
Yes, it runs as part of the It works for Release mode when |
|
||
public TaskLoggingHelper Log { get; set; } | ||
|
||
public FindTypeMapObjectsStep (TaskLoggingHelper log) => Log = log; |
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.
As a "ha ha only serious" / "future coding guidelines" question/discussion: should we start using C# primary constructors?
public partial class FindTypeMapObjectStep (TaskLoggingHelper Log) : BaseStep, IAssemblyModifierPipelineStep {
// …
}
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.
I don't love primary constructors, but if that's what we decide we want to standardize as a team it won't kill me. 😁
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.
I'm not a fan of primary constructors either... :)
}; | ||
|
||
if (Debug) { | ||
var (javaToManaged, managedToJava) = TypeMapCecilAdapter.GetDebugNativeEntries (types, Context, out var foundJniNativeRegistration); |
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.
If we have a tuple return type, why not have out var foundJniNativeRegistration
be a separate tuple value?
xml.ManagedToJavaDebugEntries.AddRange (managedToJava); | ||
xml.FoundJniNativeRegistration = foundJniNativeRegistration; | ||
|
||
if (!xml.HasDebugEntries) { |
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.
I wonder if this block and the "equivalent" block in the else
branch should be moved to the end…
return; | ||
} | ||
} | ||
|
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.
…to here, as:
if (!xml.HasDebugEntries && xml.ModuleReleaseData == null) {
Log.LogDebugMessage ("No Java types found…");
TypeMapObjectsXmlFile.WriteEmptyFile (destinationTypeMapXml, Log)
return;
}
|
||
public bool WasScanned { get; private set; } | ||
|
||
public void Export (string filename) |
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.
Should this method be responsible for checking for !this.HasDebugEntries && this.ModuleReleaseData == null
and writing an empty file if there are no entries to write? This could simplify the call-sites.
Additionally, if we add a Log
property to this class, the No Java types found
/Wrote …
message could also be moved here:
public void Export (string filename, string assemblyName)
{
if (!HasDebugEntries && ModuleReleaseData == null) {
Log.LogDebugMessage ($"No Java types found in '{assemblyName}'");
TypeMapObjectsXmlFile.WriteEmptyFile (filename, Log)
return;
}
Log.LogDebugMessage ($"Wrote '{filename}', {JavaToManagedDebugEntries.Count} JavaToManagedDebugEntries, {ManagedToJavaDebugEntries.Count} ManagedToJavaDebugEntries, FoundJniNativeRegistration: {FoundJniNativeRegistration}")
// …
}
Addressed feedback. |
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.
Pull Request Overview
This PR refactors the typemap generation process by moving the JLO scanning to a new linker step and updating the overall assembly pipeline to better support incremental builds. Key changes include:
- Introducing a new FindTypeMapObjectsStep that writes typemap XML files beside source assemblies.
- Refactoring TypeMapGenerator and TypeMapCecilAdapter to use new adapter interfaces for handling debug and release typemap generation.
- Updating the AssemblyModifierPipeline to use void-based ProcessAssembly methods and adding a SaveChangedAssemblyStep.
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/Xamarin.Android.Build.Tasks/Utilities/TypeMapObjectsXmlFile.cs | Introduces XML export logic for typemap objects with support for both debug and release entries. |
src/Xamarin.Android.Build.Tasks/Utilities/TypeMapGenerator.cs | Refactors type mapping generation using new adapter abstractions and integrates the RunCheckedBuild flag. |
src/Xamarin.Android.Build.Tasks/Utilities/TypeMapCecilAdapter.cs | Updates debug and release entry generation and duplicate handling logic. |
src/Xamarin.Android.Build.Tasks/Tasks/GenerateTypeMappings.cs | Updates the task to generate typemaps from both managed and native states, with changes for safe file output accumulation. |
src/Xamarin.Android.Build.Tasks/Tasks/AssemblyModifierPipeline.cs | Modifies pipeline steps to use void-based ProcessAssembly and introduces a new step to save changed assemblies. |
src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FindTypeMapObjectsStep.cs | Adds a new linker step that scans for Java types needed for typemap generation and writes them to an XML file. |
Files not reviewed (2)
- src/Xamarin.Android.Build.Tasks/Xamarin.Android.Build.Tasks.csproj: Language not supported
- src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets: Language not supported
src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FindTypeMapObjectsStep.cs
Show resolved
Hide resolved
b5a211d
to
eee3b1c
Compare
Context: #9893
Context: #9930
This commit moves the process of scanning for JLOs needed for the typemap generation task to a new
FindTypeMapObjectsStep
"linker step".The types are serialized to new
<assembly>.typemap.xml
files that sit beside the source assembly. A new file was used rather than adding more sections to the.jlo.xml
file because this file contains a lot more types and will likely change more often on incremental builds, and we don't want to trigger ie: creating new Java stubs when it isn't needed.This file is then read in the existing
<GenerateTypeMappings>
task to generate the final typemap file.The existing method of JLO scanning for typemaps is still used when using LLVM marshal methods, as the marshal method rewriter runs after the linker steps, and when it changes the assemblies the MVID and type tokens no longer match the values saved in the
.typemap.xml
files.Like #9893, this temporarily leaves the old typemap generation code in place, guarded behind the
$(_AndroidJLOCheckedBuild)
flag. This flag generates the typemap both the new and old way, and errors the build if there are differences.Note that #10024 updated our "pipeline" to save assemblies after all steps have been run. However, looking for JLOs needed for typemap generation has to be run after assemblies are saved. This is because Release typemaps rely on assembly MVID and type tokens which change when the assemblies are modified.
Update the pipeline's "modified assembly saving" logic to itself be a pipeline step, so we can insert it before the new
FindTypeMapObjectsStep
step.