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 bits #9778

Merged
merged 5 commits into from
Feb 14, 2025
Merged

CLR hosting bits #9778

merged 5 commits into from
Feb 14, 2025

Conversation

grendello
Copy link
Contributor

@grendello grendello commented Feb 10, 2025

Another portion of relatively stable bits from #9572

This code isn't compiled here, the PR merely serves the purpose of making #9572 smaller and
easier to review.

The PR copies 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, class in which
the code lives.

Only the code that's actually used in #9572 is copied. This is tactics which 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 grendello requested a review from jonpryor as a code owner February 10, 2025 19:38
namespace xamarin::android {
class Constants
{
#if INTPTR_MAX == INT64_MAX
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need to care about this at this time. CoreCLR only supports 64-bit ABIs on Linux, and this is unlikely to change.

Copy link
Member

@filipnavara filipnavara Feb 14, 2025

Choose a reason for hiding this comment

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

Au contraire. The 32-bit support is nearly working except for corehost build (which is not used on Android anyway), for both x86 and arm32. (linux-x86/android-x86 is on community support; linux-arm32 is supported and tested platform, there's no blocker for android-arm32 aside from packaging and build scripts, for NativeAOT the linux-bionix-arm32 runtime packs were released since .NET 9)

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 wasn't working when I started to work on #9572 but the goal is that it'll work, so we should always keep 32-bit runtimes in mind here.

@@ -0,0 +1,209 @@
#pragma once
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to share code like this across MonoVM & CoreCLR? I worry about future bug fixing and duplication.

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 want to share some code, yes. This actually might be a good candidate for sharing. I'll update the PR shortly.

private:
static inline LogTimingCategories _log_timing_categories;
static inline bool _gc_spew_enabled = false;
static inline FILE *_gref_log = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

…though this shows the benefit of duplication: CoreCLR will not be using this GREF log once it has a GC bridge.

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, logger.hh won't be shared

@grendello grendello force-pushed the dev/grendel/clr-bits-one branch from ad96d94 to 70b7479 Compare February 14, 2025 15:38
@grendello
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@grendello
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor
Copy link
Member

Context: https://github.com/dotnet/android/pull/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)

@jonpryor jonpryor merged commit a4801d0 into main Feb 14, 2025
59 checks passed
@jonpryor jonpryor deleted the dev/grendel/clr-bits-one branch February 14, 2025 20:39
grendello added a commit that referenced this pull request Feb 14, 2025
* main:
  [CoreCLR] Add src/native/clr for CoreCLR hosting bits (#9778)
  [Xamarin.Android.Build.Tasks] move .NET 8 support to `android-net8` workload (#9785)
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.

3 participants