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

[jcw-gen, jnimarshalmethod-gen] Native method consistency #1160

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented Nov 11, 2023

Context: #1153
Context: 4787e01
Context: 77800dd

PR #1153 is exploring the use of .NET Native AOT to produce a
native library which is used within a java-originated process:

% java -cp … com/microsoft/hello_from_jni/App
# launches NativeAOT-generated native lib, executes C# code…

As NativeAOT has no support for System.Reflection.Emit, the only
way for Java code to invoke managed code -- in a Desktop Java.Base
environment! 1 see 4787e01 -- would be to pre-generate the
required marshal methods via jnimarshalmethod-gen.

This in turn requires updating jcw-gen to support the pre-existing
Java.Interop.JavaCallableAttribute, so that C# code could
reasonably declare methods visible to Java, along with the
introduction of, and support for, a new
Java.Interop.JavaCallableConstructorAttribute type. This allows
straightforward usage:

[JniTypeSignature ("example/ManagedType")]      // for a nice Java name!
class ManagedType : Java.Lang.Object {

    int value;

    [JavaCallableConstructor(SuperConstructorExpression="")]
    public ManagedType (int value) {
        this.value = value;
    }

    [JavaCallable ("getString")]
    public Java.Lang.String GetString () {
        return new Java.Lang.String ($"Hello from C#, via Java.Interop! Value={value}");
    }
}

Run this through jcw-gen and jnimarshalmethod-gen, run the app,
and nothing worked (?!), because not all pieces were in agreement.

Java native method registration is One Of Those Things™ that
involves lots of moving pieces:

  • generator emits bindings for Java types, which includes Java
    method names, signatures, and (on .NET Android) the "connector
    method" to use:

    [Register ("toString", "()Ljava/lang/String;", "GetToStringHandler")]   // .NET Android
    [JniMethodSignature ("toString", "()Ljava/lang/String;")]               // Java.Base
    public override unsafe string? ToString () {…}
    
  • jcw-gen uses generator output, prefixing Java method names
    with n_ for native method declarations, along with a
    method wrapper 2

    public String toString() {return n_toString();}
    private native String n_toString();
    
  • jnimarshalmethod-gen emits marshal methods for Java.Base,
    and needs to register the native methods declared by jcw-gen.
    jnimarshalmethod-gen and jcw-gen need to be consistent with
    each other.

  • MarshalMemberbuilder.CreateMarshalToManagedMethodRegistration()
    creates a JniNativeMethodRegistration instance which contains
    the name of the Java native method to register, and was
    using a name inconsistent with jcw-gen.

Turns Out, jcw-gen, jnimarshalmethod-gen, and
MarshalMemberBuilder were not consistent.

The only "real" jnimarshalmethod-gen usage (77800dd) is with the
Java.Interop.Export-Tests unit test assembly, which did not use
jcw-gen; it contained only hand-written Java code. Consequently,
none of the Java native methods declared within it had an n_
prefix, and since this worked with jnimarshalmethod-gen, this means
that jnimarshalmethod-gen registration logic likewise didn't use
n_ prefixed method names.

The result is that in the NativeAOT app, it would attempt to register
the native Java method ManagedType.getString(), while what
jcw-gen declared was ManagedType.n_getString()!

Java promptly threw an exception, and the app crashed.

Update Java.Interop.Export-Tests so that all the methods used with
MarshalMemberBuilder are declared with n_ prefixes, and add
a Java.Lang.Object subclass example to the unit tests:

Update tests/Java.Interop.Tools.JavaCallableWrappers-Tests to
add a test for .CodeGenerationTarget==JavaInterop1.

Add $(NoWarn) to
Java.Interop.Tools.JavaCallableWrappers-Tests.csproj in order to
"work around" warnings-as-errors:

…/src/Java.Interop.NamingCustomAttributes/Java.Interop/ExportFieldAttribute.cs(19,63): error CA1019: Remove the property setter from Name or reduce its accessibility because it corresponds to positional argument name
…/src/Java.Interop.NamingCustomAttributes/Android.Runtime/RegisterAttribute.cs(53,4): error CA1019: Remove the property setter from Name or reduce its accessibility because it corresponds to positional argument name
…/src/Java.Interop.NamingCustomAttributes/Java.Interop/ExportFieldAttribute.cs(12,16): error CA1813: Avoid unsealed attributes
…

These are "weird"; the warnings/errors appear to come in because
Java.Interop.Tools.JavaCallableWrappers-Tests.csproj now includes:

<Compile Include="..\..\src\Java.Interop\Java.Interop\JniTypeSignatureAttribute.cs" />

which appears to pull in src/Java.Interop/.editorconfig, which makes
CA1019 and CA1813 errors. (I do not understand what is happening.)

Update jnimarshalmethod-gen so that the Java native methods it
registers have an n_ prefix.

Refactor ExpressionAssemblyBuilder.CreateRegistrationMethod() to
ExpressionAssemblyBuilder.AddRegistrationMethod(), so that
the EmitConsoleWriteLine() invocation can provide the full
type name of the __RegisterNativeMembers() method, which helps
when there is more than one such method running around…

Update ExpressionAssemblyBuilder so that the delegate types it
creates for marshal method registration all have
[UnmanagedFunctionPointer(CallingConvention.Winapi)]. (This isn't
needed here, but is needed in the context of NativeAOT, as
NativeAOT will only emit "marshal stubs" for delegate types which
have [UnmanagedFunctionPointer].) Unfortunately, adding
[UnmanagedFunctionPointer] broke things:

error JM4006: jnimarshalmethod-gen: Unable to process assembly '…/Hello-NativeAOTFromJNI.dll'
Failed to resolve System.Runtime.InteropServices.CallingConvention
Mono.Cecil.ResolutionException: Failed to resolve System.Runtime.InteropServices.CallingConvention
   at Mono.Cecil.Mixin.CheckedResolve(TypeReference self)
   at Mono.Cecil.SignatureWriter.WriteCustomAttributeEnumValue(TypeReference enum_type, Object value)
   …

The problem is that CallingConvention was resolved from
System.Private.CoreLib, and when we removed that assembly
reference, the CallingConvention couldn't be resolved at all.

We could "fix" this by explicitly adding a reference to
System.Runtime.InteropServices.dll, but how many more such corner
cases exist? The current approach is not viable.

Remove the code from 77800dd which attempts to remove
System.Private.CoreLib. So long as ExpressionAssemblyBuilder
output is only used in "completed" apps (not distributed in NuGet
packages or some "intermediate" form), referencing
System.Private.CoreLib is "fine".

Update jnimarshalmethod-gen assembly location probing: in #1153, it
was attempting to resolve the full assembly name of Java.Base, as
Java.Base, Version=7.0.0.0, Culture=neutral, PublicKeyToken=null,
causing it to attempt to load the file
Java.Base, Version=7.0.0.0, Culture=neutral, PublicKeyToken=null.dll,
which doesn't exist. Use AssemblyName to parse the string and
extract out the assembly name, so that Java.Base.dll is probed for
and found.

Update JreTypeManager to also register the marshal methods
generated by
Runtime.MarshalMemberBuilder.GetExportedMemberRegistrations().

With all that, the updated Java.Interop.Export-Tests test now work
both before and after jnimarshalmethod-gen is run:

% dotnet test --logger "console;verbosity=detailed" bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll &&
  dotnet bin/Debug-net7.0/jnimarshalmethod-gen.dll  bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll &&
  dotnet test --logger "console;verbosity=detailed" bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll
…

TODO:

Footnotes

  1. In a .NET Android environment, marshal methods are part of
    generator output, so things would be more straightforward
    there, though all the _JniMarshal_* types that are declared
    would also need to have
    [UnmanagedFunctionPointer(CallingConvention.Winapi)]

  2. Why not just declare toString() as native? Why have the
    separate n_-prefixed version? To make the
    #if MONODROID_TIMING block more consistent within
    JavaCallableWrapperGenerator.GenerateMethod().
    It's likely "too late" to easily change this now.

Context: #1153
Context: 4787e01
Context: 77800dd

PR #1153 is exploring the use of [.NET Native AOT][0] to produce a
native library which is used *within* a `java`-originated process:

	% java -cp … com/microsoft/hello_from_jni/App
	# launches NativeAOT-generated native lib, executes C# code…

As NativeAOT has no support for `System.Reflection.Emit`, the only
way for Java code to invoke managed code -- in a Desktop Java.Base
environment! [^0] see 4787e01 -- would be to pre-generate the
required marshal methods via `jnimarshalmethod-gen`.

This in turn requires updating `jcw-gen` to support the pre-existing
`Java.Interop.JavaCallableAttribute`, so that C# code could
reasonably declare methods visible to Java, along with the
introduction of, and support for, a new
`Java.Interop.JavaCallableConstructorAttribute` type.  This allows
straightforward usage:

	[JniTypeSignature ("example/ManagedType")]      // for a nice Java name!
	class ManagedType : Java.Lang.Object {

	    int value;

	    [JavaCallableConstructor(SuperConstructorExpression="")]
	    public ManagedType (int value) {
	        this.value = value;
	    }

	    [JavaCallable ("getString")]
	    public Java.Lang.String GetString () {
	        return new Java.Lang.String ($"Hello from C#, via Java.Interop! Value={value}");
	    }
	}

Run this through `jcw-gen` and `jnimarshalmethod-gen`, run the app,
and nothing worked (?!), because not all pieces were in agreement.

Java `native` method registration is One Of Those Things™ that
involves lots of moving pieces:

  * `generator` emits bindings for Java types, which includes Java
    method names, signatures, and (on .NET Android) the "connector
    method" to use:

        [Register ("toString", "()Ljava/lang/String;", "GetToStringHandler")]   // .NET Android
        [JniMethodSignature ("toString", "()Ljava/lang/String;")]               // Java.Base
        public override unsafe string? ToString () {…}

  * `jcw-gen` uses `generator` output, *prefixing* Java method names
    with `n_` for `native` method declarations, along with a
    "normal" method wrapper [^1]

        public String toString() {return n_toString();}
        private native String n_toString();

  * `jnimarshalmethod-gen` emits marshal methods for Java.Base,
    and needs to register the `native` methods declared by `jcw-gen`.
    `jnimarshalmethod-gen` and `jcw-gen` need to be consistent with
    each other.

Turns Out, `jcw-gen` and `jnimarshalmethod-gen` were *not* consistent.

The only "real" `jnimarshalmethod-gen` usage (77800dd) is with the
`Java.Interop.Export-Tests` unit test assembly, which *did not use*
`jcw-gen`; it contained only hand-written Java code.  Consequently,
*none* of the Java `native` methods declared within it had an `n_`
prefix, and since this worked with `jnimarshalmethod-gen`, this means
that `jnimarshalmethod-gen` registration logic likewise didn't use
`n_` prefixed method names.

The result is that in the NativeAOT app, it would attempt to register
the `native` Java method `ManagedType.getString()`, while what
`jcw-gen` declared was `ManagedType.n_getString()`!

Java promptly threw an exception, and the app crashed.

Update `Java.Interop.Export-Tests` so that all the methods used with
`MarshalMemberBuilder` are declared with `n_` prefixes, and add
a `Java.Lang.Object` subclass example to the unit tests:

Update `tests/Java.Interop.Tools.JavaCallableWrappers-Tests` to
add a test for `.CodeGenerationTarget==JavaInterop1`.

Add `$(NoWarn)` to
`Java.Interop.Tools.JavaCallableWrappers-Tests.csproj` in order to
"work around" warnings-as-errors:

	…/src/Java.Interop.NamingCustomAttributes/Java.Interop/ExportFieldAttribute.cs(19,63): error CA1019: Remove the property setter from Name or reduce its accessibility because it corresponds to positional argument name
	…/src/Java.Interop.NamingCustomAttributes/Android.Runtime/RegisterAttribute.cs(53,4): error CA1019: Remove the property setter from Name or reduce its accessibility because it corresponds to positional argument name
	…/src/Java.Interop.NamingCustomAttributes/Java.Interop/ExportFieldAttribute.cs(12,16): error CA1813: Avoid unsealed attributes
	…

These are "weird"; the warnings/errors appear to come in because
`Java.Interop.Tools.JavaCallableWrappers-Tests.csproj` now has:

	<Compile Include="..\..\src\Java.Interop\Java.Interop\JniTypeSignatureAttribute.cs" />

which appears to pull in `src/Java.Interop/.editorconfig`, which makes
CA1019 and CA1813 errors.  (I do not understand what is happening.)

Update `jnimarshalmethod-gen` so that the Java `native` methods it
registers have an `n_` prefix.

Refactor `ExpressionAssemblyBuilder.CreateRegistrationMethod()` to
`ExpressionAssemblyBuilder.AddRegistrationMethod()`, so that
the `EmitConsoleWriteLine()` invocation can provide the *full*
type name of the `__RegisterNativeMembers()` method, which helps
when there is more than one such method running around…

Update `ExpressionAssemblyBuilder` so that the delegate types it
creates for marshal method registration all have
`[UnmanagedFunctionPointer(CallingConvention.Winapi)]`.  (This isn't
needed *here*, but is needed in the context of NativeAOT, as
NativeAOT will only emit "marshal stubs" for delegate types which
have `[UnmanagedFunctionPointer]`.)  Unfortunately, adding
`[UnmanagedFunctionPointer]` broke things:

	error JM4006: jnimarshalmethod-gen: Unable to process assembly '…/Hello-NativeAOTFromJNI.dll'
	Failed to resolve System.Runtime.InteropServices.CallingConvention
	Mono.Cecil.ResolutionException: Failed to resolve System.Runtime.InteropServices.CallingConvention
	   at Mono.Cecil.Mixin.CheckedResolve(TypeReference self)
	   at Mono.Cecil.SignatureWriter.WriteCustomAttributeEnumValue(TypeReference enum_type, Object value)
	   …

The problem is that `CallingConvention` was resolved from
`System.Private.CoreLib`, and when we removed that assembly
reference, the `CallingConvention` couldn't be resolved at all.

We could "fix" this by explicitly adding a reference to
`System.Runtime.InteropServices.dll`, but how many more such corner
cases exist?  The current approach is not viable.

Remove the code from 77800dd which attempts to remove
`System.Private.CoreLib`.  So long as `ExpressionAssemblyBuilder`
output is *only* used in "completed" apps (not distributed in NuGet
packages or some "intermediate" form), referencing
`System.Private.CoreLib` is "fine".

Update `jnimarshalmethod-gen` assembly location probing: in #1153, it
was attempting to resolve the *full assembly name* of `Java.Base`, as
`Java.Base, Version=7.0.0.0, Culture=neutral, PublicKeyToken=null`,
causing it to attempt to load the file
`Java.Base, Version=7.0.0.0, Culture=neutral, PublicKeyToken=null.dll`,
which doesn't exist.  Use `AssemblyName` to parse the string and
extract out the assembly name, so that `Java.Base.dll` is probed for
and found.

Update `JreTypeManager` to *also* register the marshal methods
generated by
`Runtime.MarshalMemberBuilder.GetExportedMemberRegistrations()`.

With all that, the updated `Java.Interop.Export-Tests` test now work
both before and after `jnimarshalmethod-gen` is run:

	% dotnet test --logger "console;verbosity=detailed" bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll &&
	  dotnet bin/Debug-net7.0/jnimarshalmethod-gen.dll  bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll &&
	  dotnet test --logger "console;verbosity=detailed" bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll
	…

TODO:

  * `generator --codegen-target=JavaInterop1` should emit
    JNI method signature information for constructors!
    This would likely remove the need for
    `[JavaCallableConstructor(SuperConstructorExpression="")`.

  * #1159

[0]: https://learn.microsoft.com/dotnet/core/deploying/native-aot

[^0]: In a .NET Android environment, marshal methods are part of
      `generator` output, so things would be more straightforward
      there, though all the `_JniMarshal_*` types that are declared
      would also need to have
      `[UnmanagedFunctionPointer(CallingConvention.Winapi)]`…

[^1]: Why not just declare `toString()` as `native`?  Why have the
      separate `n_`-prefixed version?  To make the
      `#if MONODROID_TIMING` block more consistent within
      `JavaCallableWrapperGenerator.GenerateMethod()`.
      It's likely "too late" to *easily* change this now.
@jonpryor
Copy link
Member Author

Doh, and a 4th place that needs to be consistent wrt the n_-prefixed names to regsiter: MarshalMemberbuilder.CreateMarshalToManagedMethodRegistration()!

Will need to update the commit message w/ that tidbit before merging.

@jonpryor
Copy link
Member Author

Also need to comment upon the managedParameters changes to JavaCallableWrapperGenerator, which suggests that [Export] on a constructor in .NET Android didn't work properly if the constructor took any parameters.

@jonpryor
Copy link
Member Author

The managedParameters issue was already filed! dotnet/android#7554

Which means dotnet/android#7554 would be fixed with this PR.

@jonpryor
Copy link
Member Author

PR #1161 should go in first, and this should be rebased atop it.

@jonpryor jonpryor merged commit c93fea0 into main Nov 13, 2023
@jonpryor jonpryor deleted the dev/jonp/jonp-jcwgen+jnimarshalmethodgen-sync branch November 13, 2023 22:28
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants