-
Notifications
You must be signed in to change notification settings - Fork 53
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
[Java.Interop.Tools.Expressions] Add Java.Interop.Tools.Expressions #1046
Conversation
6a31346
to
75b5ce6
Compare
75b5ce6
to
36db586
Compare
36db586
to
e510fbb
Compare
20fa0aa
to
d176b58
Compare
@@ -290,6 +278,10 @@ public LambdaExpression CreateMarshalToManagedExpression (MethodInfo method, Jav | |||
typeof (object), | |||
typeof (IntPtr) | |||
}; | |||
marshalDelegateTypes = new (); |
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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}"); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
d176b58
to
1e1285e
Compare
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
1e1285e
to
180978c
Compare
Context: dotnet/java-interop#1046 The assumption is that This Cannot Possibly Impact™ Android. So… Does It Build™?
Context: dotnet/java-interop#1046 The assumption is that This Cannot Possibly Impact™ Android. So… Does It Build™?
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.
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.
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 |
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? |
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".)
Fixes: #616
Context: #14
Context: ff4053c
Context: da5d1b8
Context: 4787e01
Context: 41ba348
Remember
jnimarshalmethod-gen
(176240d)? And it's crazy idea touse the
System.Linq.Expressions
-based custom marshalinginfrastructure (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 writeassemblies 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 whichuses Mono.Cecil to compile
Expression<T>
expressions to IL.Then update
jnimarshalmethod-gen
to use it!~~ Usage ~~
First param is assembly to process;
Java.Interop.Export-Tests.dll
is handy because that's what the
run-test-jnimarshal
target inMakefile
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
addsDIR
to library resolution paths; this addsbin/TestDebug/net7.0
(dependencies ofJava.Interop.Export-Tests.dll
) andMicrosoft.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 thedefault
-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 incore-tests.yaml
which does:~~ Using
jnimarshalmethod-gen
output ~~What does
jnimarshalmethod-gen
do?is a ~1KB diff which shows, paraphrasing greatly:
wherein
ExportTest._<$>_jni_marshal_methods
and the_JniMarshal*
delegate types are added to the assembly.
This also unblocks the desire stated in 4787e01:
What can we do with this
jnimarshalmethod-gen
output? Use it!First, make sure the tests work:
Then update/replace the unit test assembly with
jnimarshalmethod-gen
output:core-tests.yaml
has been updated to do this.~~ One-Off Tests ~~
One-off tests: ensure that the generated assembly can be decompiled:
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:
Usage of which is "weird":
where:
--tokens
: Include metadata tokens in error messages.--system-module NAME
: set the "System module name". Defaultsto
mscorlib
, which is wrong for .NET 5+, so this must be set toSystem.Private.CoreLib
(no.dll
suffix!).-r FILE-GLOB
: Where to resolve assembly references for theinput assembly. Fortunately file globs are supported…
~~ Removing
System.Private.CoreLib
~~System.Private.CoreLib.dll
is private; it's not part of thepublic assembly surface area, so you can't use
csc -r:System.Private.CoreLib …
and expect it to work. This makesthings interesting because at runtime everything "important" is in
System.Private.CoreLib.dll
, likeSystem.Object
.Specifically, if we do the "obvious" thing and do:
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:
Stream
.Stream
from (1)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
.