Skip to content
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

Run FixAbstractMethodsStep before MarkStep #9207

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+71 −24
Draft

Run FixAbstractMethodsStep before MarkStep #9207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d668de1
Run FixAbstractMethodsStep before MarkStep
sbomer Aug 15, 2024
bc4ac82
Fix build
sbomer Aug 16, 2024
bee5851
Fix cache, remove ctor for ILLINK build
sbomer Aug 16, 2024
89904e8
Check nested types
sbomer Sep 6, 2024
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
93 changes: 70 additions & 23 deletions src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,30 +23,18 @@ namespace MonoDroid.Tuner
/// <summary>
/// NOTE: this step is subclassed so it can be called directly from Xamarin.Android.Build.Tasks
/// </summary>
public class FixAbstractMethodsStep :
#if ILLINK
BaseMarkHandler
#else // !ILLINK
BaseStep
#endif // !ILLINK
public class FixAbstractMethodsStep : BaseStep
{

#if ILLINK
public override void Initialize (LinkContext context, MarkContext markContext)
{
this.cache = context;
base.Initialize (context, markContext);
markContext.RegisterMarkTypeAction (type => ProcessType (type));
}
IMetadataResolver cache => Context;
#else // !ILLINK
readonly IMetadataResolver cache;

public FixAbstractMethodsStep (IMetadataResolver cache)
{
this.cache = cache;
}

readonly
#endif // !ILLINK
IMetadataResolver cache;

bool CheckShouldProcessAssembly (AssemblyDefinition assembly)
{
Expand Down Expand Up @@ -74,29 +62,78 @@ void UpdateAssemblyAction (AssemblyDefinition assembly)
}

#if ILLINK
readonly List<AssemblyDefinition> assemblies = new ();

protected override void ProcessAssembly (AssemblyDefinition assembly)
{
assemblies.Add (assembly);
}

protected override void EndProcess ()
{
foreach (var assembly in GetReferencedAssemblies().ToList()) {
ProcessAssembly_Actual(assembly);
}

IEnumerable<AssemblyDefinition> GetReferencedAssemblies ()
{
var loaded = new HashSet<AssemblyDefinition> (assemblies);
var toProcess = new Queue<AssemblyDefinition> (assemblies);

while (toProcess.Count > 0) {
var assembly = toProcess.Dequeue ();
foreach (var reference in ResolveReferences (assembly)) {
if (!loaded.Add (reference))
continue;
yield return reference;
toProcess.Enqueue (reference);
}
}
}

ICollection<AssemblyDefinition> ResolveReferences (AssemblyDefinition assembly)
{
List<AssemblyDefinition> references = new List<AssemblyDefinition> ();
if (assembly == null)
return references;

foreach (AssemblyNameReference reference in assembly.MainModule.AssemblyReferences) {
AssemblyDefinition? definition = Context.Resolve (reference);
if (definition != null)
references.Add (definition);
}

return references;
}
}

protected void ProcessType (TypeDefinition type)
{
var assembly = type.Module.Assembly;
if (!CheckShouldProcessAssembly (assembly))
return;

if (!MightNeedFix (type))
return;

if (!FixAbstractMethods (type))
return;

UpdateAssemblyAction (assembly);
MarkAbstractMethodErrorType ();
}
#else // !ILLINK
#endif // ILLINK

#if ILLINK
void ProcessAssembly_Actual (AssemblyDefinition assembly)
#else // !ILLINK
protected override void ProcessAssembly (AssemblyDefinition assembly)
#endif // !ILLINK
{
if (!CheckShouldProcessAssembly (assembly))
return;

if (FixAbstractMethods (assembly)) {
#if !ILLINK
Context.SafeReadSymbols (assembly);
#endif // !ILLINK
UpdateAssemblyAction (assembly);
MarkAbstractMethodErrorType ();
}
Expand All @@ -106,12 +143,19 @@ internal bool FixAbstractMethods (AssemblyDefinition assembly)
{
bool changed = false;
foreach (var type in assembly.MainModule.Types) {
if (MightNeedFix (type))
changed |= FixAbstractMethods (type);
changed |= FixAbstractMethodsNested (type);
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to recurse into the nested types of each type to test this properly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I measured again with a change that includes nested types and it didn't impact perf in an significant way compared to the overall change to run for all assemblies.

return changed;

bool FixAbstractMethodsNested (TypeDefinition type)
{
bool changed = FixAbstractMethods (type);
foreach (var nested in type.NestedTypes) {
changed |= FixAbstractMethodsNested (nested);
}
return changed;
}
}
#endif // !ILLINK

readonly HashSet<string> warnedAssemblies = new (StringComparer.Ordinal);

Expand Down Expand Up @@ -242,6 +286,9 @@ bool HaveSameSignature (TypeReference iface, MethodDefinition iMethod, MethodDef

bool FixAbstractMethods (TypeDefinition type)
{
if (!MightNeedFix (type))
return false;

if (!type.HasInterfaces)
return false;

Expand Down
2 changes: 1 addition & 1 deletion ...in.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.ILLink.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,14 @@ This file contains the .NET 5-specific targets to customize ILLink
<!-- add our custom steps -->
<!-- Custom steps that run before MarkStep -->
<_TrimmerCustomSteps Include="$(_AndroidLinkerCustomStepAssembly)" BeforeStep="MarkStep" Type="Microsoft.Android.Sdk.ILLink.SetupStep" />
<_TrimmerCustomSteps Include="$(_AndroidLinkerCustomStepAssembly)" BeforeStep="MarkStep" Type="MonoDroid.Tuner.FixAbstractMethodsStep" />
<!-- Custom MarkHandlers that run during MarkStep -->
<_TrimmerCustomSteps Include="$(_AndroidLinkerCustomStepAssembly)" Type="Microsoft.Android.Sdk.ILLink.PreserveSubStepDispatcher" />
<_TrimmerCustomSteps Include="$(_AndroidLinkerCustomStepAssembly)" Type="MonoDroid.Tuner.MarkJavaObjects" />
<_TrimmerCustomSteps Include="$(_AndroidLinkerCustomStepAssembly)" Type="MonoDroid.Tuner.PreserveJavaExceptions" />
<_TrimmerCustomSteps Include="$(_AndroidLinkerCustomStepAssembly)" Type="MonoDroid.Tuner.PreserveApplications" />
<_TrimmerCustomSteps Include="$(_AndroidLinkerCustomStepAssembly)" Type="Microsoft.Android.Sdk.ILLink.PreserveRegistrations" />
<_TrimmerCustomSteps Include="$(_AndroidLinkerCustomStepAssembly)" Type="Microsoft.Android.Sdk.ILLink.PreserveJavaInterfaces" />
<_TrimmerCustomSteps Include="$(_AndroidLinkerCustomStepAssembly)" Type="MonoDroid.Tuner.FixAbstractMethodsStep" />
<!-- Custom steps that run after MarkStep -->
<_TrimmerCustomSteps
Condition=" '$(_ProguardProjectConfiguration)' != '' "
Expand Down