Skip to content
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.

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

[generator] Fix StackOverflow when copying DIM from package-protected interfaces #1261

Merged
merged 7 commits into from
Oct 21, 2024

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Sep 30, 2024

Context: xamarin/AndroidX#988
Context: #1080

Imagine you bind the following Java interfaces:

package io.grpc;

public interface InternalConfigurator extends Configurator {}

interface Configurator {
  default void configureChannelBuilder(ManagedChannelBuilder<?> channelBuilder) {}
}

Because the Configurator interface is package-protected, its method is copied into the public InternalConfigurator interface via generator's FixupAccessModifiers code.

Later, we look for the "base" method that the copied InternalConfigurator.configureChannelBuilder overrides, which is Configurator.configureChannelBuilder. However, because we simply added the same method instance to two types, this method reference is actually pointing to itself.

Eventually we hit a StackOverflowException when trying to retrieve the overridden method's declaring type:

static string GetDeclaringTypeOfExplicitInterfaceMethod (Method method)
{
return method.OverriddenInterfaceMethod != null ?
GetDeclaringTypeOfExplicitInterfaceMethod (method.OverriddenInterfaceMethod) :
method.DeclaringType.FullName;
}

Fix this by cloning the method when we "copy" it, rather than having two types reference the same Method instance.

@jpobst jpobst marked this pull request as ready for review October 1, 2024 22:33
@jpobst jpobst requested a review from jonpryor October 1, 2024 22:33
Context: 1adb796
Context: #1183

While reviewing #1261, I noticed this change:

```diff
diff --git a/tests/generator-Tests/expected.ji/AccessModifiers/Xamarin.Test.IExtendedInterface.cs b/tests/generator-Tests/expected.ji/AccessModifiers/Xamarin.Test.IExtendedInterface.cs
index 3ea50e6bd..83d01f24a 100644
--- a/tests/generator-Tests/expected.ji/AccessModifiers/Xamarin.Test.IExtendedInterface.cs
+++ b/tests/generator-Tests/expected.ji/AccessModifiers/Xamarin.Test.IExtendedInterface.cs
@@ -46,7 +46,7 @@ public unsafe void BaseMethod ()
 		{
 			const string __id = "baseMethod.()V";
 			try {
-				_members_xamarin_test_BaseInterface.InstanceMethods.InvokeAbstractVoidMethod (__id, this, null);
+				_members_xamarin_test_ExtendedInterface.InstanceMethods.InvokeAbstractVoidMethod (__id, this, null);
 			} finally {
 			}
 		}
```

and went "hmm".

Recall and consider 1adb796: if we invoke the interface method on
the "wrong" declaring type, things break.  This is why 1adb796
updated interface invokers to only invoke methods upon their declared
interfaces.

The concern comes around *non-`public`* interface method invocation:

	/* package */ interface PackagePrivateInterface {
	    void m();
	}
	public interface PublicInterface extends PackagePrivateInterface {
	}

With commit 63dcf36, attempts to invoke `m()` are made upon
`PublicInterface`, not `PackagePrivateInterface`.

Is this a problem?

Begin partially implementing #1183, adding a new
`build-tools/Java.Interop.Sdk` (not quite a) "project", which has an
`Sdk.props` and `Sdk.targets` file, which combined add support for:

  * A `@(JavaCompile)` build action, for Java source.
  * A `@(JavaReference)` build action, for Java libraries.
  * As a pre-Compile step, files with `%(JavaCompile.Bind)`=True or
    `%(JavaReference.Bind)`=True will be automatically bound, via
    `class-parse` + `generator`.
  * As a post-Compile step, the assembly will be processed with
    `jcw-gen` to generate Java Callable Wrappers, which will be
    compiled into `$(AssemblyName).jar`.

Then, update `tests/Java.Base-Tests` to use this new functionality,
adding a test case to hit the "invoke method declared in a private
interface upon the public interface" scenario.

Result: it works!

Of partial interest is `IInterfaceMethodInheritanceInvoker`:

	internal partial class IInterfaceMethodInheritanceInvoker : global::Java.Lang.Object, IInterfaceMethodInheritance {
	    static readonly JniPeerMembers _members_net_dot_jni_test_BaseInterface = new JniPeerMembers ("net/dot/jni/test/BaseInterface", typeof (IInterfaceMethodInheritanceInvoker));
	    static readonly JniPeerMembers _members_net_dot_jni_test_InterfaceMethodInheritance = new JniPeerMembers ("net/dot/jni/test/InterfaceMethodInheritance", typeof (IInterfaceMethodInheritanceInvoker));
	    static readonly JniPeerMembers _members_net_dot_jni_test_InternalInterface = new JniPeerMembers ("net/dot/jni/test/InternalInterface", typeof (IInterfaceMethodInheritanceInvoker));
	    static readonly JniPeerMembers _members_net_dot_jni_test_PublicInterface = new JniPeerMembers ("net/dot/jni/test/PublicInterface", typeof (IInterfaceMethodInheritanceInvoker));
	}

Of these four fields, two are for internal types:
`_members_net_dot_jni_test_BaseInterface` and
`_members_net_dot_jni_test_InternalInterface`.

Fortunately those types aren't otherwise used.

Of concern, though, is that the constructor invocation *does* result
in a `JNIEnv::FindClass()` invocation, meaning these bindings would
look up (ostensibly) "private" types, which could change!

This presents a compatibility concern: if (when?) those type names
change, then the generated bindings will break.

TODO:

  * Test this puppy in dotnet/android.  Just because "it works" on
    Desktop JDK doesn't mean it does *on Android*.
  * Update `generator` output to *not* emit the
    `static readonly JniPeerMembers` fields for internal types.
Commit 12c9c82 didn't work properly in a "from clean" build:

	git clean -xdff
	git submodule foreach --recursive git clean -xdff
	git submodule update --init --recursive

	dotnet build -c Release -t:Prepare *.sln
	dotnet build -c Release *.sln

	dotnet test bin/TestRelease-net8.0/Java.Base-Tests.dll

failed with:

	Failed InterfaceMethod [71 ms]
	Error Message:
	 Java.Interop.JavaException : net/dot/jni/test/HasInterfaceMethodInheritance
	----> Java.Interop.JavaException : net.dot.jni.test.HasInterfaceMethodInheritance
	Stack Trace:
	   at Java.Interop.JniEnvironment.Types.TryFindClass(String classname, Boolean throwOnError) in /Users/runner/work/1/s/src/Java.Interop/Java.Interop/JniEnvironment.Types.cs:line 89
	 at Java.Interop.JniEnvironment.Types.FindClass(String classname) in /Users/runner/work/1/s/src/Java.Interop/Java.Interop/JniEnvironment.Types.cs:line 37
	 …

The cause of the failure is that `java.base-tests.jar` did not exist.

`java.base-tests.jar` did not exist because `_JavaCreateJcws` & co
didn't execute!

	<Target Name="_JavaCreateJcws"
	    Condition=" '$(TargetPath)' != '' And Exists($(TargetPath))"
	    Inputs="$(TargetPath)"
	    Outputs="$(_JavaJcwSourcesDir).stamp">

didn't execute, as per the build log:

	Skipping target "_JavaCreateOutputJar" because it has no inputs.

It had no inputs because `$(TargetPath)` didn't exist, and it didn't
exist because `$(TargetPath)` (typically) exists in `$(OutputPath)`,
and doesn't exist until after the `Build` target.

`_JavaCreateOutputJar`, meanwhile, was executing after `CoreCompile`.
*An* assembly existed, but it was in `$(IntermediateOutputPath)`,
not `$(OutputPath)`, and thus `$(TargetPath)` did not yet exist.

Rename the `JavaCreateJavaCallableWrappers` target to
`JavaCreateOutputJar`, as I like `JavaCreateOutputJar` better, and
update `AfterTargets` so that it happens after Build.  This ensures
that `$(TargetPath)` exists when `_JavaCreateOutputJar` is executed,
which in turn ensures that `java.base-tests.jar` is created.
This should allow unit tests to complete successfully.
For reasons I'm not going to investigate, if an interface type is
defined in the assembly that `jnimarshalmethod-gen` is processing,
`Assembly.Location` will be the empty string, which causes an error:

	% dotnet bin/Release-net8.0/jnimarshalmethod-gen.dll bin/TestRelease-net8.0/Java.Base-Tests.dll -v -v --keeptemp
	…
	error JM4006: jnimarshalmethod-gen: Unable to process assembly 'bin/TestRelease-net8.0/Java.Base-Tests.dll'
	Name can not be empty
	System.ArgumentException: Name can not be empty
	   at Mono.Cecil.AssemblyNameReference.Parse(String fullName)
	   at Java.Interop.Tools.Cecil.DirectoryAssemblyResolver.Resolve(String fullName, ReaderParameters parameters) in /Users/runner/work/1/s/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs:line 261
	   at Java.Interop.Tools.Cecil.DirectoryAssemblyResolver.Resolve(String fullName) in /Users/runner/work/1/s/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs:line 256
	   at Java.Interop.Tools.Cecil.DirectoryAssemblyResolver.GetAssembly(String fileName) in /Users/runner/work/1/s/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs:line 251
	   at Xamarin.Android.Tools.JniMarshalMethodGenerator.Extensions.NeedsMarshalMethod(MethodDefinition md, DirectoryAssemblyResolver resolver, TypeDefinitionCache cache, MethodInfo method, String& name, String& methodName, String& signature) in /Users/runner/work/1/s/tools/jnimarshalmethod-gen/App.cs:line 790
	   at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.CreateMarshalMethodAssembly(String path) in /Users/runner/work/1/s/tools/jnimarshalmethod-gen/App.cs:line 538
	   at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.ProcessAssemblies(List`1 assemblies) in /Users/runner/work/1/s/tools/jnimarshalmethod-gen/App.cs:line 285

Update `App.NeedsMarshalMethod()` so that when the assembly Location
is not present, `DirectoryAssemblyResolver.Resolve(assemblyName)`
is instead used.  This prevents the `ArgumentException`.
Windows is failing to build the solution!

	  error BG0000: System.InvalidOperationException: A .xml file must be specified.
	     at Xamarin.Android.Binder.CodeGeneratorOptions.Parse(String[] args) in D:\a\_work\1\s\tools\generator\CodeGeneratorOptions.cs:line 193
	     at Xamarin.Android.Binder.CodeGenerator.Main(String[] args) in D:\a\_work\1\s\tools\generator\CodeGenerator.cs:line 29
	##[error]build-tools\Java.Interop.Sdk\Sdk\Sdk.targets(147,5): Error MSB3073: The command "dotnet "D:\a\_work\1\s\bin\Release-net8.0\generator.dll" --public --global  -o "obj\\Release-net8.0\_ji\mcw\" -L "D:\a\_work\1\s\src\Java.Base\bin\Release\ref\." -L "D:\a\_work\1\s\bin\Release-net8.0\ref\." -L "D:\a\_work\1\s\bin\TestRelease-net8.0\ref\." -L "C:\hostedtoolcache\windows\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.7\ref\net8.0\." -L "C:\Users\cloudtest\.nuget\packages\microsoft.testplatform.testhost\17.5.0-preview-20221003-04\lib\netcoreapp3.1\." -L "C:\Users\cloudtest\.nuget\packages\microsoft.codecoverage\17.5.0-preview-20221003-04\lib\netcoreapp3.1\." -L "C:\Users\cloudtest\.nuget\packages\mono.options\6.12.0.148\lib\netstandard2.0\." -L "C:\Users\cloudtest\.nuget\packages\newtonsoft.json\13.0.1\lib\netstandard2.0\." -L "C:\Users\cloudtest\.nuget\packages\nuget.frameworks\5.11.0\lib\netstandard2.0\." -L "C:\Users\cloudtest\.nuget\packages\nunit\3.13.2\lib\netstandard2.0\." -L "D:\a\_work\1\s\external\xamarin-android-tools\bin\Release\net6.0\ref\." -r "D:\a\_work\1\s\src\Java.Base\bin\Release\ref\Java.Base.dll" --codegen-target=JavaInterop1 "--assembly=Java.Base-Tests" --type-map-report=obj\\Release-net8.0\_ji\mcw\type-mapping.txt --lang-features=nullable-reference-types,default-interface-methods,nested-interface-types,interface-constants --enumdir=obj\\Release-net8.0\_ji\mcw\ obj\\Release-net8.0\_ji\mcw\api.xml " exited with code 1.
	D:\a\_work\1\s\build-tools\Java.Interop.Sdk\Sdk\Sdk.targets(147,5): error MSB3073: The command "dotnet "D:\a\_work\1\s\bin\Release-net8.0\generator.dll" --public --global  -o "obj\\Release-net8.0\_ji\mcw\" -L "D:\a\_work\1\s\src\Java.Base\bin\Release\ref\." -L "D:\a\_work\1\s\bin\Release-net8.0\ref\." -L "D:\a\_work\1\s\bin\TestRelease-net8.0\ref\." -L "C:\hostedtoolcache\windows\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.7\ref\net8.0\." -L "C:\Users\cloudtest\.nuget\packages\microsoft.testplatform.testhost\17.5.0-preview-20221003-04\lib\netcoreapp3.1\." -L "C:\Users\cloudtest\.nuget\packages\microsoft.codecoverage\17.5.0-preview-20221003-04\lib\netcoreapp3.1\." -L "C:\Users\cloudtest\.nuget\packages\mono.options\6.12.0.148\lib\netstandard2.0\." -L "C:\Users\cloudtest\.nuget\packages\newtonsoft.json\13.0.1\lib\netstandard2.0\." -L "C:\Users\cloudtest\.nuget\packages\nuget.frameworks\5.11.0\lib\netstandard2.0\." -L "C:\Users\cloudtest\.nuget\packages\nunit\3.13.2\lib\netstandard2.0\." -L "D:\a\_work\1\s\external\xamarin-android-tools\bin\Release\net6.0\ref\." -r "D:\a\_work\1\s\src\Java.Base\bin\Release\ref\Java.Base.dll" --codegen-target=JavaInterop1 "--assembly=Java.Base-Tests" --type-map-report=obj\\Release-net8.0\_ji\mcw\type-mapping.txt --lang-features=nullable-reference-types,default-interface-methods,nested-interface-types,interface-constants --enumdir=obj\\Release-net8.0\_ji\mcw\ obj\\Release-net8.0\_ji\mcw\api.xml " exited with code 1. [D:\a\_work\1\s\tests\Java.Base-Tests\Java.Base-Tests.csproj]

My guess is that because I have `$(_JavaManagedBindingDir)` set to
end with `.`, it's "escaping" anything that follows it, e.g.
`-o "$(_JavaManagedBindingDir)"` becomes
`-o "obj\Release-net8.0\_ji\mcw\"`., and `\"` escapes the `"`.
Plausible result: the entire command-line no longer makes sense.

Repeat the often repeated "I can't find a good way to trim the trailing `\`"
solution by appending a `.` to the end of `$(_JavaManagedBindingDir)`
whenever `"` follows; that is, replace:

	$(_JavaManagedBindingDir)"

with

	$(_JavaManagedBindingDir)."

Additionally, ensure that all usage of `$(_JavaManagedBindingDir)`
is quoted, in case it ever contains a space.

Does It Build™?
Same but different as 611cc3f, this time with jcw-gen.

Replace `"$(_JavaJcwSourcesDir)"` with `"$(_JavaJcwSourcesDir)."`.
@jpobst
Copy link
Contributor Author

jpobst commented Oct 14, 2024

TODO from commit:

  • Test this puppy in dotnet/android. Just because "it works" on Desktop JDK doesn't mean it does on Android.
  • Update generator output to not emit the static readonly JniPeerMembers fields for internal types.

jonpryor added a commit to dotnet/android that referenced this pull request Oct 14, 2024
Does It Build™?
@jonpryor
Copy link
Member

See also: dotnet/android#9397

@jonpryor
Copy link
Member

"This puppy" has been tested on dotnet/android#9397, and is green. The approach works, and (equally important) the build system changes don't break things in dotnet/android.

We still need to "Update generator output to not emit the static readonly JniPeerMembers fields for internal types."

@jonpryor
Copy link
Member

Draft commit message:

Context: https://github.com/xamarin/AndroidX/issues/988
Context: 9e0a4690adb71c04cd88a5b54bea3c3f8da73cc0
Context: https://github.com/dotnet/java-interop/issues/1269

Imagine you bind the following Java interfaces:

	// Java
	package io.grpc;

	interface Configurator {
	  default void configureChannelBuilder(ManagedChannelBuilder<?> channelBuilder) {}
	}

	public interface InternalConfigurator extends Configurator {
	}

Because the `Configurator` interface is package-protected, its method
is copied into the `public` `InternalConfigurator` interface via
`generator`'s `FixupAccessModifiers()` code.

Later, we look for the "base" method that the copied
`InternalConfigurator.configureChannelBuilder()` overrides, which is
`Configurator.configureChannelBuilder`.  However, because we simply
added the same method instance to two types, this method reference is
actually pointing to itself.  

Eventually we hit a StackOverflowException when trying to retrieve the
[overridden method's declaring type][0].

Fix this by _cloning_ the method when we "copy" it, rather than having
two types reference the same `Method` instance.

[0]: https://github.com/dotnet/java-interop/blob/9d997232f0ce3ca6a5f788b1cdb0fb9b2f978c84/tools/generator/SourceWriters/BoundMethod.cs#L95-L100

@jonpryor jonpryor merged commit 2a1e180 into main Oct 21, 2024
4 checks passed
@jonpryor jonpryor deleted the clone-dim branch October 21, 2024 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants