[android] .NET 9 p6, trimmer step no longer saving changes to disk

[jonathanpeppers]

Description

We noticed this happening on both of these PRs:

• [main] Update dependencies from dotnet/sdk android#9031
• [release/9.0.1xx-preview6] Update dependencies from dotnet/sdk android#9051

We have a test failure:

• https://github.com/dotnet/android/blob/e9ee3d4cc35edd5986629d2d5a5415ff93dbe0d5/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs#L183-L207

Xamarin.Android.JcwGen_Tests, Xamarin.Android.JcwGenTests.BindingTests.JavaAbstractMethodTest / Release
Error message
System.TypeLoadException : VTable setup of type Library.MyClrCursor failed
Stack trace
   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object , BindingFlags )

This test is verifying that this trimmer step works:

• https://github.com/dotnet/android/blob/e9ee3d4cc35edd5986629d2d5a5415ff93dbe0d5/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.ILLink.targets#L60
• https://github.com/dotnet/android/blob/e9ee3d4cc35edd5986629d2d5a5415ff93dbe0d5/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs#L305

What the trimmer step does, is attempt to "fill in" new abstract members of new Android APIs. This would allow existing net8.0-android NuGet packages to work in .NET 9, when Android adds new abstract members. (unfortunately Google does!). The general idea, is we'd prevent an exception unless the new member was actually called.

It seems like our trimmer step is doing its work:

ILLink: Added method: System.Void Test.Bindings.ICursor::NewMethod() to type: Library.MyClrCursor scope: Xamarin.Android.FixJavaAbstractMethod-Library.dll
ILLink: Added method: System.Int32 Test.Bindings.ICursor::NewMethodWithParams(System.Int32,System.String) to type: Library.MyClrCursor scope: Xamarin.Android.FixJavaAbstractMethod-Library.dll
ILLink: Added method: System.Int32 Test.Bindings.ICursor::NewMethodWithParams(System.Int32,System.String,System.Single) to type: Library.MyClrCursor scope: Xamarin.Android.FixJavaAbstractMethod-Library.dll
ILLink: Added method: System.Int32 Test.Bindings.ICursor::NewMethodWithRT() to type: Library.MyClrCursor scope: Xamarin.Android.FixJavaAbstractMethod-Library.dll

But we don't see the changes in illink's output.

Reproduction Steps

• Build .NET for Android
• Run test project

.\dotnet-local.cmd build tests\CodeGen-Binding\Xamarin.Android.JcwGen-Tests\Xamarin.Android.JcwGen-Tests.csproj -c Release -t:Install,RunTestApks --p:_ExtraTrimmerArgs=--verbose

Expected behavior

JavaAbstractMethodTest passes

Actual behavior

JavaAbstractMethodTest fails with TypeLoadException

Regression?

Yes, this worked in .NET 9 Preview 5

Known Workarounds

None

Configuration

Runtime: 9.0.0-preview.6.24321.8 or newer

Other information

.binlog: illink-verbose.zip

Output assemblies: linked.zip

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[agocke]

@sbomer

[vitek-karas]

On debug build this asserts in the trimmer, but it looks unrelated to the problem:

runtime/src/tools/illink/src/linker/Linker/Annotations.cs

Line 307 in 203f993

Debug.Assert (preserve != TypePreserve.Nothing);

The call comes from:
https://github.com/dotnet/android/blob/06bb1dc6a292ef5618a3bb6ecca3ca869253ff2e/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs#L346

This assert has been there for 3 years now - added in dotnet/linker#1768
And the caller was updated to match the new APIs in dotnet/android#5748 - apparently not correctly :-)

So this is probably a long standing bug which doesn't seem to matter much - I didn't try to figure out what the assert guards against and thus what is the problem (been a while).

@sbomer - probably worth filing a bug/PR in dotnet/android for this.

[vitek-karas]

(This looks to be a bit of a bug farm :-))
For context - The above asserts is happening around type Java.Lang.AbstractMethodError
Ignoring the above assert leads to another assert, this time in:

runtime/src/tools/illink/src/linker/Linker/Annotations.cs

Line 511 in a900bbf

Debug.Assert (GetPreservedMethods (definition) == null);

The method in question is Java.Lang.ICharSequenceInvoker..ctor - so it's very likely unrelated to the previous assert.
The caller is (a bit indirectly):
https://github.com/dotnet/android/blob/06bb1dc6a292ef5618a3bb6ecca3ca869253ff2e/src/Microsoft.Android.Sdk.ILLink/MarkJavaObjects.cs#L189

and then

https://github.com/dotnet/android/blob/06bb1dc6a292ef5618a3bb6ecca3ca869253ff2e/src/Microsoft.Android.Sdk.ILLink/MarkJavaObjects.cs#L147

I vaguely remember something around the AddPreserveMethod and the complexity behind it. This feels like an internal illink problem which is triggered by the custom step. So either unexpected call sequence from the custom step or some other problem.

Another very likely unrelated bug to the original issue here.

This second assert is hit many times over on various other methods, in all cases the class name seems to include the word Invoker - but I don't know if that makes it special somehow.

[vitek-karas]

The actual problem in this case is because the added method is not marked so SweepStep removes it.

I looked at this:

ILLink: Added method: System.Void Test.Bindings.ICursor::NewMethod() to type: Library.MyClrCursor scope: Xamarin.Android.FixJavaAbstractMethod-Library.dll

The part I don't understand is that this behavior has been like this since forever. It's possible this has something to do with interface overrides as the added method is meant to implement ICursor.NewMethod which seems to be preserved in the output (it's part of a different assembly). So it's possible that before the more recent changes to better implement interface marking this method ended up being marked by some other logic in the linker, but now it's not.

This looks really weird - the output is "corrupted" I think:

As you can see the MyClrCursor class doesn't implement the method NewMethod on the interface ICursor even though it does implement that interface.

It's possible that this is caused by the fact that the method is added by a custom step, since the addition happens during MarkType and it might be "Too late" for some of the internal caches in the trimmer - but really I'm just guessing, unfortunately I lost track of how interface marking works in the trimmer after the recent changes.

I think this will require either @jtschuster or @sbomer to take a deeper look.

I'll send you the repro offline.

[vitek-karas]

On a related note:
@jonathanpeppers it might be a good idea to try to see if we could run a debug trimmer in one of the CI lanes in the dotnet/android repo - as apparently the custom steps are not following the expected behavior :-)
But I don't know if we publish debug packages on the transfer feeds.

[jtschuster]

#103140 could be relevant here.

[sbomer]

Thank you for the repro and notes @vitek-karas!

fbb67a6 introduced the specific regression. Similar to #103115 (comment), this looks like a bad interaction with custom steps and potentially with the interface logic.

The custom step is relying on getting called before we build the type info for types it touches. This isn't guaranteed, and I am surprised it didn't cause problems earlier. Here's the sequence of events in the test failure:

• We reflection mark a method from MyClrCursor: https://github.com/dotnet/android/blob/06bb1dc6a292ef5618a3bb6ecca3ca869253ff2e/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs#L204
• To process base methods of that method, we build the type info for every type in that assembly, here:

  runtime/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs

  Line 3388 in 7f7bb7b

  var base_methods = Annotations.GetBaseMethods (method);

  At this point the custom step hasn't run, so we don't discover the new methods in MapInterfaceMethodsInTypeHierarchy.
• Eventually we get to ProcessInterfaceMethods. Normally this would discover interface method implementations, but because it uses the cached type map info we don't see it.

Before fbb67a6, while processing JavaAbstractMethodTest, the call to Library.MyClrCursor () would call MarkType for MyClrCursor. This called the custom step immediately (instead of delaying until after we process the reflection-accessed method), so it happened that the custom step got a chance to process the type before we called into MapTypeInfo for the assembly. But I think this is a latent issue that just happened to show up now, since we don't guarantee the custom step gets called before building type info.

If we want to make this guarantee we might need to build the type info cache per type, instead of per assembly, and ensure it never gets built for a type before ProcessType calls the custom step.

[jonathanpeppers]

On a related note: @jonathanpeppers it might be a good idea to try to see if we could run a debug trimmer in one of the CI lanes in the dotnet/android repo - as apparently the custom steps are not following the expected behavior :-) But I don't know if we publish debug packages on the transfer feeds.

If someone can share some instructions, I could try this, thanks. We just use dotnet-install.sh/ps1 to provision .NET, so I would expect we'd need some MSBuild logic to swap ILLink packs.

It does seem like it would be useful for part of our CI to fail, if one of those Debug.Assert() calls are hit.

[agocke]

But I don't know if we publish debug packages on the transfer feeds.

Unfortunately we don't. It might be tricky to enable this. We do sometimes pass around non-shipping bits in transport packages, but building in multiple configurations can get hairy, and you have to be very careful not to depend on the non-shipping bits in any shipping configuration.

It might be easier to light up these asserts based on some environment variable.

[jtschuster]

@jonathanpeppers I want to make sure I understand the problem that FixAbstractMethods solves. Would the problem also be fixed if the Android libraries created default interface methods that throw Java.Lang.AbstractMethodError (or if the trimmer adds an implementation into the interface instead of the implementing type)?

[jonathanpeppers]

Yes, there isn't a good public link that describes it. Ancient commit history from Xamarin says:

[linker] fix abstract java methods

• it adds FixAbstractMethods linker step, which looks for methods
  with unimplemented abstract methods in non-abstract classes,
  derived from Java.Lang.Object class, and fixes them by adding
  implementation, which throws AbstractMethodError exception

• the newly added step is used in XA linker

• the LinkAssemblies task is now used even for Debug configuration
  when LinkMode is set to LinkNone. very simple "linker" is build in
  this case, where the pipeline contains only FixAbstractMethods and
  Output steps. this task in Debug configuration takes about 80 to
  190 ms on my system, with test app

radekdoulik
committed 9 years ago

My understanding, it's something like:

1. Android version X has an ICursor interface
2. You implement it in C#, put it in a NuGet package
3. Android version Y adds a new method to ICursor, so we add it to the Android workload for .NET

The problem here is C# will throw TypeLoadException very early if you try to use the existing C# class in no. 2. While for Java (JVM), they are less strict about it -- you would only get AbstractMethodError if you called the method, while C# it happens when the type is loaded.

So, the trimmer step goes through and adds the missing methods, and just makes them throw. But this gives us the same behavior as Java.

[agocke]

@sbomer and I discussed this a few weeks ago and I think the conclusion was that strictly adding default interface methods that throw wasn’t fully correct because you want the compiler to error if the user doesn’t implement all the methods. The specific case this is trying to protect against is when new interface methods were added later, in newer runtime assemblies.

An alternate fix would be to produce reference assemblies with no default implementations, and impl assemblies with default implementations.

Another fix would be a standalone tool that runs before the linker and does the same thing. There’s no dependency in linker behavior here.

[jonathanpeppers]

I think the conclusion was that strictly adding default interface methods that throw wasn’t fully correct because you want the compiler to error if the user doesn’t implement all the methods.

Yes, they would get this behavior if a NuGet package author updated their major .NET version newer and recompiled. But the problem occurs for existing NuGet packages. You can still use net8.0-android class libraries in net9.0-android apps, for example.

Another fix would be a standalone tool that runs before the linker and does the same thing. There’s no dependency in linker behavior here.

For this part, we would need to do something like this for NativeAOT as well, as I think there is no custom trimmer step support.

What we have right now is the solution from almost a decade ago. This is something we could rework for .NET 10, if there is a good reason for removing this step.