Update to new linker custom steps API #11374
Conversation
sbomer
force-pushed
the
trimmerCustomSteps
branch
from
7480829 to
3e48452
Compare
| get { | ||
| if (_markHandlers == null) { | ||
| var pipeline = typeof (LinkContext).GetProperty ("Pipeline").GetGetMethod ().Invoke (Context, null); | ||
| _markHandlers = (List<IMarkHandler>) pipeline.GetType ().GetProperty ("MarkHandlers").GetValue (pipeline); |
There was a problem hiding this comment.
is there any plan to make those API public ?
There was a problem hiding this comment.
@sbomer why is it needed?
There was a problem hiding this comment.
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.
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.
I'd like to see that in linker together with timing details under verbose mode
There was a problem hiding this comment.
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.
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.
We could just add generic shared state holder to LinkContext and use that instead.
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).
| @@ -18,16 +19,31 @@ | |||
|
|
|||
| namespace Xamarin.Linker { | |||
|
|
|||
| #if NET | |||
There was a problem hiding this comment.
I don't want to block this PR but wouldn't be better to adopt this method to net6? Looking at the logic it seems to be a simpler version of dead blocks elimination which linker does automatically. At first look, all the code looks like it can be replaced with few substitutions.
There was a problem hiding this comment.
Yes, it might be possible to port it to use substitutions. Our list of things to do is pretty big though, and the current code isn't blocking anything.
There was a problem hiding this comment.
Also much easier to debug / compare by doing this step-by-step.
There was a problem hiding this comment.
I'm not sure substitutions, as available today, would work here. At least not for IsDirectBinding since the decision to always make it true is based on each individual type (computed in an earlier step) and not on some global state (known before build) given to the linker.
There are other places where substitutions would work (e.g. what remains of RemoveCode). We can start there and compare the results (since it won't be identical).
There was a problem hiding this comment.
Captured my thoughts on this as I went over the code in #11477
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
❌ Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffTest results9 tests failed, 98 tests passed.Failed tests
Pipeline on Agent XAMBOT-1104.BigSur' |
|
@sbomer tests are failing with |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
❌ Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diff✅ API Diff (from PR only) (no change) Test results36 tests failed, 71 tests passed.Failed tests
Pipeline on Agent XAMBOT-1100.BigSur' |
- Fix indentation - Add Initialize(LinkContext) to ExceptionalMarkHandler - Remove unnecessary ifdef - Use IsSetter/IsGetter - Use [0] instead of Single() - Avoid allocating empty collections
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diff✅ API Diff (from PR only) (no change) Test results4 tests failed, 103 tests passed.Failed tests
Pipeline on Agent XAMBOT-1096.BigSur' |
@sbomer those are unrelated to your PR and already fixed in HEAD (just rebase to get the fix) |
There was a problem hiding this comment.
I tested this (rebased on HEAD) and it does work.
The app size increases a bit - but we were missing stuff (that did not trigger crash in the small sample used to check the minimal app size).
I'll be comparing the app diff but any further work should be based on those changes anyway so I don't see any reason not to merge this and continue work on different PR.
detection code. This solve the issue that `ILLink` does a similar job _before_ we have the chance to disable the dynamic registrar.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diff✅ API Diff (from PR only) (no change) Test results6 tests failed, 101 tests passed.Failed tests
Pipeline on Agent XAMBOT-1104.BigSur' |
spouliot
added
do-not-merge
I was wrong (same suites but different reasons or I opened the wrong log file). The failures are related to the PR and are still present in the last build (along new ones). Those might have to be adapted since the dotnet linker behave differently. Lack of custom preserve and how IntPtr.Size is removed. The two "new" ones happens because the dynamic registrar is removed - but some tests are not happy (meaning something else is missing to make this work properly). |
|
Thanks, I'm looking into it. |
when it is itself preserved at the assembly-level. This ignored test is checking that feature so it cannot be enabled for `NET` Added to known breaking changes xamarin#8900
|
@sbomer I already know what the issues are :)
The monotouch-test failures is because I only tested dotnet while fixing the dynamic registrar removal (and I broke legacy). |
|
Ah, even better. Thanks! |
|
well I just learned that ilspy uses CTRL (not command) and C for copy... :-(
[CompilerGenerated] // Trigger optimizations
[Foundation.Export ("alsoRequiredForOptimizations")]
void Size8Test ()
{
// Everything in this method should be optimized away (when building for 64-bits)
if (IntPtr.Size != 8)
throw new NUnit.Framework.Internal.NUnitException ("1");
if (IntPtr.Size == 4)
throw new NUnit.Framework.Internal.NUnitException ("2");
if (IntPtr.Size > 8)
throw new NUnit.Framework.Internal.NUnitException ("3");
if (IntPtr.Size < 8)
throw new NUnit.Framework.Internal.NUnitException ("4");
if (IntPtr.Size >= 9)
throw new NUnit.Framework.Internal.NUnitException ("5");
if (IntPtr.Size <= 7)
throw new NUnit.Framework.Internal.NUnitException ("6");
}becomes [CompilerGenerated]
[Export("alsoRequiredForOptimizations")]
private void Size8Test()
{
if (IntPtr.Size != 8)
{
}
if (IntPtr.Size == 4)
{
}
if (IntPtr.Size > 8)
{
}
if (IntPtr.Size < 8)
{
}
if (IntPtr.Size >= 9)
{
}
if (IntPtr.Size > 7)
{
}
}instead of [Export("alsoRequiredForOptimizations")]
private void Size8Test()
{
} |
Good to know :) I think this is happening because the following is (unintentionally) dead code after my changes: xamarin-macios/tools/linker/CoreOptimizeGeneratedCode.cs Lines 753 to 762 in 8fc51f7 I missed that something needs to set [Export("alsoRequiredForOptimizations")]
private void Size8Test()
{
}or is it enough to rely on the linker's built-in constant propagation, which leaves in the |
I think that's the same issue I filed here: dotnet/linker#1106 |
That's for now intentional and is removed by AOT/JIT/interpreter compilers so the impact is minimal |
Yes, the code exists and has never been an issue (only applies to our generated binding code). Even if it has a minimal impact (we can measure later) it's has zero cost, so infinite value ;-)
That I did not expect, nice! |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diff✅ API Diff (from PR only) (no change) Test results2 tests failed, 216 tests passed.Failed tests
Pipeline on Agent XAMBOT-1104.BigSur' |
|
Right now the net effect of the PR is (dotnet numbers only) Application Comparer
The order of changes (to the IL) differs so empty branches (and properties) are left around (see decompiled diff). Not enough to be seen (within padding limits) in the [1] which is not a matter since we'll (at some point) do an IL strip like it's done for legacy. --- dotnet-Xamarin.iOS.cs 2021-05-10 22:39:30.000000000 -0400
+++ sbomer-Xamarin.iOS.cs 2021-05-11 09:23:58.000000000 -0400
@@ -483,6 +483,8 @@
public static Arch Arch;
+ public static bool DynamicRegistrationSupported => false;
+
[MonoPInvokeCallback(typeof(gc_collect_delegate))]
private static void gc_collect(out IntPtr exception_gchandle)
{
@@ -1319,6 +1321,9 @@
options->Delegates->bridge_is_valuetype = GetFunctionPointer(new bridge_is_valuetype_delegate(bridge_is_valuetype));
options->Delegates->bridge_is_delegate = GetFunctionPointer(new bridge_is_delegate_delegate(bridge_is_delegate));
options->Delegates->bridge_is_class_of_type = GetFunctionPointer(new bridge_is_class_of_type_delegate(bridge_is_class_of_type));
+ if (!DynamicRegistrationSupported)
+ {
+ }
}
private unsafe static void Initialize(InitializationOptions* options)
@@ -1331,6 +1336,8 @@
}
if (8 != sizeof(nint))
{
+ _ = 8;
+ _ = 8;
string text2 = string.Format("Native type size mismatch between Xamarin.iOS.dll and the executing architecture. Xamarin.iOS.dll was built for {0}-bit, while the current process is {1}-bit.", "32", "64");
NSLog(text2);
throw ErrorHelper.CreateError(8010, text2);
@@ -1344,6 +1351,9 @@
intptr_bool_ctor_cache = new Dictionary<Type, ConstructorInfo>(TypeEqualityComparer);
lock_obj = new object();
NSObjectClass = NSObject.Initialize();
+ if (DynamicRegistrationSupported)
+ {
+ }
RegisterDelegates(options);
Class.Initialize(options);
InitializePlatform(options);
@@ -2911,7 +2921,7 @@
[DllImport("__Internal")]
private static extern IntPtr xamarin_get_block_descriptor();
- public unsafe void SetupBlockImpl(Delegate trampoline, Delegate userDelegate, bool safe, string signature)
+ private unsafe void SetupBlockImpl(Delegate trampoline, Delegate userDelegate, bool safe, string signature)
{
isa = NSConcreteStackBlock;
invoke = Marshal.GetFunctionPointerForDelegate(trampoline);
@@ -3025,6 +3035,9 @@
BlockLiteral block = default(BlockLiteral);
if (signature == null)
{
+ if (Runtime.DynamicRegistrationSupported)
+ {
+ }
throw ErrorHelper.CreateError(8026, "BlockLiteral.GetBlockForDelegate with a null signature is not supported when the dynamic registrar has been linked away (delegate type: " + @delegate.GetType().FullName + ").");
}
block.SetupBlockImpl((Delegate)value, (Delegate)@delegate, safe: true, signature);
@@ -3070,6 +3083,7 @@
if (registrationMap != null)
{
class_to_type = new Type[registrationMap->map_count];
+ _ = Runtime.DynamicRegistrationSupported;
}
}
@@ -3119,7 +3133,6 @@
private static IntPtr GetClassHandle(Type type, bool throw_if_failure, out bool is_custom_type)
{
- //Discarded unreachable code: IL_00ea
IntPtr value = IntPtr.Zero;
if (type.IsByRef || type.IsPointer || type.IsArray)
{
@@ -3149,6 +3162,7 @@
}
if (value == IntPtr.Zero)
{
+ _ = Runtime.DynamicRegistrationSupported;
if (throw_if_failure)
{
throw ErrorHelper.CreateError(8026, "Can't register the class " + type.FullName + " when the dynamic registrar has been linked away.");
@@ -3200,9 +3214,13 @@
{
return type;
}
+ _ = Runtime.DynamicRegistrationSupported;
intPtr = class_getSuperclass(intPtr);
}
while (intPtr != IntPtr.Zero);
+ if (Runtime.DynamicRegistrationSupported)
+ {
+ }
if (throw_on_error)
{
throw ErrorHelper.CreateError(8026, "Can't lookup the Objective-C class 0x" + klass.ToString("x") + " (" + Marshal.PtrToStringAuto(class_getName(klass)) + ") when the dynamic registrar has been linked away.");
@@ -3858,15 +3876,16 @@
[Export("setTitle:forState:")]
public virtual void SetTitle(string title, UIControlState forState)
{
- //Discarded unreachable code: IL_0030
UIApplication.EnsureUIThread();
IntPtr arg = NSString.CreateNative(title);
if (base.IsDirectBinding)
{
+ _ = 8;
Messaging.void_objc_msgSend_IntPtr_UInt64(base.Handle, Selector.GetHandle("setTitle:forState:"), arg, (ulong)forState);
}
else
{
+ _ = 8;
Messaging.void_objc_msgSendSuper_IntPtr_UInt64(base.SuperHandle, Selector.GetHandle("setTitle:forState:"), arg, (ulong)forState);
}
NSString.ReleaseNative(arg);
@@ -3929,12 +3948,14 @@
[Export("bounds")]
get
{
- //Discarded unreachable code: IL_0030
+ //Discarded unreachable code: IL_002f
UIApplication.EnsureUIThread();
if (base.IsDirectBinding)
{
+ _ = 8;
return Messaging.CGRect_objc_msgSend(base.Handle, Selector.GetHandle("bounds"));
}
+ _ = 8;
return Messaging.CGRect_objc_msgSendSuper(base.SuperHandle, Selector.GetHandle("bounds"));
}
}
@@ -3980,11 +4001,13 @@
[Export("bounds")]
get
{
- //Discarded unreachable code: IL_002b
+ //Discarded unreachable code: IL_002a
if (base.IsDirectBinding)
{
+ _ = 8;
return Messaging.CGRect_objc_msgSend(base.Handle, Selector.GetHandle("bounds"));
}
+ _ = 8;
return Messaging.CGRect_objc_msgSendSuper(base.SuperHandle, Selector.GetHandle("bounds"));
}
}
@@ -4626,12 +4649,13 @@
[Export("compare:")]
public virtual NSComparisonResult Compare(NSString aString)
{
- //Discarded unreachable code: IL_0030
IntPtr nonNullHandle = NativeObjectExtensions.GetNonNullHandle(aString, "aString");
if (base.IsDirectBinding)
{
+ _ = 8;
return (NSComparisonResult)Messaging.Int64_objc_msgSend_IntPtr(base.Handle, Selector.GetHandle("compare:"), nonNullHandle);
}
+ _ = 8;
return (NSComparisonResult)Messaging.Int64_objc_msgSendSuper_IntPtr(base.SuperHandle, Selector.GetHandle("compare:"), nonNullHandle);
}
@@ -5201,6 +5225,7 @@
{
return true;
}
+ _ = Runtime.DynamicRegistrationSupported;
return false;
}
|
This is new... Unlikely to come from this PR (but not impossible) since that project's configuration is not linked. It might be coming from merging |
@spouliot @rolfbjarne here's a start on the update to the new custom steps API. It's not complete yet, but I'd be happy to get early feedback if you have a chance to look.
The changes so far are intended to turn steps into
IMarkHandlers when applicable so that they are triggered by marking:IsNsObjectaren't cached because CoreTypeMapStep isn't part of the pipeline. I will look into changing things to use the type resolution cache that @marek-safar added in Unified handling of Resolve calls dotnet/linker#1979.IsOverriddenInUserCodewill miss overrides that are discovered by the linker later during marking. I think it could be fixed by inverting the logic to check whether a user method is an override of an exported method, and preserving it if it is.