[Android] Add callback to host-runtime contract for getting assembly data

[grendello]

Add a new callback to host_runtime_contract that is called default assembly resolution:

// Probe the host for `path`. Sets pointer to data start and its size, if found.
// Returns true if found, false otherwise. If false, out parameter values are ignored by the runtime.
bool(HOST_CONTRACT_CALLTYPE* external_assembly_probe)(
    const char* path,
    /*out*/ void** data
    /*out*/ int64_t* size);

Contributes to #112706

This also makes fatal errors that the runtime prints to stderr get printed to logcat on Android.

This is a 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]

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

[lewing]

/azp run runtime

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[AaronRobinsonMSFT]

@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]

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]

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]

@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.

👍

[ivanpovazan]

As we have changes in hosting header files (this one and src/native/corehost/host_runtime_contract.h below), I think we could consider shipping hosting headers as part of the Android CoreCLR runtime pack?

@elinor-fung @jkotas what do you think?

[jkotas]

We have been treating the coreclr.dll hosting interface as an internal detail that is not officially documented and that is not guaranteed to be stable. Are you suggesting that we change that?

[steveisok]

Can we ship headers and still consider them not guaranteed to be stable? That's what mono does with certain parts of its hosting API.

[ivanpovazan]

In addition to what Steve said:

Currently, on dotnet/macios side the build downloads the coreclrhost.h header file to build the CoreCLR host:
https://github.com/dotnet/macios/blob/30cce24d4c18d149879fe8afa64331491535ea0b/runtime/Makefile#L38
so with every change to the file, the download URL needs to be updated to have consistent behavior. This is difficult to maintain as the version of the file needs to be aligned with the runtime pack version including the actual implementation changes.
As we are now bringing CoreCLR support for more platforms and as we are in the development stage syncing changes between the runtime and embedders/hosts, I was curious if we would want to ship the required header files as part of the runtime pack (like mono does), to avoid manual downloads or potential mismatches.

[steveisok]

There's always the option of sharing a transport package if we don't want to ship it.

[elinor-fung]

I don't think we want the internal coreclr interface as part of the shipped runtime pack. We do include public hosting headers (coreclr_delegates.h, hostfxr.h, nethost.h) in the host pack.

Transport package seems reasonable if we need a better way for other repos to get at internals.

[ivanpovazan]

@elinor-fung it seems like we are stripping this PR down to what is described in: #112706 should we rename the PR and link it to that issue, so that it becomes in-pr ?

[jkotas]

The change LGTM once we get a successful CI run.

[janvorli]

LGTM, thank you!

[elinor-fung]

Follow-ups I plan on for #112706:

• host tests for external_assembly_probe
• refactoring/renaming to separate external assembly probe from bundle / bundle file location idea
  • external assembly probe doesn't have to mean bundle - plugging into the bundle probing was convenient, since it was already hooked in to be called before all the default resolution, but from the runtime side, external assembly data from the host shouldn't equate to bundle
  • BundleFileLocation name seems misleading now that it also represents externally provided assembly data

[steveisok]

Thanks everyone for working together and helping to see this through!

[jkoritzinsky]

I think these changes may break source-build, as it effectively regresses #35727. cc: @omajid can you take a look?

@grendello What was the motivation behind this change?

[am11]

This change is expanding the coverage of #35727 to cmake configure step. How is it regressing, do you have an example?

[omajid]

From #35727:

The normal way to do that is by exporting environment variables like CFLAGS and CXXFLAGS. Fedora provides a bunch of values that we can use . But this doesn't play very nice with the liberal use of -Werror in corefx. Worse, the flags leak into various configure tests . For example HAVE_IN_PKTINFO would fail due to an unrelated warning

[grendello]

@jkoritzinsky *FLAGS must be exported before cmake configuration run, otherwise they will be ignored. Where they were previously exported, they were essentially ignored. My motivation, tbh, was to get color in errors and warnings working, but it just seemed to be a mistake to have the exports without any effect. I think it may also be a requirement for packagers in some Linux distributions. For instance, Debian can specify standard system-wide package compilation flags via various dpkg scripts and it's done by pre-setting the *FLAGS environment variables.

[am11]

it just seemed to be a mistake to have the exports without any effect.

Before this PR extra flags did had effect but only over cmake --build step, not the config step.

If something is breaking because of this, it is most likely the problem which needs to be fixed in that config.

[grendello]

@omajid The policies talk about build, but they don't mean cmake build stage. cmake is a build system generator, so it takes the *FLAGS into account only when generating the make/ninja build files, during cmake configuration phase. Here's dotnet/runtime's build.ninja:

artifacts/obj/coreclr/android.arm64.Release $ grep -e '\(CXX\|LD\|CC\)FLAGS' build.ninja | wc -l
0

artifacts/obj/coreclr/android.arm64.Release $ grep -e 'FLAGS' build.ninja | wc -l
2833

artifacts/obj/coreclr/android.arm64.Release $ grep -e 'FLAGS' build.ninja | head -1
  FLAGS = -g -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security -fno-rtti  -O3 -DNDEBUG  -std=gnu++11 -fPIE -O2 -Wall -Wno-null-conversion -glldb -fno-omit-frame-pointer -fwrapv -fstack-protector-strong -Werror -Wno-unused-variable -Wno-unused-value -Wno-unused-function -Wno-tautological-compare -Wno-unknown-pragmas -Wimplicit-fallthrough -Wvla -Wno-invalid-offsetof -Wno-unused-but-set-variable -ffp-contract=off -fno-rtti -Wno-unknown-warning-option -ferror-limit=4096 -Wno-unused-private-field -Wno-constant-logical-operand -Wno-pragma-pack -Wno-incompatible-ms-struct -Wno-reserved-identifier -Wno-unsafe-buffer-usage -Wno-single-bit-bitfield-constant-conversion -Wno-cast-function-type-strict -Wno-switch-default -Wno-nontrivial-memaccess -fsigned-char -fvisibility=hidden -ffunction-sections

As you can see, there's no mention of any of the *FLAGS - all of them were expanded and placed by cmake configuration stage in the build file.

[grendello]

@omajid if setting some flags at configure time breaks .NET tests, then the problem is in those tests and it should be fixed there. If any component is incompatible with some flags, it can (and should) check for and remove the offending flags/defines. This post gives a potential solution (not tested by me):

function(remove_compiler_flag target flag)
  foreach(property IN COMPILE_DEFINITIONS COMPILE_FEATURES COMPILE_FLAGS COMPILE_OPTIONS etc)
    # Get the target property
    # If the specified flag is in that list, remove it and re-set the target property
  endforeach()
endfunction()

[am11]

@omajid, somehow it stopped working. The cmake generated makefiles (or ninja files) do not explicitly account for $CFLAGS etc. from environment variable anymore (assuming it was working when the original issue was fixed):

FROM alpine:latest

RUN apk add --no-cache cmake make ninja gcc musl-dev

WORKDIR /app

RUN cat <<'EOF' > CMakeLists.txt
cmake_minimum_required(VERSION 3.10)
project(HelloWorld C)

add_executable(hello main.c)
EOF

RUN cat <<'EOF' > main.c
#include <stdio.h>

int main() {
    #ifdef PRE_CONFIG_FLAG
        #error pre config CFLAGS were respected!
    #else
        #error pre config CFLAGS were ignored!
    #endif

    #ifdef POST_CONFIG_FLAG
        #error post config CFLAGS were respected!
    #else
        #error post config CFLAGS were ignored!
    #endif

    return 0;
}
EOF

ENV CFLAGS="-Wall -Wextra -DPRE_CONFIG_FLAG"

RUN cmake -G Ninja -B build_ninja
RUN cmake -G "Unix Makefiles" -B build_make

ENV CFLAGS="-Wall -Wextra -DPOST_CONFIG_FLAG"

RUN ninja -C build_ninja -v || true

RUN make -C build_make VERBOSE=1

with docker build . -t foo 2>&1 | tee
both ninja and make (penultimate and last lines) show us:

#12 0.125 /app/main.c:5:10: error: #error pre config CFLAGS were respected!
#12 0.125     5 |         #error pre config CFLAGS were respected!
#12 0.125       |          ^~~~~
#12 0.125 /app/main.c:13:10: error: #error post config CFLAGS were ignored!
#12 0.125    13 |         #error post config CFLAGS were ignored!
#12 0.125       |          ^~~~~

[steveisok]

Should we revert the part of this change that impacts source build? Or are we ok with fixing outside of runtime?

[omajid]

Thanks, @am11 and @grendello !

@steveisok said:

Or are we ok with fixing outside of runtime?

Based on the comments and feedback, I think we should fix it as a separate PR. We don't know what that exactly looks like, right now.