[Android] Minimal (and quick) set of changes to make CoreCLR host on Android work #112705

grendello · 2025-02-19T18:33:52Z

This is a draft set of minimal changes required to make dotnet/android#9572 (the .NET for Android CoreCLR host) work with standard .NET for Android applications.

dotnet-policy-service · 2025-02-19T18:34:47Z

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

src/coreclr/dlls/mscoree/exports.cpp

src/native/corehost/host_runtime_contract.h

src/coreclr/vm/util.cpp

src/coreclr/vm/peimage.inl

src/coreclr/vm/peimage.cpp

src/coreclr/vm/common.h

src/coreclr/hosts/inc/coreclrhost.h

src/coreclr/dlls/mscoree/mscorwks_unixexports.src

src/coreclr/dlls/mscoree/exports.cpp

lewing · 2025-02-19T21:31:35Z

/azp run runtime

azure-pipelines · 2025-02-19T21:31:50Z

Azure Pipelines successfully started running 1 pipeline(s).

AaronRobinsonMSFT · 2025-02-19T22:25:20Z

@grendello I think a bunch of these changes make sense to me, for example, a bunch of the bundling logic. My pushback would be either we are doing something quick and dirty or we are chasing optimizations, not both. For example coreclr_initialize seems to be just fine, but we don't want to have the other side convert to UTF-8, why not? Would that make composition harder in some way? That is the existing API and we should keep that for now to get things up.

I would change the android side to conform, where possible, to existing APIs for now and put a bunch of the cost there.

jkotas · 2025-02-19T22:28:28Z

This change was incorrectly merged with main and a lot of unrelated commits from main are showing up as part of the delta. Could you please rebase against main and force push so that the delta is reviewable?

grendello · 2025-02-20T08:35:43Z

This change was incorrectly merged with main and a lot of unrelated commits from main are showing up as part of the delta. Could you please rebase against main and force push so that the delta is reviewable?

I use git merge, which is the correct way of tracking changes, because it doesn't rewrite history like git rebase does, but sure, I can rebase.

grendello · 2025-02-20T08:44:07Z

@grendello I think a bunch of these changes make sense to me, for example, a bunch of the bundling logic. My pushback would be either we are doing something quick and dirty or we are chasing optimizations, not both. For example coreclr_initialize seems to be just fine, but we don't want to have the other side convert to UTF-8, why not? Would that make composition harder in some way? That is the existing API and we should keep that for now to get things up.

Using UTF-8 strings on the Android side is easy and bears no costs for us, I can make the change without any effort. This is not the only issue with coreclr_initialize, though, and I'm going to push back against using the pointer-to-string-to-pointer conversion (as I did years ago when the same approach was suggested for MonoVM).

I would change the android side to conform, where possible, to existing APIs for now and put a bunch of the cost there.

👍

…em.native library for the singlefilehost

… R2R that acts like linux

… msbuild places instead.

Co-authored-by: Jan Kotas <[email protected]>

vitek-karas · 2025-02-21T10:00:10Z

src/coreclr/vm/bundle.cpp

+
+    if (m_probe != nullptr)
+    {
+        assemblyFound = m_probe(utf8Path, &loc.Offset, &fileSize, &compressedSize);


This is a slight change in the existing behavior. Currently the existing probe is defined as returning bool, but the code ignored it, now it doesn't. Given that we don't really expose this probe as an external API it's probably OK, but we should make it a concious decision.

Made / documented this to be an explicit check - the outs aren't used if the probe returns false.

…-clr-host-local

elinor-fung · 2025-02-21T18:15:54Z

src/native/corehost/host_runtime_contract.h

@@ -51,6 +51,12 @@ struct host_runtime_contract
    const void* (HOST_CONTRACT_CALLTYPE* pinvoke_override)(
        const char* library_name,
        const char* entry_point_name);
+
+    // Probe the host for `path`. Sets pointer to data start and its size, if found.
+    bool(HOST_CONTRACT_CALLTYPE* external_assembly_probe)(


@grendello I moved this to the end of the struct to keep the existing order. I don't think this should require updating your changes in dotnet/android, since they use designators.

src/coreclr/vm/peimagelayout.cpp

jkotas · 2025-02-21T20:04:37Z

src/coreclr/vm/peimage.cpp

+#if defined(TARGET_ANDROID)
+    m_hFile = GetData(nullptr);
+#else


Suggested change

#if defined(TARGET_ANDROID)

m_hFile = GetData(nullptr);

#else

If the GetFileHandle is still called after the other changes. fix the caller instead of this hack?

src/coreclr/vm/peimage.h

…le on Android

elinor-fung · 2025-02-21T22:47:00Z

src/coreclr/vm/bundle.cpp

@@ -64,6 +65,13 @@ BundleFileLocation Bundle::Probe(const SString& path, bool pathIsBundleRelative)
    pathBuffer.SetAndConvertToUTF8(path.GetUnicode());
    LPCSTR utf8Path(pathBuffer.GetUTF8());

+#if defined(TARGET_ANDROID)
+    // On Android we always strip away the assembly path, if any


@grendello Thoughts on doing this stripping on the Android host side instead? For the runtime, I think it would make more sense to just pass the raw path to external_assembly_probe.

AaronRobinsonMSFT · 2025-02-22T02:01:00Z

src/coreclr/inc/clrconfigvalues.h

@@ -383,6 +383,7 @@ RETAIL_CONFIG_DWORD_INFO(INTERNAL_LogToConsole, W("LogToConsole"), 0 , "Writes t
 CONFIG_DWORD_INFO(INTERNAL_LogToDebugger, W("LogToDebugger"), 0 , "Writes the CLR log to debugger (OutputDebugStringA).")
 CONFIG_DWORD_INFO(INTERNAL_LogToFile, W("LogToFile"), 0 , "Writes the CLR log to a file.")
 CONFIG_DWORD_INFO(INTERNAL_LogWithPid, W("LogWithPid"), FALSE, "Appends pid to filename for the CLR log.")
+CONFIG_DWORD_INFO(INTERNAL_LogToAndroid, W("LogToAndroid"), 0 , "Writes the CLR log to android logcat")


I don't think this is needed. Given the conversations about Android, we are really printing to the "console". I'm removing this and Android scenarios should reuse LogToConsole.

Minor nits.

dotnet-issue-labeler bot added the area-VM-coreclr label Feb 19, 2025

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 19, 2025

grendello mentioned this pull request Feb 19, 2025

CLR hosting dotnet/android#9572

Draft

AaronRobinsonMSFT reviewed Feb 19, 2025

View reviewed changes

steveisok self-requested a review February 20, 2025 01:24

grendello force-pushed the dev/grendel/android-clr-host-local branch 2 times, most recently from ff6a948 to b285800 Compare February 20, 2025 10:02

grendello and others added 7 commits February 20, 2025 13:08

Beginnings of native Android build

7b144f3

Beginnings of native Android build

06c1c83

Bump min API level to 29 (proper TLS support) and link the right syst…

62189fc

…em.native library for the singlefilehost

Fix host/target confusion in R2R tool publish / Add Android target to…

f9f7b90

… R2R that acts like linux

Beginnings of native Android build

2a1535e

Cleanup

10e88b0

Revert changes to R2R that alias Android to linux and put it in a few…

c2b4eac

… msbuild places instead.

grendello and others added 3 commits February 20, 2025 17:38

Update src/coreclr/inc/bundle.h

f398c6e

Co-authored-by: Jan Kotas <[email protected]>

Update src/coreclr/hosts/inc/coreclrhost.h

7127b59

Co-authored-by: Jan Kotas <[email protected]>

Address feedback

e1e7e32

vitek-karas reviewed Feb 21, 2025

View reviewed changes

steveisok and others added 3 commits February 21, 2025 09:04

Merge remote-tracking branch 'upstream/main' into dev/grendel/android…

58b2698

…-clr-host-local

Remove some logging related things since Elinor's PR landed

7d4254a

Fix HostRuntimeContract tests

a16e3ed

elinor-fung reviewed Feb 21, 2025

View reviewed changes

elinor-fung added 3 commits February 21, 2025 10:49

AndroidGetDataStart -> GetData

99e2bbf

Don't try to manage external assembly not mapped by the runtime

cf72fbe

Remove newline-only changes

f286e15

jkotas reviewed Feb 21, 2025

View reviewed changes

src/coreclr/vm/peimagelayout.cpp Outdated Show resolved Hide resolved

elinor-fung added 2 commits February 21, 2025 11:44

Explicitly check / document checking return value of probes

1533d9f

Check for non-file data instead of assuming that is the case on Android

1cdb0ce

jkotas reviewed Feb 21, 2025

View reviewed changes

src/coreclr/vm/peimagelayout.cpp Outdated Show resolved Hide resolved

jkotas reviewed Feb 21, 2025

View reviewed changes

src/coreclr/vm/peimage.h Outdated Show resolved Hide resolved

elinor-fung added 6 commits February 21, 2025 12:17

Remove unnecessary check

5a97aff

Don't set file handle for non-file data

885d117

Check for null data instead of always returns FALSE for PEImage::IsFi…

54ad33c

…le on Android

GetData -> GetExternalData

2821e75

Remove AndroidGetAppName

ff9021d

Remove BundleFileLocation::AppName

38c6376

elinor-fung reviewed Feb 21, 2025

View reviewed changes

AaronRobinsonMSFT reviewed Feb 22, 2025

View reviewed changes

Remove LogToAndroid. Scenario should now use LogToConsole.

8c60c1a

Minor nits.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android] Minimal (and quick) set of changes to make CoreCLR host on Android work #112705

[Android] Minimal (and quick) set of changes to make CoreCLR host on Android work #112705

grendello commented Feb 19, 2025

dotnet-policy-service bot commented Feb 19, 2025

lewing commented Feb 19, 2025

azure-pipelines bot commented Feb 19, 2025

AaronRobinsonMSFT commented Feb 19, 2025

jkotas commented Feb 19, 2025

grendello commented Feb 20, 2025

grendello commented Feb 20, 2025

vitek-karas Feb 21, 2025

elinor-fung Feb 21, 2025

elinor-fung Feb 21, 2025

jkotas Feb 21, 2025

elinor-fung Feb 21, 2025

AaronRobinsonMSFT Feb 22, 2025

[Android] Minimal (and quick) set of changes to make CoreCLR host on Android work #112705

Are you sure you want to change the base?

[Android] Minimal (and quick) set of changes to make CoreCLR host on Android work #112705

Conversation

grendello commented Feb 19, 2025

dotnet-policy-service bot commented Feb 19, 2025

lewing commented Feb 19, 2025

azure-pipelines bot commented Feb 19, 2025

AaronRobinsonMSFT commented Feb 19, 2025

jkotas commented Feb 19, 2025

grendello commented Feb 20, 2025

grendello commented Feb 20, 2025

vitek-karas Feb 21, 2025

Choose a reason for hiding this comment

elinor-fung Feb 21, 2025

Choose a reason for hiding this comment

elinor-fung Feb 21, 2025

Choose a reason for hiding this comment

jkotas Feb 21, 2025

Choose a reason for hiding this comment

elinor-fung Feb 21, 2025

Choose a reason for hiding this comment

AaronRobinsonMSFT Feb 22, 2025

Choose a reason for hiding this comment