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

CLR hosting #9572

Draft
wants to merge 126 commits into
base: main
Choose a base branch
from
Draft

CLR hosting #9572

wants to merge 126 commits into from

Conversation

grendello
Copy link
Contributor

@grendello grendello commented Dec 2, 2024

Required CoreCLR runtime changes: dotnet/runtime#112705

@grendello grendello force-pushed the dev/grendel/clr-host branch 3 times, most recently from c085352 to 13bbd93 Compare December 3, 2024 21:15
@grendello grendello force-pushed the dev/grendel/clr-host branch 2 times, most recently from 82cfb8a to 77aa40a Compare December 4, 2024 22:10
@@ -216,5 +218,6 @@
<ItemGroup>
<AndroidAbiAndRuntimeFlavor Include="@(AndroidSupportedTargetJitAbi)" AndroidRuntime="Mono" />
<AndroidAbiAndRuntimeFlavor Include="@(AndroidSupportedTargetJitAbi)" AndroidRuntime="NativeAOT" />
<AndroidAbiAndRuntimeFlavor Include="@(AndroidSupportedTargetJitAbi)" AndroidRuntime="CoreCLR" />
Copy link
Member

Choose a reason for hiding this comment

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

I couldn't figure out if we should case it CoreClr or CoreCLR:

I found both, so we can probably decide which we prefer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like CoreCLR better, since CLR is an acronym

Comment on lines 237 to 240
<_ExcludedNativeLibraries Condition=" '$(AndroidRuntime)' == 'Mono' Or '$(AndroidRuntime)' == '' " Include="libnet-android.debug" />
<_ExcludedNativeLibraries Condition=" '$(AndroidRuntime)' == 'Mono' Or '$(AndroidRuntime)' == '' " Include="libnet-android.release" />
<_ExcludedNativeLibraries Condition=" '$(AndroidRuntime)' == 'CoreCLR' " Include="libmono-android.debug" />
<_ExcludedNativeLibraries Condition=" '$(AndroidRuntime)' == 'CoreCLR' " Include="libmono-android.release" />
Copy link
Member

Choose a reason for hiding this comment

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

So, $(AndroidRuntime) isn't defined for customer's project builds. I think it would always be blank?

Right now, we only have these:

  • $(UseMonoRuntime)=true
  • $(_AndroidNativeAot)=true

Maybe we need to figure out what property xamarin/xamarin-macios uses to identify CoreCLR? You can use it for macOS, to choose between Mono & CoreCLR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it would always be blank now and I think we should always expect a value in there. The macios way looks fine.

Comment on lines 301 to 302
<_AndroidUseCLR Condition=" '$(AndroidRuntime)' == 'CoreCLR' ">True</_AndroidUseCLR>
<_AndroidUseCLR Condition=" '$(AndroidRuntime)' != 'CoreCLR' ">False</_AndroidUseCLR>
Copy link
Member

Choose a reason for hiding this comment

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

$(AndroidRuntime) is probably always blank in customer's Android projects, but is this the property to select a runtime? Should the property be in DefaultProperties.targets?

Maybe we should align with xamarin/xamarin-macios?

@jonathanpeppers
Copy link
Member

Ok, here is what we should do for MSBuild properties for now:

  • $(_AndroidRuntime) is the property we use throughout Android project builds (customer projects), it can say MonoVM, NativeAOT, or CoreCLR. It is a private property.
  • If PublishAot=true, $(_AndroidRuntime)=NativeAOT
  • If UseMonoRuntime=false, $(_AndroidRuntime)=CoreCLR
  • If $(UseMonoRuntime)="", we default to $(UseMonoRuntime)=true in DefaultProperties.targets
  • If UseMonoRuntime=true, $(_AndroidRuntime)=MonoVM

We could remove $(_AndroidNativeAot) and check '$(_AndroidRuntime)' == 'NativeAOT' instead. If we keep it, we should introduce $(_AndroidCoreClr).

@grendello
Copy link
Contributor Author

Ok, here is what we should do for MSBuild properties for now:

* `$(_AndroidRuntime)` is the property we use throughout Android project builds (customer projects), it can say `MonoVM`, `NativeAOT`, or `CoreCLR`. It is a private property.

* If `PublishAot=true`, `$(_AndroidRuntime)=NativeAOT`

* If `UseMonoRuntime=false`, `$(_AndroidRuntime)=CoreCLR`

* If `$(UseMonoRuntime)=""`, we default to `$(UseMonoRuntime)=true` in `DefaultProperties.targets`

* If `UseMonoRuntime=true`, `$(_AndroidRuntime)=MonoVM`

We could remove $(_AndroidNativeAot) and check '$(_AndroidRuntime)' == 'NativeAOT' instead. If we keep it, we should introduce $(_AndroidCoreClr).

I like $(_AndroidRuntime) for this, it's much leaner and reads better IMO :)

jonathanpeppers added a commit that referenced this pull request Jan 15, 2025
Context: #9572 (comment)
Context: https://github.com/xamarin/xamarin-macios/blob/2009c571aa8a975ab0e0b1785e5484dbd64d6f9b/dotnet/targets/Xamarin.Shared.Sdk.targets#L1004-L1006

To align with xamarin/xamarin-macios, the following public MSBuild
properties can be used to select a runtime for iOS, macOS, etc.:

* `$(UseMonoRuntime)=true`: MonoVM
* `$(UseMonoRuntime)=false`: CoreCLR
* `$(PublishAot)=true`: NativeAOT
* Defaults if blank, select MonoVM

Introduce a private `$(_AndroidRuntime)` property we can use
throughout our build to know which runtime is being targetted.

This way, if a new property is designed in the future, we can keep
using `$(_AndroidRuntime)` and simply update the defaults.
jonpryor pushed a commit that referenced this pull request Jan 21, 2025
…#9686)

Context: #9572 (comment)
Context: https://github.com/xamarin/xamarin-macios/blob/2009c571aa8a975ab0e0b1785e5484dbd64d6f9b/dotnet/targets/Xamarin.Shared.Sdk.targets#L1004-L1006

To align with xamarin/xamarin-macios, the following public MSBuild
properties can be used to select a runtime for iOS, macOS, etc.:

  * `$(UseMonoRuntime)=true`: MonoVM
  * `$(UseMonoRuntime)=false`: CoreCLR
  * `$(PublishAot)=true`: NativeAOT
  * Defaults if blank, select MonoVM

Introduce a private `$(_AndroidRuntime)` property we can use
throughout our build to know which runtime is being targeted.

This way, if a new property is designed in the future, we can keep
using `$(_AndroidRuntime)` and simply update the defaults.
jonpryor pushed a commit that referenced this pull request Feb 6, 2025
Context: #9572

#9572 is prototyping CoreCLR support, and as part of
that it renamed e.g. `src/native/monodroid` to
`src/native/mono/monodroid` -- "inserting" a "runtime" value
underneath `src/native` -- so that CoreCLR code won't be intermingled
with MonoVM code.

Separate out these file moves into a separate PR, to reduce the size
and review complexity of #9572.
jonpryor pushed a commit that referenced this pull request Feb 14, 2025
Context: #9572

Add support for Unicode strings to the LLVM IR generator, together with
de-duplication and support for outputting the same string encoded both
as UTF-8 and Unicode (UTF16LE).

Additionally, since all the files are generated from separate tasks, we
don't have a global LLVM IR state which can keep track of strings and
ensure that there are no duplicate symbol names.  To prevent potential
clashes, each generator now sets the default string group name which is
unique for each module.

	; From marshal_methods.arm64-v8a.ll
	@.mm.0 = dso_local constant [102 x i8] c"Android.App.Activity, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065\00", align 1

	; from environment.arm64-v8a.ll
	@.env.0 = dso_local constant [7 x i8] c"normal\00", align 1

	; from typemaps.arm64-v8a.ll
	@.tmr.0 = dso_local constant [22 x i8] c"android/os/BaseBundle\00", align 1

`mm`, `env`, and `tmr` are the default string groups for each module.

In the future, we should try to manage strings globally (which would
also result in more de-duplication).
jonpryor pushed a commit that referenced this pull request Feb 14, 2025
Context: #9572

Add a new `src/native/common` for files which can be shared between
MonoVM and CoreCLR, and move common infrastructural source there,
such as `jni-wrappers.hh` and `strings.hh`.

Copy the parts of `src/native/mono` that will be used by #9572 but
require modification to be used with CoreCLR into `src/native/clr`.

`src/native/clr` is not yet compiled; this merely serves the purpose
of making #9572 smaller and easier to review, copying code from the
existing MonoVM implementation to the CoreCLR one, mostly without
functional changes.  There are very few functional changes, most code
is copied verbatim with the only changes being formatting, function
declaration syntax and, sometimes, the class in which the code lives.

Only the code that's actually used in #9572 is copied.  This allows
us to refresh our runtime code while cleaning it up at the same time,
with the goal of having an implementation that is tailored strictly
towards CoreCLR (which doesn't have e.g. Mono's embedding APIs)
grendello added a commit that referenced this pull request Feb 18, 2025
grendello added a commit that referenced this pull request Feb 19, 2025
grendello added a commit that referenced this pull request Feb 20, 2025
grendello added a commit that referenced this pull request Feb 20, 2025
This is needed until #9572 is merged, since the != 'NativeAOT'
would enable the guarded code to run also for `CoreCLR`, which
is not fully functional in this PR.
@grendello grendello force-pushed the dev/grendel/clr-host branch from af8bb41 to c853ea1 Compare February 21, 2025 08:48
jonpryor added a commit that referenced this pull request Feb 21, 2025
Context: #9572

PR #9572 is quite large.  Extract out a set of changes for easier
review.

Co-authored-by: Jonathan Pryor <[email protected]>
Co-authored-by: Jonathan Peppers <[email protected]>
@grendello grendello force-pushed the dev/grendel/clr-host branch from b9bd06b to ee0c8a5 Compare February 21, 2025 16:47
grendello and others added 29 commits February 25, 2025 00:16
Caveats:

  * Trimming must be turned off
  * Typemaps don't work (everything is resolved dynamically)
  * Marshal methods must be turned off (they use MonoVM embedding API)
By convention (and often enforced requirement), `$(OutputPath)` ends
with `\`, which means that quoting it for use with `<Exec/>`:

    <Exec Command="whatever &quot;$(OutputPath)&quot;" />

will often result in an *invalid* command, because the `\` escapes the quote!

    whatever "C:\path\to\output\" # ruh roh!

The fix is to instead use `$(OutputPath.OutputPath.TrimEnd('\')`,
which ensures that the path does *not* end in `\`, preventing
the unintentional escape usage:

    whatever "C:\path\to\output" # yay!
@grendello grendello force-pushed the dev/grendel/clr-host branch from 620f785 to 8583a0e Compare February 24, 2025 23:16
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.

4 participants