From ff707c145e6b011000b79606194ae2eb8eda3855 Mon Sep 17 00:00:00 2001 From: Rolf Bjarne Kvinge Date: Mon, 2 Sep 2024 15:02:10 +0200 Subject: [PATCH 1/2] [dotnet-linker] Trim away the static constructor for protocol interfaces if we're registering protocols in the static registrar. (#21012) When we implemented support for using default interface members for binding protocols, we also unintentionally introduced a size regression. This happened because we now tell the linker to keep all methods in a protocol interface, thus all the corresponding types end up marked as well, etc. This had an additional side effect: depending on the types that weren't linked away anymore, the App Store could flag an app, saying that additional entitlements is needed. This is what's happening in #21002: the App Store detects that the app references the `application:didRegisterForRemoteNotificationsWithDeviceToken:` selector [1] (from the method `RegisteredForRemoteNotifications` on `IUIApplicationDelegate`) and tells the developer they probably need the push notification entitlement. The good news is that we don't need these protocol interface methods at runtime if the optimization to register protocols with the static registrar is enabled (which it is by default). In this PR I teach the optimizer to remove the DynamicDependency attributes keeping these protocol interface methods from being trimmed out. ## Size improvements * monotouch-test build for Release/ios-arm64 shrinks by [2.9mb (-2.6%)](https://gist.github.com/rolfbjarne/5e8ca6ea6854dc4a46f8e838dff11e6b) * A very simple app (tests/dotnet/MySimpleApp) shrinks by [176kb (-0.3%)](https://gist.github.com/rolfbjarne/f0e062900528eb499fd96d124d18376f) [1]: This is somewhat speculative, but it's probably not far from what the App Store actually does. Fixes #21002. --- src/bgen/Generator.cs | 1 + .../ObjCRuntime/RegistrarTest.cs | 32 ++++++++ .../Steps/CollectUnmarkedMembers.cs | 6 ++ .../dotnet-linker/Steps/PreMarkDispatcher.cs | 1 + .../Steps/SetBeforeFieldInitStep.cs | 55 +++++++++++++ tools/linker/CoreOptimizeGeneratedCode.cs | 77 +++++++++++++++++++ tools/mtouch/Errors.designer.cs | 18 +++++ tools/mtouch/Errors.resx | 7 ++ 8 files changed, 197 insertions(+) create mode 100644 tools/dotnet-linker/Steps/SetBeforeFieldInitStep.cs diff --git a/src/bgen/Generator.cs b/src/bgen/Generator.cs index a43205b4f7..70481dab89 100644 --- a/src/bgen/Generator.cs +++ b/src/bgen/Generator.cs @@ -5074,6 +5074,7 @@ public partial class Generator : IMemberGatherer { foreach (var docId in docIds) { print ($"[DynamicDependencyAttribute (\"{docId}\")]"); } + print ("[BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)]"); print ($"static I{TypeName} ()"); print ("{"); print ("\tGC.KeepAlive (null);"); // need to do _something_ (doesn't seem to matter what), otherwise the static cctor (and the DynamicDependency attributes) are trimmed away. diff --git a/tests/monotouch-test/ObjCRuntime/RegistrarTest.cs b/tests/monotouch-test/ObjCRuntime/RegistrarTest.cs index db0661c43b..5232aceaf8 100644 --- a/tests/monotouch-test/ObjCRuntime/RegistrarTest.cs +++ b/tests/monotouch-test/ObjCRuntime/RegistrarTest.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.Drawing; using System.Linq; +using System.Reflection; using System.Runtime.InteropServices; using System.Runtime.Versioning; using System.Threading; @@ -5536,6 +5537,37 @@ namespace MonoTouchFixtures.ObjCRuntime { Assert.AreEqual (EnumUL.b, ul, "out: UL"); } } + +#if NET && HAS_UIKIT + [Test] + public void ProtocolsTrimmedAway () + { + PreserveIUIApplicationDelegate (null); + + // A little indirection to try to make the trimmer not be helpful and preserve all the methods on IUIApplicationDelegate. + AssertMemberCount (typeof (IUIApplicationDelegate)); + } + + void AssertMemberCount (Type type) + { + var members = type.GetMembers (BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static | BindingFlags.Instance); +#if OPTIMIZEALL || NATIVEAOT + var expectNoMembers = true; +#else + var expectNoMembers = false; +#endif + if (expectNoMembers) { + Assert.AreEqual (0, members.Length, $"All members should be trimmed away in {type.FullName}:\n\t{string.Join ("\n\t", members.Select (v => v.ToString ()))}"); + } else { + Assert.AreNotEqual (0, members.Length, $"All members should not be trimmed away in {type.FullName}"); + } + } + + void PreserveIUIApplicationDelegate (IUIApplicationDelegate obj) + { + GC.KeepAlive (obj); + } +#endif // NET && HAS_UIKIT } #if !__WATCHOS__ diff --git a/tools/dotnet-linker/Steps/CollectUnmarkedMembers.cs b/tools/dotnet-linker/Steps/CollectUnmarkedMembers.cs index 0e257297ea..0fef0c7c4d 100644 --- a/tools/dotnet-linker/Steps/CollectUnmarkedMembers.cs +++ b/tools/dotnet-linker/Steps/CollectUnmarkedMembers.cs @@ -26,6 +26,12 @@ namespace Xamarin.Linker { if (!Annotations.IsMarked (type)) LinkContext.AddLinkedAwayType (type); + if (type.IsInterface && + Configuration.DerivedLinkContext.App.Optimizations.RegisterProtocols == true && + type.HasCustomAttribute (LinkContext, Namespaces.Foundation, "ProtocolAttribute")) { + Configuration.DerivedLinkContext.StoreProtocolMethods (type); + } + if (type.HasInterfaces) { foreach (var iface in type.Interfaces) { if (Annotations.IsMarked (iface)) diff --git a/tools/dotnet-linker/Steps/PreMarkDispatcher.cs b/tools/dotnet-linker/Steps/PreMarkDispatcher.cs index fa091bf178..8970681eb1 100644 --- a/tools/dotnet-linker/Steps/PreMarkDispatcher.cs +++ b/tools/dotnet-linker/Steps/PreMarkDispatcher.cs @@ -7,6 +7,7 @@ namespace Xamarin.Linker.Steps { class PreMarkDispatcher : SubStepsDispatcher { public PreMarkDispatcher () : base (new BaseSubStep [] { + new SetBeforeFieldInitStep (), new CollectUnmarkedMembersSubStep (), new StoreAttributesStep () }) diff --git a/tools/dotnet-linker/Steps/SetBeforeFieldInitStep.cs b/tools/dotnet-linker/Steps/SetBeforeFieldInitStep.cs new file mode 100644 index 0000000000..22f70f051e --- /dev/null +++ b/tools/dotnet-linker/Steps/SetBeforeFieldInitStep.cs @@ -0,0 +1,55 @@ +using Mono.Linker.Steps; +using Xamarin.Linker; + +using Mono.Cecil; +using Mono.Tuner; + +#nullable enable + +namespace Xamarin.Linker.Steps { + public class SetBeforeFieldInitStep : ConfigurationAwareSubStep { + protected override string Name { get; } = "Set BeforeFieldInit"; + protected override int ErrorCode { get; } = 2380; + + public override SubStepTargets Targets { + get { + return SubStepTargets.Type; + } + } + + protected override void Process (TypeDefinition type) + { + // If we're registering protocols, we want to remove the static + // constructor on the protocol interface, because it's not needed + // (because we've removing all the DynamicDependency attributes + // from the cctor). + // + // However, just removing the static constructor from the type + // causes problems later on in the trimming process, so we want + // the trimmer to just not mark it. + // + // The trimmer marks it, because it has a static constructor, so + // we're in a bit of a cyclic dependency here. + // + // This is complicated by a few facts: + // - When we optimize the cctor (i.e. removing the + // DynamicDependency attributes), the cctor is already marked. + // - Adding a MarkHandler that processes types doesn't work + // either, because it may be called after the cctor is marked: + // https://github.com/dotnet/runtime/blob/6177a9f920861900681cfda2b6cc66ac3557e93b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs#L1928-L1952 + // + // So this is a pre-mark step that just sets + // IsBeforeFieldInit=true for interfaces we want trimmed away by + // the linker. + + if (Configuration.DerivedLinkContext.App.Optimizations.RegisterProtocols != true) + return; + + if (!type.IsBeforeFieldInit && type.IsInterface && type.HasMethods) { + var cctor = type.GetTypeConstructor (); + if (cctor is not null && cctor.IsBindingImplOptimizableCode (LinkContext)) + type.IsBeforeFieldInit = true; + } + } + } +} diff --git a/tools/linker/CoreOptimizeGeneratedCode.cs b/tools/linker/CoreOptimizeGeneratedCode.cs index 0e49ce76e6..356e3289ae 100644 --- a/tools/linker/CoreOptimizeGeneratedCode.cs +++ b/tools/linker/CoreOptimizeGeneratedCode.cs @@ -755,6 +755,9 @@ namespace Xamarin.Linker { return; // nothing else to do here. } + if (ProcessProtocolInterfaceStaticConstructor (method)) + return; + var instructions = method.Body.Instructions; for (int i = 0; i < instructions.Count; i++) { var ins = instructions [i]; @@ -1191,6 +1194,19 @@ namespace Xamarin.Linker { return ins; } + static Instruction SkipNops (Instruction ins) + { + if (ins is null) + return null; + + while (ins.OpCode == OpCodes.Nop) { + if (ins.Next is null) + return null; + ins = ins.Next; + } + return ins; + } + int ProcessIsARM64CallingConvention (MethodDefinition caller, Instruction ins) { const string operation = "inline Runtime.IsARM64CallingConvention"; @@ -1345,5 +1361,66 @@ namespace Xamarin.Linker { } return caller.Module.ImportReference (block_ctor_def); } + + bool ProcessProtocolInterfaceStaticConstructor (MethodDefinition method) + { + // The static cctor in protocol interfaces exists only to preserve the protocol's members, for inspection by the registrar(s). + // If we're registering protocols, then we don't need to preserve protocol members, because the registrar + // already knows everything about it => we can remove the static cctor. + + if (!(method.DeclaringType.IsInterface && method.DeclaringType.IsInterface && method.IsStatic && method.IsConstructor && method.HasBody)) + return false; + + if (Optimizations.RegisterProtocols != true) { + Driver.Log (4, "Did not optimize static constructor in the protocol interface {0}: the 'register-protocols' optimization is disabled.", method.DeclaringType.FullName); + return false; + } + + if (!method.DeclaringType.HasCustomAttributes || !method.DeclaringType.CustomAttributes.Any (v => v.AttributeType.Is ("Foundation", "ProtocolAttribute"))) { + Driver.Log (4, "Did not optimize static constructor in the protocol interface {0}: no Protocol attribute found.", method.DeclaringType.FullName); + return false; + } + + var ins = SkipNops (method.Body.Instructions.First ()); + if (ins is null) { + ErrorHelper.Show (ErrorHelper.CreateWarning (LinkContext.App, 2112, method, ins, Errors.MX2112_A /* Could not optimize the static constructor in the interface {0} because it did not have the expected instruction sequence (found end of method too soon). */, method.DeclaringType.FullName)); + return false; + } else if (ins.OpCode != OpCodes.Ldnull) { + ErrorHelper.Show (ErrorHelper.CreateWarning (LinkContext.App, 2112, method, ins, Errors.MX2112_B /* Could not optimize the static constructor in the interface {0} because it had an unexpected instruction {1} at offset {2}. */, method.DeclaringType.FullName, ins.OpCode, ins.Offset)); + return false; + } + + ins = SkipNops (ins.Next); + var callGCKeepAlive = ins; + if (ins is null) { + ErrorHelper.Show (ErrorHelper.CreateWarning (LinkContext.App, 2112, method, ins, Errors.MX2112_A /* Could not optimize the static constructor in the interface {0} because it did not have the expected instruction sequence (found end of method too soon). */, method.DeclaringType.FullName)); + return false; + } else if (callGCKeepAlive.OpCode != OpCodes.Call || !(callGCKeepAlive.Operand is MethodReference methodOperand) || methodOperand.Name != "KeepAlive" || !methodOperand.DeclaringType.Is ("System", "GC")) { + ErrorHelper.Show (ErrorHelper.CreateWarning (LinkContext.App, 2112, method, ins, Errors.MX2112_B /* Could not optimize the static constructor in the interface {0} because it had an unexpected instruction {1} at offset {2}. */, method.DeclaringType.FullName, ins.OpCode, ins.Offset)); + return false; + } + + ins = SkipNops (ins.Next); + if (ins is null) { + ErrorHelper.Show (ErrorHelper.CreateWarning (LinkContext.App, 2112, method, ins, Errors.MX2112_A /* Could not optimize the static constructor in the interface {0} because it did not have the expected instruction sequence (found end of method too soon). */, method.DeclaringType.FullName)); + return false; + } else if (ins.OpCode != OpCodes.Ret) { + ErrorHelper.Show (ErrorHelper.CreateWarning (LinkContext.App, 2112, method, ins, Errors.MX2112_B /* Could not optimize the static constructor in the interface {0} because it had an unexpected instruction {1} at offset {2}. */, method.DeclaringType.FullName, ins.OpCode, ins.Offset)); + return false; + } + + ins = SkipNops (ins.Next); + if (ins is not null) { + ErrorHelper.Show (ErrorHelper.CreateWarning (LinkContext.App, 2112, method, ins, Errors.MX2112_B /* Could not optimize the static constructor in the interface {0} because it had an unexpected instruction {1} at offset {2}. */, method.DeclaringType.FullName, ins.OpCode, ins.Offset)); + return false; + } + + // We can just remove the entire method, however that confuses the linker later on, so just empty it out and remove all the attributes. + Driver.Log (4, "Optimized static constructor in the protocol interface {0} (static constructor was cleared and custom attributes removed)", method.DeclaringType.FullName); + method.Body.Instructions.Clear (); + method.Body.Instructions.Add (Instruction.Create (OpCodes.Ret)); + method.CustomAttributes.Clear (); + return true; + } } } diff --git a/tools/mtouch/Errors.designer.cs b/tools/mtouch/Errors.designer.cs index 8ff0567c53..023c6f0a66 100644 --- a/tools/mtouch/Errors.designer.cs +++ b/tools/mtouch/Errors.designer.cs @@ -3923,6 +3923,24 @@ namespace Xamarin.Bundler { } } + /// + /// Looks up a localized string similar to Could not optimize the static constructor in the interface {0} because it did not have the expected instruction sequence (found end of method too soon).. + /// + public static string MX2112_A { + get { + return ResourceManager.GetString("MX2112_A", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Could not optimize the static constructor in the interface {0} because it had an unexpected instruction {1} at offset {2}.. + /// + public static string MX2112_B { + get { + return ResourceManager.GetString("MX2112_B", resourceCulture); + } + } + /// /// Looks up a localized string similar to Could not {0} the assembly '{1}' /// . diff --git a/tools/mtouch/Errors.resx b/tools/mtouch/Errors.resx index e68e28c514..a909ea87a5 100644 --- a/tools/mtouch/Errors.resx +++ b/tools/mtouch/Errors.resx @@ -1327,6 +1327,13 @@ + + Could not optimize the static constructor in the interface {0} because it did not have the expected instruction sequence (found end of method too soon). + + + + Could not optimize the static constructor in the interface {0} because it had an unexpected instruction {1} at offset {2}. + From 3b6a5c20c6f7c9d99980893433d5454958a723a5 Mon Sep 17 00:00:00 2001 From: Rolf Bjarne Kvinge Date: Mon, 2 Sep 2024 15:02:39 +0200 Subject: [PATCH 2/2] [bgen] Add support for delegates with pointer types. (#21159) --- src/bgen/Generator.cs | 14 ++++++++++++-- tests/generator/BGenTests.cs | 15 +++++++++++++++ tests/generator/tests/delegate-types.cs | 15 +++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 tests/generator/tests/delegate-types.cs diff --git a/src/bgen/Generator.cs b/src/bgen/Generator.cs index 70481dab89..85db12aec7 100644 --- a/src/bgen/Generator.cs +++ b/src/bgen/Generator.cs @@ -718,6 +718,14 @@ public partial class Generator : IMemberGatherer { } } + if (pi.ParameterType.IsPointer && pi.ParameterType.GetElementType ().IsValueType) { + // Technically we should only allow blittable types here, but the C# compiler shows an error later on if any non-blittable types + // are used, because this ends up in the signature of a UnmanagedCallersOnly method. + pars.Add (new TrampolineParameterInfo (TypeManager.FormatType (null, pi.ParameterType), safe_name)); + invoke.Append (safe_name); + continue; + } + if (pi.ParameterType.IsSubclassOf (TypeCache.System_Delegate)) { if (!delegate_types.ContainsKey (pi.ParameterType.Name)) { delegate_types [pi.ParameterType.FullName] = pi.ParameterType.GetMethod ("Invoke"); @@ -4731,11 +4739,13 @@ public partial class Generator : IMemberGatherer { print ("[MonoNativeFunctionWrapper]\n"); var accessibility = mi.DeclaringType.IsInternal (this) ? "internal" : "public"; - print ("{3} delegate {0} {1} ({2});", + var isUnsafe = mi.GetParameters ().Any ((v => v.ParameterType.IsPointer)) || mi.ReturnType.IsPointer; + print ("{3}{4} delegate {0} {1} ({2});", TypeManager.RenderType (mi.ReturnType, mi.ReturnTypeCustomAttributes), shortName, RenderParameterDecl (mi.GetParameters ()), - accessibility); + accessibility, + isUnsafe ? " unsafe" : string.Empty); } if (group.Namespace is not null) { diff --git a/tests/generator/BGenTests.cs b/tests/generator/BGenTests.cs index 155bac2973..26e67f552a 100644 --- a/tests/generator/BGenTests.cs +++ b/tests/generator/BGenTests.cs @@ -1719,5 +1719,20 @@ namespace GeneratorTests { var delegateCallback = bgen.ApiAssembly.MainModule.GetType ("NS", "MyCallback").Methods.First ((v) => v.Name == "EndInvoke"); Assert.That (delegateCallback.MethodReturnType.CustomAttributes.Any (v => v.AttributeType.Name == "NullableAttribute"), "Nullable return type"); } + + [Test] + [TestCase (Profile.iOS)] + public void DelegatesWithPointerTypes (Profile profile) + { + Configuration.IgnoreIfIgnoredPlatform (profile.AsPlatform ()); + var bgen = BuildFile (profile, "tests/delegate-types.cs"); + bgen.AssertNoWarnings (); + + var delegateCallback = bgen.ApiAssembly.MainModule.GetType ("NS", "MyCallback").Methods.First ((v) => v.Name == "EndInvoke"); + Assert.IsTrue (delegateCallback.MethodReturnType.ReturnType.IsPointer, "Pointer return type"); + foreach (var p in delegateCallback.Parameters.Where (v => v.Name != "result")) { + Assert.IsTrue (p.ParameterType.IsPointer, $"Pointer parameter type: {p.Name}"); + } + } } } diff --git a/tests/generator/tests/delegate-types.cs b/tests/generator/tests/delegate-types.cs new file mode 100644 index 0000000000..650fef631b --- /dev/null +++ b/tests/generator/tests/delegate-types.cs @@ -0,0 +1,15 @@ +using System; + +using Foundation; +using ObjCRuntime; + +namespace NS { + unsafe delegate byte* MyCallback (sbyte* a, short* b, ushort* c, int* d, uint* e, long* f, ulong* g, float* h, double* i); + + [BaseType (typeof (NSObject))] + interface Widget { + [Export ("foo")] + [NullAllowed] + MyCallback Foo { get; set; } + } +}