[linker] improve method detection in FixAbstractMethodsStep by radekdoulik · Pull Request #861 · dotnet/android

radekdoulik · 2017-09-15T09:08:17Z

The forms team run into an issue with debugger in VS [1]. During
investigation it turned out that our FixAbstractMethodsStep injects 2
methods, which were not needed, as the methods were already
implemented. The bug itself looks like an issue, where original mdb
file was used in the apk, instead of the one written by our linker.

This change improves the method detection, where method name starts
with global::. In this particular case it were these methods:

global::Android.Views.View.IOnTouchListener.OnTouch
global::Android.Views.View.IOnClickListener.OnClick

[1] https://bugzilla.xamarin.com/show_bug.cgi?id=59293

jonpryor · 2017-09-15T15:03:34Z

As discussed elsewhere, I do not like explicit string comparison, as it's likely to be wrong. For example, the IL name of explicitly-implemented interface methods is compiler-specific, so even if global::Android.Views.View.IOnTouchListener.OnTouch works now, it very likely will not work with e.g. a VB.NET-generated assembly or F#-generated assembly.

Instead, the fix should probably involve MethodDefinition.Overrides, and check to see if the MethodDefinition overrides the interface's MethodDefinition.

dellis1972 · 2017-09-18T17:02:32Z

build

radekdoulik · 2017-09-18T18:15:41Z

I will also extend the test case. I need to fix a linker bug first though.

radekdoulik · 2017-09-21T10:22:06Z

build

radekdoulik · 2017-09-26T09:20:01Z

added unit test

jonpryor · 2017-09-28T14:57:49Z

 }
+
+ var mi = ic.GetType ().GetMethod ("MethodWithCursor");
+ var mi2 = ic.GetType ().GetMethod ("global::Test.Bindings.ICursor.MethodWithCursor");


This is compiler-specific, and will fail if/when the compiler changes.
For example, given:
classT:IEnumerable{IEnumeratorglobal::System.Collections.IEnumerable.GetEnumerator(){returnnull;}}
When compiling the above with mcs, we get:
.methodprivatehidebysignewslotvirtualfinalinstanceclass[mscorlib]System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() cilmanaged
compare to csc (Roslyn):
.methodprivatehidebysignewslotvirtualfinalinstanceclass[mscorlib]System.Collections.IEnumerator 'global::System.Collections.IEnumerable.GetEnumerator'() cilmanaged
Use mcs, and this test will break. That's not a good test.

The forms team run into an issue with debugger in VS [1]. During investigation it turned out that our FixAbstractMethodsStep injects 2 methods, which were not needed, as the methods were already implemented. The bug itself looks like an issue, where original mdb file was used in the apk, instead of the one written by our linker. This change improves the method detection, where explicit interface methods name starts with `global::` [2]. In this particular case it were these methods: global::Android.Views.View.IOnTouchListener.OnTouch global::Android.Views.View.IOnClickListener.OnClick This is possibly compiler specific, so we rather look in MethodDefinition's Overrides to find the interface method and compare it to the interface method we are looking for. [1] https://bugzilla.xamarin.com/show_bug.cgi?id=59293 [2] excerpt from ikdasm output: .method private hidebysig newslot virtual final instance void 'global::Android.Views.View.IOnClickListener.OnClick'(class [Mono.Android]Android.Views.View v) cil managed { .override [Mono.Android]Android.Views.View/IOnClickListener::OnClick

radekdoulik · 2017-10-16T14:11:06Z

build

…t#861)_wow Context: https://bugzilla.xamarin.com/show_bug.cgi?id=22074 Context: https://bugzilla.xamarin.com/show_bug.cgi?id=59293 Context: https://developer.xamarin.com/releases/android/xamarin.android_6/xamarin.android_6.1/#Improved_Java_Interface_Versioning_Support **Background**: Until [C# gains support for default interface methods][ifaces], there is an "impedance mismatch" between Java interfaces and C# interfaces: Java methods can be added to interfaces *without* requiring that existing classes implementing those interfaces implement the "new" methods. (*This is not* Java default interface methods!) [ifaces]: https://github.com/dotnet/csharplang/blob/master/proposals/default-interface-methods.md Consider [`android.database.Cursor`][cursor]: [cursor]: https://developer.android.com/reference/android/database/Cursor.html ```java // API-1 interface Cursor { void close(); // ... } class MyCursor implements Cursor { public void close() {} } ``` The `Cursor` interface had several methods added to it over time -- *before* the Android SDK supported default interface methods! -- e.g. the addition of `Cursor.getType()` in API-11: ```java interface Cursor { // in API-1 void close(); // ... // Added in API-11 int getType(int columnIndex); } ``` Question: What happens when `MyCursor.getType()` is invoked, if/when `MyCursor` *has not* been recompiled? ```java Cursor c = new MyCursor(); // Implements API-1 API, *not* API-11! c.getType(0); // Method added in API-11! ``` What happens is that an [`AbstractMethodError`][ame] is thrown. [ame]: https://developer.android.com/reference/java/lang/AbstractMethodError.html Compare to C#, in which *the type cannot be loaded* if an interface has a method added: [ERROR] FATAL UNHANDLED EXCEPTION: System.TypeLoadException: VTable setup of type MyCursor failed What this meant is [Bug #22074][b22074]: if you have a Xamarin.Android library, and that library: 1. Has `$(TargetFrameworkVersion)`=v2.3 (API-10), and 2. Has a type which implements `Android.Database.ICursor` and you then use the type (2) in an App which has `$(TargetFrameworkVersion)`=v4.4, the app would crash with a `TypeLoadException`. [b22074]: https://bugzilla.xamarin.com/show_bug.cgi?id=22074 The fix, [introduced in Xamarin.Android 6.1][xa6.1], is the `FixAbstractMethodsStep` step, which looks at every type in every assembly to be included in the `.apk`, and checks to see if that type is "missing" any abstract methods. If any such type is found, then the "missing" method is *inserted*, and it will throw an `AbstractMethodError`. [xa6.1]: https://developer.xamarin.com/releases/android/xamarin.android_6/xamarin.android_6.1/#Improved_Java_Interface_Versioning_Support For example, if an assembly `Lib.dll` built against API-10 has: ```csharp public class MyCursor : Java.Lang.Object, Android.Database.ICursor { // Implement `ICursor` methods from API-10 } ``` then when `Lib.dll` is packaged as part of an API-11+ app the `FixAbstractMethodsStep` step will *add* the following method: ```csharp int ICursor.GetType(int columnIndex) { throw new Java.Lang.AbstractMethodError(); } ``` **The Problem**: The Xamarin.Forms team ran into an issue with [the debugger in Visual Studio][b59293]. During investigation it turned out that our `FixAbstractMethodsStep` injects two methods which were not needed, as the methods were already implemented. [b59293]: https://bugzilla.xamarin.com/show_bug.cgi?id=59293 **The Fix**: Improve "missing abstract method" detection by looking in `MethodDefinition.Overrides` to find the interface method and compare it to the interface method we are looking for, not just using the method name. This will be more resilient to compiler changes -- not all compilers emit the same name for explicitly implemented interface methods -- and prevent us from emitting explicit interface methods when they're already present.

Context: #6131 (review) Context: fcb9ea3 Changes: http://github.com/xamarin/Java.Interop/compare/4fb7c147f8c6eb9bf94d9bfb8305c7d2a7a9fb33...dd1ef455ee4fbfa7e17f34c51cbe2ef24459e2e6 * dotnet/java-interop@dd1ef455: Bump to mono/linker/main@b888d67 Mono.Cecil 0.11.4 (#861) Commit fcb9ea3 didn't fix the build breakage. Bump to xamarin/Java.Interop/main@dd1ef455 and set `$(_XamarinAndroidCecilVersion)`, which will override the Mono.Cecil NuGet package version within the Java.Interop build. This will hopefully ensure/allow everything to now use Cecil 0.11.4. TODO: This Quick-And-Dirty approach hardcodes `$(_XamarinAndroidCecilVersion)`=0.11.4, meaning we now have two different properties for the same thing: Directory.Build.props: <MonoCecilVersion>0.11.4</MonoCecilVersion> build-tools/scripts/Configuration.Java.Interop.Override.props: <_XamarinAndroidCecilVersion>0.11.4</_XamarinAndroidCecilVersion> This is "undesirable". Is there a way to update `xaprepare` so that we can generate `external/Java.Interop/Configuration.Override.props` so that `$(_XamarinAndroidCecilVersion)`=`$(MonoCecilVersion)`? (Assuming that this approach even works…)

Changes: dotnet/installer@cc10fae...3b43390 Changes: dotnet/linker@6eae019...0cb9250 Changes: dotnet/runtime@d019e70...cf52b7e Updates: * Microsoft.Dotnet.Sdk.Internal: from 6.0.100-rc.1.21376.3 to 6.0.100-rc.1.21379.2 * Microsoft.NET.ILLink.Tasks: from 6.0.100-preview.6.21370.1 to 6.0.100-preview.6.21378.1 * Microsoft.NETCore.App.Ref: from 6.0.0-rc.1.21374.7 to 6.0.0-rc.1.21378.2 Bump to Mono.Cecil 0.11.4. Hopefully fixes a [build break][0]: CSC : error CS1705: Assembly 'illink' with identity 'illink, Version=6.0.100.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' uses 'Mono.Cecil, Version=0.11.4.0, Culture=neutral, PublicKeyToken=50cebf1cceb9d05e' which has a higher version than referenced assembly 'Mono.Cecil' with identity 'Mono.Cecil, Version=0.11.3.0, Culture=neutral, PublicKeyToken=50cebf1cceb9d05e' Bump `$(MonoCecilVersion)` to compensate? Note: an actual fix very likely requires a Java.Interop bump, as Java.Interop references Cecil 0.11.3 (same as the error message) while `$(MonoCecilVersion)` is 0.11.2 (not present in the error). [0]: #6131 (review) Context: #6131 (review) Context: fcb9ea3 Changes: http://github.com/xamarin/Java.Interop/compare/4fb7c147f8c6eb9bf94d9bfb8305c7d2a7a9fb33...dd1ef455ee4fbfa7e17f34c51cbe2ef24459e2e6 * dotnet/java-interop@dd1ef45: Bump to dotnet/linker@b888d67 Mono.Cecil 0.11.4 (#861) Commit fcb9ea3 didn't fix the build breakage. Bump to dotnet/java-interop@dd1ef45 and set `$(_XamarinAndroidCecilVersion)`, which will override the Mono.Cecil NuGet package version within the Java.Interop build. This will hopefully ensure/allow everything to now use Cecil 0.11.4. TODO: This Quick-And-Dirty approach hardcodes `$(_XamarinAndroidCecilVersion)`=0.11.4, meaning we now have two different properties for the same thing: Directory.Build.props: <MonoCecilVersion>0.11.4</MonoCecilVersion> build-tools/scripts/Configuration.Java.Interop.Override.props: <_XamarinAndroidCecilVersion>0.11.4</_XamarinAndroidCecilVersion> This is "undesirable". Is there a way to update `xaprepare` so that we can generate `external/Java.Interop/Configuration.Override.props` so that `$(_XamarinAndroidCecilVersion)`=`$(MonoCecilVersion)`? (Assuming that this approach even works…) Co-authored-by: Jonathan Pryor <jonpryor@vt.edu>

dnfclas added the cla-already-signed label Sep 15, 2017

radekdoulik force-pushed the pr-improve-method-detection branch from 4f63cbb to 528219bCompare September 18, 2017 13:49

radekdoulik requested a review from jonpryor September 19, 2017 06:29

radekdoulik force-pushed the pr-improve-method-detection branch from 528219b to 5e0a0d8Compare September 19, 2017 10:16

radekdoulik force-pushed the pr-improve-method-detection branch from 5e0a0d8 to 8a6f16eCompare September 26, 2017 09:19

jonpryor reviewed Sep 28, 2017
View reviewed changes

radekdoulik force-pushed the pr-improve-method-detection branch from 8a6f16e to e4848bdCompare October 13, 2017 13:27

jonpryor merged commit f96fcf9 into dotnet:masterOct 16, 2017

jonpryor mentioned this pull request Oct 17, 2018
[jnimarshalmethod-gen] Avoid creating duplicate marshal methods dotnet/java-interop#385
Merged

jonpryor mentioned this pull request Feb 15, 2020
[linker] remove unneeded string.Format calls #4260
Merged

github-actionsBot locked and limited conversation to collaborators Feb 2, 2024

Navigation Menu

Uh oh!

[linker] improve method detection in FixAbstractMethodsStep - #861

[linker] improve method detection in FixAbstractMethodsStep#861
jonpryor merged 1 commit into
dotnet:masterfrom
radekdoulik:pr-improve-method-detection

radekdoulik commented Sep 15, 2017

Uh oh!

jonpryor commented Sep 15, 2017

Uh oh!

dellis1972 commented Sep 18, 2017

Uh oh!

radekdoulik commented Sep 18, 2017

Uh oh!

radekdoulik commented Sep 21, 2017

Uh oh!

radekdoulik commented Sep 26, 2017

Uh oh!

jonpryorSep 28, 2017

Uh oh!

radekdoulik commented Oct 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

radekdoulik commented Sep 15, 2017

Uh oh!

jonpryor commented Sep 15, 2017

Uh oh!

dellis1972 commented Sep 18, 2017

Uh oh!

radekdoulik commented Sep 18, 2017

Uh oh!

radekdoulik commented Sep 21, 2017

Uh oh!

radekdoulik commented Sep 26, 2017

Uh oh!

jonpryorSep 28, 2017

Choose a reason for hiding this comment

Uh oh!

radekdoulik commented Oct 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants