-
Notifications
You must be signed in to change notification settings - Fork 515
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
Update to new linker custom steps API #11374
Merged
Merged
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
6e0234b
Update to new linker custom steps API
sbomer ae665f7
PR feedback
sbomer 11733b9
Note override issue
sbomer 7362b74
Clean up comments
sbomer 975e045
Merge branch 'main' into trimmerCustomSteps
224c3d4
Move `DynamicRegistrationSupported` change earlier, along with the
f15bbc1
ILLink does not support considering other attributes as `[Preserve]`
1d9e716
Fix removal of the dynamic registrar on legacy
046e01b
Fix IntPtr size inlining
sbomer f48385d
Merge branch 'main' into trimmerCustomSteps
sbomer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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 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
18 changes: 18 additions & 0 deletions
18
tools/dotnet-linker/Steps/ConfigurationAwareMarkHandler.cs
This file contains 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,18 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
|
||
using Xamarin.Bundler; | ||
|
||
namespace Xamarin.Linker { | ||
public abstract class ConfigurationAwareMarkHandler : ExceptionalMarkHandler { | ||
protected override void Report (Exception exception) | ||
{ | ||
LinkerConfiguration.Report (context, exception); | ||
} | ||
|
||
protected void Report (List<Exception> exceptions) | ||
{ | ||
LinkerConfiguration.Report (context, exceptions); | ||
} | ||
} | ||
} |
This file contains 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,11 @@ | ||
using Mono.Linker.Steps; | ||
|
||
namespace Xamarin.Linker.Steps { | ||
// MarkSubStepsDispatcher is abstract, so create a subclass we can instantiate. | ||
// Can be removed when we update to the preview4 linker, which makes MarkSubStepsDispatcher non-abstract. | ||
class DotNetMarkAssemblySubStepDispatcher : MarkSubStepsDispatcher { | ||
public DotNetMarkAssemblySubStepDispatcher (params BaseSubStep[] subSteps) : base (subSteps) | ||
{ | ||
} | ||
} | ||
} |
This file contains 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,121 @@ | ||
// Copyright 2016 Xamarin Inc. | ||
|
||
using System; | ||
using Mono.Cecil; | ||
using Mono.Tuner; | ||
using Xamarin.Bundler; | ||
|
||
using Xamarin.Tuner; | ||
|
||
using Mono.Linker; | ||
using Mono.Linker.Steps; | ||
|
||
namespace Xamarin.Linker { | ||
|
||
// Similar to ExceptionalSubStep, but this only runs for marked members | ||
// that were registered for handling by the subclass. | ||
public abstract class ExceptionalMarkHandler : IMarkHandler | ||
{ | ||
public abstract void Initialize (LinkContext context, MarkContext markContext); | ||
|
||
public virtual void Initialize (LinkContext context) | ||
{ | ||
this.context = context; | ||
} | ||
|
||
protected DerivedLinkContext LinkContext => Configuration.DerivedLinkContext; | ||
|
||
protected LinkContext context { get; private set; } | ||
|
||
protected AnnotationStore Annotations => context.Annotations; | ||
protected LinkerConfiguration Configuration => LinkerConfiguration.GetInstance (context); | ||
|
||
protected Profile Profile => Configuration.Profile; | ||
|
||
public void ProcessAssembly (AssemblyDefinition assembly) | ||
{ | ||
try { | ||
Process (assembly); | ||
} catch (Exception e) { | ||
Report (Fail (assembly, e)); | ||
} | ||
} | ||
|
||
public void ProcessType (TypeDefinition type) | ||
{ | ||
try { | ||
Process (type); | ||
} catch (Exception e) { | ||
Report (Fail (type, e)); | ||
} | ||
} | ||
|
||
public void ProcessField (FieldDefinition field) | ||
{ | ||
try { | ||
Process (field); | ||
} catch (Exception e) { | ||
Report (Fail (field, e)); | ||
} | ||
} | ||
|
||
public void ProcessMethod (MethodDefinition method) | ||
{ | ||
try { | ||
Process (method); | ||
} catch (Exception e) { | ||
Report (Fail (method, e)); | ||
} | ||
} | ||
|
||
// state-aware versions to be subclassed | ||
|
||
protected virtual void Process (AssemblyDefinition assembly) | ||
{ | ||
} | ||
|
||
protected virtual void Process (TypeDefinition type) | ||
{ | ||
} | ||
|
||
protected virtual void Process (FieldDefinition field) | ||
{ | ||
} | ||
|
||
protected virtual void Process (MethodDefinition method) | ||
{ | ||
} | ||
|
||
// failure overrides, with defaults | ||
|
||
protected virtual Exception Fail (AssemblyDefinition assembly, Exception e) | ||
{ | ||
return ErrorHelper.CreateError (ErrorCode, e, Errors.MX_ExceptionalSubSteps, Name, assembly?.FullName); | ||
} | ||
|
||
protected virtual Exception Fail (TypeDefinition type, Exception e) | ||
{ | ||
return ErrorHelper.CreateError (ErrorCode | 1, e, Errors.MX_ExceptionalSubSteps, Name, type?.FullName); | ||
} | ||
|
||
protected virtual Exception Fail (FieldDefinition field, Exception e) | ||
{ | ||
return ErrorHelper.CreateError (ErrorCode | 2, e, Errors.MX_ExceptionalSubSteps, Name, field?.FullName); | ||
} | ||
|
||
protected virtual Exception Fail (MethodDefinition method, Exception e) | ||
{ | ||
return ErrorHelper.CreateError (ErrorCode | 3, e, Errors.MX_ExceptionalSubSteps, Name, method?.FullName); | ||
} | ||
protected virtual void Report (Exception e) | ||
{ | ||
throw e; | ||
} | ||
|
||
// abstracts | ||
|
||
protected abstract string Name { get; } | ||
|
||
protected abstract int ErrorCode { get; } | ||
} | ||
} |
This file contains 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,85 @@ | ||
using System; | ||
using System.Linq; | ||
|
||
using Mono.Cecil; | ||
|
||
using Mono.Linker; | ||
using Mono.Linker.Steps; | ||
using Mono.Tuner; | ||
|
||
using Xamarin.Bundler; | ||
|
||
namespace Xamarin.Linker.Steps { | ||
|
||
public class PreserveBlockCodeHandler : ConfigurationAwareMarkHandler { | ||
|
||
protected override string Name { get; } = "Preserve Block Code"; | ||
protected override int ErrorCode { get; } = 2240; | ||
|
||
public override void Initialize (LinkContext context, MarkContext markContext) | ||
{ | ||
base.Initialize (context); | ||
markContext.RegisterMarkTypeAction (ProcessType); | ||
} | ||
|
||
protected override void Process (TypeDefinition type) | ||
{ | ||
/* For the following class: | ||
|
||
static internal class SDInnerBlock { | ||
// this field is not preserved by other means, but it must not be linked away | ||
static internal readonly DInnerBlock Handler = Invoke; | ||
|
||
[MonoPInvokeCallback (typeof (DInnerBlock))] | ||
static internal void Invoke (IntPtr block, int magic_number) | ||
{ | ||
} | ||
} | ||
|
||
We need to make sure the linker doesn't remove the Handler field | ||
and the Invoke method. | ||
*/ | ||
|
||
// First make sure we got the right class | ||
// The type for the field we're looking for is abstract, sealed and nested and contains exactly 1 field. | ||
if (!type.HasFields || !type.IsAbstract || !type.IsSealed || !type.IsNested) | ||
return; | ||
sbomer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (type.Fields.Count != 1) | ||
return; | ||
|
||
// The type is also nested inside ObjCRuntime.Trampolines class) | ||
var nestingType = type.DeclaringType; | ||
if (!nestingType.Is ("ObjCRuntime", "Trampolines")) | ||
return; | ||
|
||
// The class has a readonly field named 'Handler' | ||
var field = type.Fields [0]; | ||
if (!field.IsInitOnly) | ||
return; | ||
if (field.Name != "Handler") | ||
return; | ||
|
||
// The class has a parameterless 'Invoke' method with a 'MonoPInvokeCallback' attribute | ||
if (!type.HasMethods) | ||
return; | ||
var method = type.Methods.SingleOrDefault (v => { | ||
if (v.Name != "Invoke") | ||
return false; | ||
if (!v.HasParameters) | ||
return false; | ||
if (!v.HasCustomAttributes) | ||
return false; | ||
if (!v.CustomAttributes.Any (v => v.AttributeType.Name == "MonoPInvokeCallbackAttribute")) | ||
return false; | ||
return true; | ||
}); | ||
|
||
if (method == null) | ||
return; | ||
|
||
// The type was used, so preserve the method and field | ||
context.Annotations.Mark (method); | ||
context.Annotations.Mark (field); | ||
} | ||
} | ||
} |
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.
is there any plan to make those API public ?
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.
@sbomer why is it needed?
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.
This was just to get something working - I am looking into passing all steps individually as --custom-step args.
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.
SetupStep currently logs the pipeline steps which is one case where making this API public could be useful:
xamarin-macios/tools/dotnet-linker/SetupStep.cs
Lines 116 to 123 in 993c744
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'd like to see that in linker together with timing details under verbose mode
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.
Per discussion with @spouliot and @rolfbjarne the plan is to remove the reflection once dotnet/linker#1314 is fixed, since there's no good workaround for sharing state without it. I also filed dotnet/linker#2013 about the suggestion to output diagnostic info.
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 had quick look dotnet/linker#1314 won't be probably fixed anytime soon as it needs to introduce complex ALC handling. We could just add generic shared state holder to LinkContext and use that instead.
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.
That won't solve the problem where the custom step assembly has a different identity every time it's loaded (so you can't use custom types when storing state).