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

[marshal methods] Part 3 of ? #7163

Merged
merged 14 commits into from
Aug 19, 2022
Merged

[marshal methods] Part 3 of ? #7163

merged 14 commits into from
Aug 19, 2022

Conversation

grendello
Copy link
Contributor

@grendello grendello commented Jul 11, 2022

Implement marshal methods LLVM IR executable code generator.

The point of this commit is only to generate code and make sure it's valid as far as compiling and linking are concerned.
The code has not been tested at run time as not all the infrastructure on the Xamarin.Android side is implemented yet.
This is on purpose, to keep PRs smaller.

The majority of this PR introduces various classes, enums and structures related to code generation. Support for various LLVM IR instructions
is limited only to those we actually use and only to elements of those constructions that we use. As such, it's not a general purpose code generator which
allows us to make some assumptions and take some shortcuts (without compromising correctness and validity of the generated code)

Portions of the PR (the native type handling system) are to be treated as proof-of-concept as they are not as optimized (design wise) as they should be.
The reason for this limitation is that it requires modifying the previous LLVM IR data generation code and it would contribute to this PR's already substantial
size. The next PR in the series will take care of that rewrite as well as it will focus on implementing the runtime side of marshal methods, making it
possible to actually run applications which use marshal methods.

What this PR implements is the following:

  • LLVM IR
    • function and instruction attributes
    • function parameter (declaration/definition) and argument (runtime) handling
    • function variable (including parameters) handling, including unnamed local variables
    • support for native function signatures and pointers to functions
    • The ret, store, load, icmp, br, call and phi instructions
  • Marshal method generator
    • managed to JNI signature and symbol name translations

@grendello grendello force-pushed the mm-codegen branch 9 times, most recently from dcab5c9 to 0c36cf6 Compare July 18, 2022 09:53
@grendello grendello marked this pull request as ready for review July 18, 2022 09:54
jonpryor pushed a commit that referenced this pull request Jul 18, 2022
…7123)

Context: e1af958
Context: #7163

Changes: dotnet/java-interop@c942ab6...fadbb82

  * dotnet/java-interop@fadbb82c: [generator] Add support for @explicitInterface metadata (#1006)
  * dotnet/java-interop@3e4a3c4f: [Java.Interop.Tools.JavaCallableWrappers] JavaCallableMethodClassifier (#998)

dotnet/java-interop@3e4a3c4f reworked how
`Java.Interop.Tools.JavaCallableWrappers.JavaCallableWrapperGenerator`
can be used to find JNI marshal methods.

Update `Xamarin.Android.Build.Tasks.dll` to provide a
`JavaCallableMethodClassifier` to `JavaCallableWrapperGenerator`,
collecting Cecil `MethodDefinition` instances for JNI marshal methods
which can be created at build time via LLVM Marshal Methods.

Methods which cannot be created at build time include methods with
`[Export]`.

Once candidate LLVM marshal methods are found, the marshal method is
updated to have the [`UnmanagedCallersOnlyAttribute` attribute][0],
and related System.Reflection.Emit-based infrastructure such as the
`Get…Handler()` methods and `cb_…` fields are removed.

As with e1af958, this feature is *not* enabled by default, and
remains a xamarin-android Build time configuration option.

[0]:https://docs.microsoft.com/dotnet/api/system.runtime.interopservices.unmanagedcallersonlyattribute?view=net-6.0
@grendello grendello force-pushed the mm-codegen branch 2 times, most recently from c96dbd4 to 20088b4 Compare July 25, 2022 13:32
The point of this commit is only to generate code and make sure it's
valid as far as compiling and linking are concerned. The code has not
been tested at run time as not all the infrastructure on the
Xamarin.Android side is implemented yet. This is on purpose, to keep PRs
smaller.

The majority of this PR introduces various classes, enums and structures
related to code generation. Support for various LLVM IR instructions is
limited only to those we actually use and only to elements of those
constructions that we use. As such, it's not a general purpose code
generator which allows us to make some assumptions and take some
shortcuts (without compromising correctness and validity of the
generated code)

Portions of the PR (the native type handling system) are to be treated
as proof-of-concept as they are not as optimized (design wise) as they
should be. The reason for this limitation is that it requires modifying
the previous LLVM IR data generation code and it would contribute to
this PR's already substantial size. The next PR in the series will take
care of that rewrite as well as it will focus on implementing the
runtime side of marshal methods, making it possible to actually run
applications which use marshal methods.

What this PR implements is the following:

  * LLVM IR
    * function and instruction attributes
    * function parameter (declaration/definition) and argument (runtime)
      handling
    * function variable (including parameters) handling, including
      unnamed local variables
    * support for native function signatures and pointers to functions
    * The ret, store, load, icmp, br, call and phi instructions
  * Marshal method generator
    * managed to JNI signature and symbol name translations
* main:
  LEGO: Merge pull request 7221
  LEGO: Merge pull request 7219
  [Xamarin.Android.Build.Tasks] use `ToJniName(type, cache)` (dotnet#7211)
  [docs] Synchronize with MicrosoftDocs/xamarin-docs (dotnet#7208)
  [Mono.Android] Remove System.Linq usage (dotnet#7210)
  Bump to Android NDK r25 (dotnet#6764)
  Bump to mono/opentk@daa9b2d5 (dotnet#7192)
  [Mono.Android] Optional NTLMv2 support in AndroidMessageHandler (dotnet#6999)
  Bump to dotnet/installer@53587f9 7.0.100-rc.1.22374.1 (dotnet#7198)
* main:
  Bump to dotnet/java-interop@a5756ca8. (dotnet#7226)
  Bump `$(AndroidNet6Version)` to 32.0.447 (dotnet#7224)
* main:
  [Mono.Android] fix crash on startup with EnableLLVM (dotnet#7188)
  [ci] Add Android Designer test template (dotnet#7227)
* main:
  [Localization] Import translated resx files (dotnet#7190)
* main:
  [build] update to latest JDKs (dotnet#7236)
* main:
  [ci] Upload test assemblies after signing (dotnet#7241)
* main:
  Bump to xamarin/monodroid@210073e1 (dotnet#7272)
  [OneLoc] Localize Microsoft.Android.Templates (dotnet#7248)
  [README] Add links to XA 13.0 release installers (dotnet#7251)
  Bump to dotnet/installer@716bd17 7.0.100-rc.1.22409.23 (dotnet#7247)
  Bump manifest-merger from 30.2.1 to 30.2.2 (dotnet#7238)
* main:
  [ci] disable `<XamarinTelemetry/>` task (dotnet#7275)
  [Xamarin.Android.Build.Tasks] Use marshal-ilgen component (dotnet#7260)
* main:
  [xabuild] update binding redirects for MSBuild 17.3 (dotnet#7273)
sb.Append ('"');
}

RenderAssignedValue (sb);
Copy link
Member

Choose a reason for hiding this comment

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

Semantically, it feels like RenderAssignedValue() should ensure that the assigned value does not contain ", lest it prematurely end the quoting. However, none of the overrides do anything to escape or ensure that the value printed doesn't contain "; see e.g. TuneCpuFunctionAttribute.

Currently, this might not be a problem, but eventually the object graph will be "too big" to easily audit all locations to know that " won't ever be present in a value.

Should a method be added to deal with escaping?

protected void AppendValue (StringBuilder sb, string value)
{
    sb.Append (value.Replace ("\"", "\\\"");
}

Copy link
Contributor Author

@grendello grendello Aug 18, 2022

Choose a reason for hiding this comment

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

I didn't see any attribute values with quotes in the LLVM IR docs, but I will make that change

@@ -350,7 +350,7 @@ namespace xamarin::android::internal
static void monodroid_debugger_unhandled_exception (MonoException *ex);

#if defined (RELEASE) && defined (ANDROID)
static void* get_function_pointer (uint32_t mono_image_index, uint32_t class_token, uint32_t method_token) noexcept;
static void get_function_pointer (uint32_t mono_image_index, uint32_t class_token, uint32_t method_token, void*& target_ptr) noexcept;
Copy link
Member

Choose a reason for hiding this comment

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

I forget; why make the function pointer an "output" parameter instead of a return value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It generates faster code

@@ -5,12 +5,12 @@

using namespace xamarin::android::internal;

void* MonodroidRuntime::get_function_pointer (uint32_t mono_image_index, uint32_t class_token, uint32_t method_token) noexcept
void MonodroidRuntime::get_function_pointer (uint32_t mono_image_index, uint32_t class_index, uint32_t method_token, void *&target_ptr) noexcept
Copy link
Member

Choose a reason for hiding this comment

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

Why rename class_token to class_index? mono_class_get() uses type_token as the parameter name, so a _token suffix appears to make more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because token is no longer used to look up a class. Each generated method passes an index into cache array of class instances:

; marshal_methods_class_cache
@marshal_methods_class_cache = global [104 x %struct.MarshalMethodsManagedClass] [
        ; 0
        %struct.MarshalMethodsManagedClass {
                i32 33554780, ; token 0x200015c; class name: Android.Views.View, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065
                %struct.MonoClass* null; klass
        }, 
        ; 1
        %struct.MarshalMethodsManagedClass {
                i32 33554448, ; token 0x2000010; class name: AndroidX.Activity.ContextAware.IOnContextAvailableListenerInvoker, Xamarin.AndroidX.Activity, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null
                %struct.MonoClass* null; klass
        }, 
        ; 2
        %struct.MarshalMethodsManagedClass {
                i32 33555145, ; token 0x20002c9; class name: Android.Content.IDialogInterfaceOnClickListenerInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065
                %struct.MonoClass* null; klass
        }, 
        ; 3

It's faster to look up this way:

MarshalMethodsManagedClass &klass = marshal_methods_class_cache[class_index];
if (klass.klass == nullptr) {
  klass.klass = mono_class_get (image, klass.token);
}

@jonpryor
Copy link
Member

Context: e1af9587bb98d4c249bbc392ceccc2b53ffff155

Commit e1af9587 mentioned a TODO:

> * Finish the LLVM code generator so that LLVM Marshal Methods are
>   emitted into `libxamarin-app.so`.

Implement LLVM Marshal Methods LLVM-IR executable code generator:

  * LLVM-IR Generation Infrastructure:
    * function and instruction attributes
    * function parameter (declaration/definition) and argument
      (runtime) handling
    * function variable (including parameters) handling, including
      unnamed local variables
    * support for native function signatures and pointers to functions
    * The ret, store, load, icmp, br, call and phi instructions
  * LLVM-IR Marshal Method generator
    * managed to JNI signature and symbol name translations

When `ENABLE_MARSHAL_METHODS` is defined, LLVM-IR code is generated,
compiled, and linked.  Generated code is minimally tested for runtime
behavior, as not all the infrastructure on the Xamarin.Android side
is implemented yet. This is on purpose, to keep commits smaller.

Support for various LLVM IR instructions is limited only to those we
actually use and only to elements of those constructions that we use.
As such, it's not a general purpose code generator which allows us to
make some assumptions and take some shortcuts, without compromising
correctness and validity of the generated code.

The native type handling system portion of this commit is to be
treated as proof-of-concept: is is not as optimized (design wise) as
it should be. The reason for this limitation is that it requires
modifying the previous LLVM IR data generation code and it would
contribute to this commits' already substantial size.

~~ TODO ~~

Update/rewrite infrastructure to focus on implementing the runtime
side of marshal methods, making it possible to actually run
applications which use marshal methods.

Finish prototyping the infrastructure so that a MAUI app can be run
with LLVm Marshal Methods enabled.

@jonpryor jonpryor merged commit 903ba37 into dotnet:main Aug 19, 2022
@grendello grendello deleted the mm-codegen branch August 19, 2022 17:33
grendello added a commit to grendello/xamarin-android that referenced this pull request Aug 19, 2022
* main:
  [Xamarin.Android.Build.Tasks, monodroid] LLVM-IR Generator (dotnet#7163)
  $(AndroidPackVersionSuffix)=rc.2; net7 is 33.0.0.rc.2 (dotnet#7283)
  [ci] Add EXE files to windows-toolchain-pdb artifact (dotnet#7279)
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 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