Assert matching method parameters in class-parse

[jpobst]

• Download org.tensorflow:tensorflow-android version 1.13.1 from Maven:
  • https://mvnrepository.com/artifact/org.tensorflow/tensorflow-android/1.13.1
• Extract the classes.jar from the .aar
• Run it through class-parse

It will hit the Debug.Assert in Methods.UpdateParametersFromMethodParametersAttribute (...):

Unexpected number of method parameters; expected 0, got 1
   at Xamarin.Android.Tools.Bytecode.MethodInfo.UpdateParametersFromMethodParametersAttribute(ParameterInfo[] parameters) in C:\code\xamarin-android\external\Java.Interop\src\Xamarin.Android.Tools.Bytecode\Methods.cs:line 305
   at Xamarin.Android.Tools.Bytecode.MethodInfo.GetParameters() in C:\code\xamarin-android\external\Java.Interop\src\Xamarin.Android.Tools.Bytecode\Methods.cs:line 92
   at Xamarin.Android.Tools.Bytecode.XmlClassDeclarationBuilder.GetMethodParameters(MethodInfo method)+MoveNext() in C:\code\xamarin-android\external\Java.Interop\src\Xamarin.Android.Tools.Bytecode\XmlClassDeclarationBuilder.cs:line 389
   at System.Xml.Linq.XContainer.AddContentSkipNotify(Object content)
   at System.Xml.Linq.XContainer.AddContentSkipNotify(Object content)
   at Xamarin.Android.Tools.Bytecode.XmlClassDeclarationBuilder.GetMethod(String element, String name, MethodInfo method, String returns) in C:\code\xamarin-android\external\Java.Interop\src\Xamarin.Android.Tools.Bytecode\XmlClassDeclarationBuilder.cs:line 338
   at Xamarin.Android.Tools.Bytecode.XmlClassDeclarationBuilder.<GetConstructors>b__23_2(MethodInfo c) in C:\code\xamarin-android\external\Java.Interop\src\Xamarin.Android.Tools.Bytecode\XmlClassDeclarationBuilder.cs:line 321
...

This works in the class-parse we ship in .NET 6 because it works today in the GooglePlayServices repository CI, so it's a regression since then.

Perhaps it was introduced in 8ccb837.

[jonpryor]

Oddly, commit 8ccb837 is in the release/6.0.3xx branch, but was reverted in the release/6.0.1xx-preview10 branch.

@jonathanpeppers: do you remember the background for the revert in 6.0.1xx-preview10? 2229ccc

xamarin-android on release/6.0.4xx is using commit 2a882d2d, which does contain 8ccb837 and does not contain the revert in 2229ccc, so as far as I can tell .NET 6 currently contains 8ccb837.

[jonathanpeppers]

I'm not sure why I didn't put that information here: dotnet/android#6426

My memory is there was some error caused by it, and we just wanted to revert for this one release.

But then I don't see a java.interop commit that "fixes" something caused by 8ccb837.

[jonpryor]

"Repro steps":

curl -o tensorflow-android-1.13.1.aar https://repo1.maven.org/maven2/org/tensorflow/tensorflow-android/1.13.1/tensorflow-android-1.13.1.aar
unzip tensorflow-android-1.13.1.aar classes.jar
dotnet path/to/class-parse.dll classes.jar
# boom

The assert is triggered when processing the constructor for org/tensorflow/Session$Runner$Reference. class-parse.exe --dump shows:

…
ThisClass: Utf8("org/tensorflow/Session$Runner$Reference")
…
Methods Count: 2
	0: <init> (Lorg/tensorflow/Session$Runner;)V Public
		Code(60, Unknown[LineNumberTable](30), LocalVariableTableAttribute(LocalVariableTableEntry(Name='this', Descriptor='Lorg/tensorflow/Session$Runner$Reference;', StartPC=0, Index=0)), Xamarin.Android.Tools.Bytecode.StackMapTableAttribute)
		MethodParametersAttribute(MethodParameterInfo(Name='this$1', AccessFlags=Final, Synthetic))

which is to say that the only constructor accepts 1 parameter, but that parameter is named this. What is (probably) happening is Session.Runner.Reference is a non-static inner class of Session.Runner, and as such its constructor requires a reference of its enclosing type.

% javap -cp . 'org/tensorflow/Session$Runner$Reference.class'
Compiled from "Session.java"
class org.tensorflow.Session$Runner$Reference implements java.lang.AutoCloseable {
  final org.tensorflow.Session$Runner this$1;
  public org.tensorflow.Session$Runner$Reference(org.tensorflow.Session$Runner);
  public void close();
}

% dotnet path/to/class-parse.exe --dump 'org/tensorflow/Session$Runner.class'
.class version: 52.0
…
Attributes Count: 2
        0: SourceFile('Session.java')
        1: InnerClasses(Count=3,
          InnerClass(InnerClass='org/tensorflow/Session$Runner', OuterClass='org/tensorflow/Session', InnerName='Runner', InnerClassAccessFlags=Public, Final),
          InnerClass(InnerClass='org/tensorflow/Session$Runner$Reference', OuterClass='org/tensorflow/Session$Runner', InnerName='Reference', InnerClassAccessFlags=Private),
          InnerClass(InnerClass='org/tensorflow/Session$Run', OuterClass='org/tensorflow/Session', InnerName='Run', InnerClassAccessFlags=Public, Static, Final))
…

Aside: .class version 52.0 is JDK 1.8.

So we look at this block of code: https://github.com/xamarin/java.interop/blob/909239d4b70e5367e2d47e6ce60a2b399830324a/src/Xamarin.Android.Tools.Bytecode/Methods.cs#L297-L307

Turns Out™ that for the "offending" method, pinfo[0].AccessFlags is Final, Synthetic (0x1010), while what we were looking for -- in the form of OuterThis -- was Final, Mandated (0x8010). The actual value is what we expected for a "outer this reference", so we didn't skip it, which caused the assert to fire.

[jonpryor]

I don't know why tensorflow is using Final, Synthetic instead of Final, Mandated. I suspect a compiler bug of some form?

[jpobst]

Another example is https://mvnrepository.com/artifact/com.google.crypto.tink/tink-android/1.8.0.

However, this appears to be the same issue, as it is also Final, Synthetic.