Skip to content

[One .NET] fix more Mono.Android.dll linker warnings #6270

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

Merged
jonathanpeppers merged 3 commits into from
Sep 17, 2021
Merged

[One .NET] fix more Mono.Android.dll linker warnings #6270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0c5753d
[One .NET] fix more Mono.Android.dll linker warnings
jonathanpeppers Sep 7, 2021
bab7d0a
Don't preserve Adapter types in MarkJavaObjects
jonathanpeppers Sep 10, 2021
21b441e
Clean up XASdkTests.cs
jonathanpeppers Sep 15, 2021
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
2 changes: 2 additions & 0 deletions src/Mono.Android/Android.Runtime/ResourceIdManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;

namespace Android.Runtime
Expand Down Expand Up @@ -30,6 +31,7 @@ public static void UpdateIdValues ()
}
}

[UnconditionalSuppressMessage ("Trimming", "IL2026", Justification = "Types in Resource.designer.cs are preserved, because it is the root assembly passed to the linker.")]
static Type? GetResourceTypeFromAssembly (Assembly assembly)
{
foreach (var customAttribute in assembly.GetCustomAttributes (typeof (ResourceDesignerAttribute), true)) {
Expand Down
10 changes: 6 additions & 4 deletions src/Mono.Android/Java.Interop/JavaObjectExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ static IJavaObject CastClass (IJavaObject instance, Type resultType)

if (resultType.IsAbstract) {
// TODO: keep in sync with TypeManager.CreateInstance() algorithm
var invokerType = GetHelperType (resultType, "Invoker");
var invokerType = GetInvokerType (resultType);
if (invokerType == null)
throw new ArgumentException ("Unable to get Invoker for abstract type '" + resultType.FullName + "'.", "TResult");
resultType = invokerType;
Expand Down Expand Up @@ -122,10 +122,12 @@ static IJavaObject CastClass (IJavaObject instance, Type resultType)
instance.GetType ().FullName, resultType.FullName));
}

// typeof(Foo) -> FooSuffix
// typeof(Foo<>) -> FooSuffix`1
internal static Type? GetHelperType (Type type, string suffix)
// typeof(Foo) -> FooInvoker
// typeof(Foo<>) -> FooInvoker`1
[UnconditionalSuppressMessage ("Trimming", "IL2026", Justification = "*Invoker types are preserved by the MarkJavaObjects linker step.")]
internal static Type? GetInvokerType (Type type)
Comment on lines -127 to +128

@jonathanpeppers jonathanpeppers Sep 9, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@jonpryor this was the part, I wanted to check it's OK to rename this method.

I thought allowing any string suffix through here could introduce a future linker issue, because MarkJavaObjects only preserves *Invoker and *Adapter right now.

@jonpryor jonpryor Sep 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This just raises a related question: what's a *Adapter type, and why is it being preserved? I forget. :-(

I think for "consistency" we should consider updating src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/MarkJavaObjects.cs to likewise rename GetHelperType() to, or otherwise introduce, GetInvokerType(). At least then our internal terminology would be consistent.

@jonpryor jonpryor Sep 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This appears to be the commit which introduce the Adapter suffix; it was apparently for interfaces: https://github.com/xamarin/monodroid/commit/ff043c0281fbe1fefb3e83b7ccda1eca791c3fb0

@jonpryor jonpryor Sep 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suspect what happened is we "unified" the Adapter and Invoker suffixes into Invoker, but I can't easily find such a commit.

@jonpryor jonpryor Sep 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like Adapter was removed in generator output, with Invoker as the replacement, as part of https://github.com/xamarin/monodroid/commit/efeff037dfc4d4c60ad87fd6d6b22d9af45d7583

So it looks like there's no need to preserve Adapter-suffixed types, not since 2011.

@jonathanpeppers jonathanpeppers Sep 10, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated the MarkJavaObjects step. I wonder if we'll see any .apk size differences?

{
const string suffix = "Invoker";
Type[] arguments = type.GetGenericArguments ();
if (arguments.Length == 0)
return type.Assembly.GetType (type + suffix);
Expand Down
2 changes: 1 addition & 1 deletion src/Mono.Android/Java.Interop/TypeManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ internal static IJavaPeerable CreateInstance (IntPtr handle, JniHandleOwnership
type = targetType;

if (type.IsInterface || type.IsAbstract) {
var invokerType = JavaObjectExtensions.GetHelperType (type, "Invoker");
var invokerType = JavaObjectExtensions.GetInvokerType (type);
if (invokerType == null)
throw new NotSupportedException ("Unable to find Invoker for type '" + type.FullName + "'. Was it linked away?",
CreateJavaLocationException ());
Expand Down
34 changes: 4 additions & 30 deletions src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/MarkJavaObjects.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ void PreserveJavaObjectImplementation (TypeDefinition type)
{
PreserveIntPtrConstructor (type);
PreserveAttributeSetConstructor (type);
PreserveAdapter (type);
PreserveInvoker (type);
}

Expand Down Expand Up @@ -151,24 +150,6 @@ void PreserveMethod (TypeDefinition type, MethodDefinition method)
Annotations.AddPreservedMethod (type, method);
}

void PreserveAdapter (TypeDefinition type)
{
var adapter = PreserveHelperType (type, "Adapter");

if (adapter == null || !adapter.HasMethods)
return;

foreach (MethodDefinition method in adapter.Methods) {
if (method.Name != "GetObject")
continue;

if (method.Parameters.Count != 2)
continue;

PreserveMethod (type, method);
}
}

string TypeNameWithoutKey (string name)
{
var idx = name.IndexOf (", PublicKeyToken=");
Expand Down Expand Up @@ -208,25 +189,18 @@ void PreserveInterfaceMethods (TypeDefinition type, TypeDefinition invoker)

void PreserveInvoker (TypeDefinition type)
{
var invoker = PreserveHelperType (type, "Invoker");
var invoker = GetInvokerType (type);
if (invoker == null)
return;

PreserveConstructors (type, invoker);
PreserveIntPtrConstructor (invoker);
PreserveInterfaceMethods (type, invoker);
}

TypeDefinition PreserveHelperType (TypeDefinition type, string suffix)
{
var helper = GetHelperType (type, suffix);
if (helper != null)
PreserveConstructors (type, helper);

return helper;
}

TypeDefinition GetHelperType (TypeDefinition type, string suffix)
TypeDefinition GetInvokerType (TypeDefinition type)
{
const string suffix = "Invoker";
string fullname = type.FullName;

if (type.HasGenericParameters) {
Expand Down
6 changes: 6 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/XASdkTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,14 @@ public void DotNetNew ([Values ("android", "androidlib", "android-bindinglib")]
File.WriteAllBytes (Path.Combine (dotnet.ProjectDirectory, "foo.jar"), ResourceData.JavaSourceJarTestJar);
Assert.IsTrue (dotnet.New ("android-activity"), "`dotnet new android-activity` should succeed");
Assert.IsTrue (dotnet.New ("android-layout", Path.Combine (dotnet.ProjectDirectory, "Resources", "layout")), "`dotnet new android-layout` should succeed");

// Debug build
Assert.IsTrue (dotnet.Build (), "`dotnet build` should succeed");
dotnet.AssertHasNoWarnings ();

// Release build
Assert.IsTrue (dotnet.Build (parameters: new [] { "Configuration=Release" }), "`dotnet build` should succeed");
dotnet.AssertHasNoWarnings ();
}

[Test]
Expand Down