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

[Java.Interop.Tools.Expressions] Add Java.Interop.Tools.Expressions #1046

Merged
merged 3 commits into from
Feb 28, 2023

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented Sep 29, 2022

Fixes: #616

Context: #14
Context: ff4053c
Context: da5d1b8
Context: 4787e01
Context: 41ba348

Remember jnimarshalmethod-gen (176240d)? And it's crazy idea to
use the System.Linq.Expressions-based custom marshaling
infrastructure (ff4053c, da5d1b8) to generate JNI marshal methods
at build/packaging time. And how we had to back burner it because
it depended upon System.Reflection.Emit being able to write
assemblies to disk, which is a feature that never made it to
.NET Core, and is still lacking as of .NET 7?

Add src/Java.Interop.Tools.Expressions, which contains code which
uses Mono.Cecil to compile Expression<T> expressions to IL.

Then update jnimarshalmethod-gen to use it!

~~ Usage ~~

% dotnet bin/Debug-net7.0/jnimarshalmethod-gen.dll \
  bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll \
  -v -v --keeptemp \
  --jvm /Library/Java/JavaVirtualMachines/microsoft-11.jdk/Contents/Home/lib/jli/libjli.dylib \
  -o _x \
  -L bin/TestDebug-net7.0 \
  -L /usr/local/share/dotnet/shared/Microsoft.NETCore.App/7.0.0

First param is assembly to process; Java.Interop.Export-Tests.dll
is handy because that's what the run-test-jnimarshal target in
Makefile processed.

  • -v -v is really verbose output

  • --keeptemp is keep temporary files, in this case
    _x/Java.Interop.Export-Tests.dll.cecil.

  • --jvm PATH is the path to the JVM library to load+use.

  • -o DIR is where to place output files; this will create
    _x/Java.Interop.Export-Tests.dll.

  • -L DIR adds DIR to library resolution paths; this adds
    bin/TestDebug/net7.0 (dependencies of
    Java.Interop.Export-Tests.dll) and
    Microsoft.NETCore.App/7.0.0-rc.1.22422.12 (net7 libs).

By default the directories containing input assemblies and the
directory containing System.Private.CoreLib.dll are part of the
default -L list.

When running in-tree, e.g. AzDO pipeline execution, --jvm PATH
will attempt to read the path in bin/Build*/JdkInfo.props
a'la TestJVM (002dea4). This allows an in-place update in
core-tests.yaml which does:

dotnet bin/Debug-net7.0/jnimarshalmethod-gen.dll \
  bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll \
  -v -v --keeptemp -o bin/TestDebug-net7.0

~~ Using jnimarshalmethod-gen output ~~

What does jnimarshalmethod-gen do?

% ikdasm bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll > beg.il
% ikdasm _x/Java.Interop.Export-Tests.dll > end.il
% git diff --no-index beg.il end.il
# https://gist.github.com/jonpryor/b8233444f2e51043732bea922f6afc81

is a ~1KB diff which shows, paraphrasing greatly:

public partial class ExportTest {
    partial class '__<$>_jni_marshal_methods' {
        static IntPtr funcIJavaObject (IntPtr jnienv, IntPtr this) => …
        // …
        [JniAddNativeMethodRegistration]
        static void __RegisterNativeMembers (JniNativeMethodRegistrationArguments args) => …
    }
}
internal delegate long _JniMarshal_PP_L (IntPtr jnienv, IntPtr self);
// …

wherein ExportTest._<$>_jni_marshal_methods and the _JniMarshal*
delegate types are added to the assembly.

This also unblocks the desire stated in 4787e01:

For Java.Base, @jonpryor wants to support the custom marshaling
infrastructure introduced in 77a6bf8. This would allow types to
participate in JNI marshal method ("connector method") generation
at runtime, allowing specialization based on the current set of
types and assemblies.

What can we do with this jnimarshalmethod-gen output? Use it!

First, make sure the tests work:

% dotnet test --logger "console;verbosity=detailed" bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll
…
Passed!  - Failed:     0, Passed:    17, Skipped:     0, Total:    17, Duration: 103 ms - Java.Interop.Export-Tests.dll (net7.0)

Then update/replace the unit test assembly with
jnimarshalmethod-gen output:

% \cp _x/Java.Interop.Export-Tests.dll bin/TestDebug-net7.0
% dotnet test --logger "console;verbosity=detailed" bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll
…
Total tests: 17
     Passed: 17

core-tests.yaml has been updated to do this.

~~ One-Off Tests ~~

One-off tests: ensure that the generated assembly can be decompiled:

% ikdasm  bin/TestDebug-net7.0/Java.Interop.Tools.Expressions-Tests-ExpressionAssemblyBuilderTests.dll
% monodis bin/TestDebug-net7.0/Java.Interop.Tools.Expressions-Tests-ExpressionAssemblyBuilderTests.dll

% ikdasm  _x/Java.Interop.Export-Tests.dll
% monodis _x/Java.Interop.Export-Tests.dll
# which currently fails :-()

Re-enable most of Java.Interop.Export-Tests.dll for .NET 7;
see 41ba348, which disabled those tests.

To verify the generated IL, use the dotnet-ilverify tool:

dotnet tool install --global dotnet-ilverify

Usage of which is "weird":

$HOME/.dotnet/tools/ilverify _x/Java.Interop.Export-Tests.dll \
  --tokens --system-module System.Private.CoreLib \
  -r 'bin/TestDebug-net7.0/*.dll' \
  -r '/usr/local/share/dotnet/shared/Microsoft.NETCore.App/7.0.0/*.dll'
All Classes and Methods in /Volumes/Xamarin-Work/src/xamarin/Java.Interop/_x/Java.Interop.Export-Tests.dll Verified.
# no errors!

where:

  • --tokens: Include metadata tokens in error messages.
  • --system-module NAME: set the "System module name". Defaults
    to mscorlib, which is wrong for .NET 5+, so this must be set to
    System.Private.CoreLib (no .dll suffix!).
  • -r FILE-GLOB: Where to resolve assembly references for the
    input assembly. Fortunately file globs are supported…

~~ Removing System.Private.CoreLib ~~

System.Private.CoreLib.dll is private; it's not part of the
public assembly surface area, so you can't use
csc -r:System.Private.CoreLib … and expect it to work. This makes
things interesting because at runtime everything "important" is in
System.Private.CoreLib.dll, like System.Object.

Specifically, if we do the "obvious" thing and do:

newTypeDefinition.BaseType = assemblyDefinition.MainModule
    .ImportReference (typeof (object));

you're gonna have a bad type, because the resulting IL for
newTypeDefinition will have a base class of
[System.Private.CoreLib]System.Object, which isn't usable.

Fix this by:

  1. Writing the assembly to a Stream.
  2. Reading the Stream from (1)
  3. Fixing all member references and assembly references so that
    System.Private.CoreLib is not referenced.

If jnimarshalmethod-gen.dll --keeptemp is used, then a .cecil
file is created with the contents of (1).

Additionally, and unexpectedly -- jbevain/cecil#895 -- Mono.Cecil
adds a reference to the assembly being modified. Remove the declaring
assembly from AssemblyReferences.

@jonpryor jonpryor force-pushed the jonp-cecil-expression-compiler branch from 6a31346 to 75b5ce6 Compare November 2, 2022 00:42
@jonpryor jonpryor force-pushed the jonp-cecil-expression-compiler branch from 75b5ce6 to 36db586 Compare January 31, 2023 02:06
@jonpryor jonpryor changed the title [Java.Interop.Tools.Expressions] Add [Java.Interop.Tools.Expressions] Add Java.Interop.Tools.Expressions Feb 1, 2023
@jonpryor jonpryor force-pushed the jonp-cecil-expression-compiler branch from 36db586 to e510fbb Compare February 1, 2023 03:05
@jonpryor jonpryor force-pushed the jonp-cecil-expression-compiler branch 5 times, most recently from 20fa0aa to d176b58 Compare February 10, 2023 18:40
@@ -290,6 +278,10 @@ public LambdaExpression CreateMarshalToManagedExpression (MethodInfo method, Jav
typeof (object),
typeof (IntPtr)
};
marshalDelegateTypes = new ();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better if it used an ordinal string comparer?

List<Instruction> GetFixupsForLabelTarget (LabelTarget target)
{
List<Instruction>? fixups;
if (!returnFixups.TryGetValue (target, out fixups)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare fixups in the method call? :)

{
Logger (TraceLevel.Verbose, $"# jonp: CecilCompilerExpressionVisitor.VisitLabel: {node}");
var target = il.Body.Instructions.Last ();
if (returnFixups.TryGetValue (node.Target, out var fixups)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the actual type instead of var here? For us poor folks who don't use IDEs and those who are reviewing PRs without the docs at hand? :)

var variableVisitor = new VariableExpressionVisitor (variables.Keys, Logger);
variableVisitor.Visit (e);

Console.WriteLine ($"# jonp: filling {variableVisitor.Variables.Count} variables");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover debug statement?

if (c == variableVisitor.ReturnValue) {
ReturnValue = v;
}
Console.WriteLine ($"# jonp: FillVariables: local var {c.Name} is index {i}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

delegateName.Append ("_JniMarshal_PP");

for (int i = 2; i < parameters.Count; i++) {
delegateName.Append (GetJniMarshalDelegateParameterIdentifier (parameters [i].ParameterType));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might have missed it, but shouldn't it handle arrays too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arrays will hit the default case of L, which is "any pointer type" at the ABI boundary. This is akin to dotnet/android@baa5a73, which doesn't need to care about how many [ are encountered, as no matter how deep the array is, it's still just a pointer.

@jonpryor jonpryor force-pushed the jonp-cecil-expression-compiler branch from d176b58 to 1e1285e Compare February 15, 2023 13:13
@jonpryor jonpryor marked this pull request as ready for review February 15, 2023 23:28
Fixes: dotnet#616

Context: dotnet#14
Context: ff4053c
Context: da5d1b8
Context: 4787e01
Context: 41ba348

Remember `jnimarshalmethod-gen` (176240d)?  And it's crazy idea to
use the `System.Linq.Expressions`-based custom marshaling
infrastructure (ff4053c, da5d1b8) to generate JNI marshal methods
at build/packaging time.  And how we had to back burner it because
it depended upon `System.Reflection.Emit` being able to write
assemblies to disk, which is a feature that never made it to
.NET Core, and is still lacking as of .NET 7?

Add `src/Java.Interop.Tools.Expressions`, which contains code which
uses Mono.Cecil to compile `Expression<T>` expressions to IL.

Then update `jnimarshalmethod-gen` to use it!

~~ Usage ~~

	% dotnet bin/Debug-net7.0/jnimarshalmethod-gen.dll \
	  bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll \
	  -v -v --keeptemp \
	  --jvm /Library/Java/JavaVirtualMachines/microsoft-11.jdk/Contents/Home/lib/jli/libjli.dylib \
	  -o _x \
	  -L bin/TestDebug-net7.0 \
	  -L /usr/local/share/dotnet/shared/Microsoft.NETCore.App/7.0.0

First param is assembly to process; `Java.Interop.Export-Tests.dll`
is handy because that's what the `run-test-jnimarshal` target in
`Makefile` processed.

  * `-v -v` is *really* verbose output

  * `--keeptemp` is keep temporary files, in this case
    `_x/Java.Interop.Export-Tests.dll.cecil`.

  * `--jvm PATH` is the path to the JVM library to load+use.

  * `-o DIR` is where to place output files; this will create
    `_x/Java.Interop.Export-Tests.dll`.

  * `-L DIR` adds `DIR` to library resolution paths; this adds
    `bin/TestDebug/net7.0` (dependencies of
    `Java.Interop.Export-Tests.dll`) and
    `Microsoft.NETCore.App/7.0.0-rc.1.22422.12` (net7 libs).

By default the directories containing input assemblies and the
directory containing `System.Private.CoreLib.dll` are part of the
default `-L` list.

When running in-tree, e.g. AzDO pipeline execution, `--jvm PATH`
will attempt to read the path in `bin/Build*/JdkInfo.props`
a'la `TestJVM` (002dea4).  This allows an in-place update in
`core-tests.yaml` which does:

	dotnet bin/Debug-net7.0/jnimarshalmethod-gen.dll \
	  bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll \
	  -v -v --keeptemp -o bin/TestDebug-net7.0

~~ Using `jnimarshalmethod-gen` output ~~

What does `jnimarshalmethod-gen` *do*?

	% ikdasm bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll > beg.il
	% ikdasm _x/Java.Interop.Export-Tests.dll > end.il
	% git diff --no-index beg.il end.il
	# https://gist.github.com/jonpryor/b8233444f2e51043732bea922f6afc81

is a ~1KB diff which shows, paraphrasing greatly:

	public partial class ExportTest {
	    partial class '__<$>_jni_marshal_methods' {
	        static IntPtr funcIJavaObject (IntPtr jnienv, IntPtr this) => …
	        // …
	        [JniAddNativeMethodRegistration]
	        static void __RegisterNativeMembers (JniNativeMethodRegistrationArguments args) => …
	    }
	}
	internal delegate long _JniMarshal_PP_L (IntPtr jnienv, IntPtr self);
	// …

wherein `ExportTest._<$>_jni_marshal_methods` and the `_JniMarshal*`
delegate types are added to the assembly.

This also unblocks the desire stated in 4787e01:

> For `Java.Base`, @jonpryor wants to support the custom marshaling
> infrastructure introduced in 77a6bf8.  This would allow types to
> participate in JNI marshal method ("connector method") generation
> *at runtime*, allowing specialization based on the current set of
> types and assemblies.

What can we do with this `jnimarshalmethod-gen` output?  Use it!

First, make sure the tests work:

	% dotnet test --logger "console;verbosity=detailed" bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll
	…
	Passed!  - Failed:     0, Passed:    17, Skipped:     0, Total:    17, Duration: 103 ms - Java.Interop.Export-Tests.dll (net7.0)

Then update/replace the unit test assembly with
`jnimarshalmethod-gen` output:

	% \cp _x/Java.Interop.Export-Tests.dll bin/TestDebug-net7.0
	% dotnet test --logger "console;verbosity=detailed" bin/TestDebug-net7.0/Java.Interop.Export-Tests.dll
	…
	Total tests: 17
	     Passed: 17

`core-tests.yaml` has been updated to do this.

~~ One-Off Tests ~~

One-off tests: ensure that the generated assembly can be decompiled:

	% ikdasm  bin/TestDebug-net7.0/Java.Interop.Tools.Expressions-Tests-ExpressionAssemblyBuilderTests.dll
	% monodis bin/TestDebug-net7.0/Java.Interop.Tools.Expressions-Tests-ExpressionAssemblyBuilderTests.dll

	% ikdasm  _x/Java.Interop.Export-Tests.dll
	% monodis _x/Java.Interop.Export-Tests.dll
	# which currently fails :-()

Re-enable most of `Java.Interop.Export-Tests.dll` for .NET 7;
see 41ba348, which disabled those tests.

To verify the generated IL, use the [dotnet-ilverify][0] tool:

	dotnet tool install --global dotnet-ilverify

Usage of which is "weird":

	$HOME/.dotnet/tools/ilverify _x/Java.Interop.Export-Tests.dll \
	  --tokens --system-module System.Private.CoreLib \
	  -r 'bin/TestDebug-net7.0/*.dll' \
	  -r '/usr/local/share/dotnet/shared/Microsoft.NETCore.App/7.0.0/*.dll'
	All Classes and Methods in /Volumes/Xamarin-Work/src/xamarin/Java.Interop/_x/Java.Interop.Export-Tests.dll Verified.
	# no errors!

where:

  * `--tokens`: Include metadata tokens in error messages.
  * `--system-module NAME`: set the "System module name".  Defaults
    to `mscorlib`, which is wrong for .NET 5+, so this must be set to
    `System.Private.CoreLib` (no `.dll` suffix!).
  * `-r FILE-GLOB`: Where to resolve assembly references for the
    input assembly.  Fortunately file globs are supported…

~~ Removing `System.Private.CoreLib` ~~

`System.Private.CoreLib.dll` is *private*; it's not part of the
public assembly surface area, so you can't use
`csc -r:System.Private.CoreLib …` and expect it to work.  This makes
things interesting because *at runtime* everything "important" is in
`System.Private.CoreLib.dll`, like `System.Object`.

Specifically, if we do the "obvious" thing and do:

	newTypeDefinition.BaseType = assemblyDefinition.MainModule
	    .ImportReference (typeof (object));

you're gonna have a bad type, because the resulting IL for
`newTypeDefinition` will have a base class of
`[System.Private.CoreLib]System.Object`, which isn't usable.

Fix this by:

 1. Writing the assembly to a `Stream`.
 2. Reading the `Stream` from (1)
 3. Fixing all member references and assembly references so that
    `System.Private.CoreLib` is not referenced.

If `jnimarshalmethod-gen.dll --keeptemp` is used, then a `.cecil`
file is created with the contents of (1).

Additionally, and unexpectedly -- [jbevain/cecil#895][1] -- Mono.Cecil
adds a reference to the assembly being modified.  Remove the declaring
assembly from `AssemblyReferences`.

[0]: https://www.nuget.org/packages/dotnet-ilverify
[1]: jbevain/cecil#895
@jonpryor jonpryor force-pushed the jonp-cecil-expression-compiler branch from 1e1285e to 180978c Compare February 23, 2023 00:31
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Feb 23, 2023
Context: dotnet/java-interop#1046

The assumption is that This Cannot Possibly Impact™ Android.
So… Does It Build™?
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Feb 23, 2023
Context: dotnet/java-interop#1046

The assumption is that This Cannot Possibly Impact™ Android.
So… Does It Build™?
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Feb 23, 2023
Context: dotnet/java-interop#1046

The assumption is that This Cannot Possibly Impact™ Android.
So… Does It Build™?

…no, it does not build, because dotnet/java-interop#1046 removes
the `net472` build of `jnimarshalmethod-gen.exe`, which causes
packaging to fail.  Noice.

Given that `jnimarshalmethod-gen.exe` only "worked" in Classic,
and that main is (slowly) dropping support for Classic (618bd4a),
update the repo to stop packaging `jnimarshalmethod-gen.*` and
`Java.Runtime.Environment.*`.  This should fix the packaging errors.
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Feb 23, 2023
Context: dotnet/java-interop#1046

The assumption is that This Cannot Possibly Impact™ Android.
So… Does It Build™?

…no, it does not build, because dotnet/java-interop#1046 removes
the `net472` build of `jnimarshalmethod-gen.exe`, which causes
packaging to fail.  Noice.

Given that `jnimarshalmethod-gen.exe` only "worked" in Classic,
and that main is (slowly) dropping support for Classic (618bd4a),
update the repo to stop packaging `jnimarshalmethod-gen.*` and
`Java.Runtime.Environment.*`.  This should fix the packaging errors.
@dellis1972
Copy link
Contributor

Do we need to worry about adding a PublicKey to these assemblies? Like we did for the Resource designer one? https://github.com/xamarin/xamarin-android/blob/22f10b2edec9e40a139c04564f96d8c1b3c17526/src/Xamarin.Android.Build.Tasks/Tasks/GenerateResourceDesignerAssembly.cs#L356

@jonpryor
Copy link
Member Author

@dellis1972 asked:

Do we need to worry about adding a PublicKey to these assemblies?

Good question. I don't know. We can answer that question when we get around to attempting to integrate this madness into xamarin-android, in…two years?

@jonpryor jonpryor merged commit 77800dd into dotnet:main Feb 28, 2023
jonpryor added a commit to dotnet/android that referenced this pull request Feb 28, 2023
Changes: dotnet/java-interop@bbaeda6...77800dd

  * dotnet/java-interop@77800dda: [Java.Interop.Tools.Expressions] Add Java.Interop.Tools.Expressions (dotnet/java-interop#1046)

Remember `jnimarshalmethod-gen.exe`?  (0140ab8, d5b2ece, 106a621, …)
It never made it to completion, was never stable enough to be used,
even though we did add a "public" *documented*
`$(AndroidGenerateJniMarshalMethods)` MSBuild property to control it…

`jnimarshalmethod-gen.exe` never made it to .NET Android, as it
required .NET Framework features which didn't make it to .NET Core.

dotnet/java-interop@77800dda updates `jnimarshalmethod-gen` to drop
support for building on .NET Framework 4.7.2, and adds support to
build it for .NET 7.

Update `build-tools/installers/create-installers.targets` so that
`jnimarshalmethod-gen.exe` is no longer included in the Classic
installers (which are increasingly moot anyway; see also 618bd4a).

Remove generation of `Java.Runtime.Environment.dll.config`, as
that file was only supported when using Mono, which won't be the case
under .NET 7.

Update the `_GenerateJniMarshalMethods` target so that it `<Error/>`s
when `$(AndroidGenerateJniMarshalMethods)` is True.  While
`jnimarshalmethod-gen.dll` may run on .NET 7 now, @jonpryor doesn't
want to deal with the *integration* work to see if it is usable on
.NET Android, especially considering that the Classic version didn't
work either!  (That integration effort is for "later".)
@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.

Port jnimarshalmethod-gen.exe to net5
4 participants