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

IL Linker Intrinsics + Marshal Methods, oh my! #8155

Closed
jonpryor opened this issue Jun 28, 2023 · 5 comments · May be fixed by #8156
Closed

IL Linker Intrinsics + Marshal Methods, oh my! #8155

jonpryor opened this issue Jun 28, 2023 · 5 comments · May be fixed by #8156
Assignees
Labels
Area: App Runtime Issues in `libmonodroid.so`.

Comments

@jonpryor
Copy link
Member

jonpryor commented Jun 28, 2023

Android application type

.NET Android (net7.0-android, etc.)

Affected platform version

.NET 8 in main (ff6eb66)

Description

The .NET Linker will apply optimizations that can result in per-ABI assemblies, see e.g. https://github.com/dotnet/runtime/blob/318c0e6708ced35180fd5218170f82246e2f2bac/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.64bit.xml

LLVM Marshal Methods (8bc7a3e) updates assemblies.

What happens if we mix these together?

Steps to Reproduce

See intrinsics+mm.zip, which is:

  1. Create an Android Class Library project: dotnet new androidlib -n androidlib

  2. To (1), add MyRunner.java, which contains a virtual/abstract/etc. method. (There needs to be virtual methods so that the assembly contains marshal methods.)

    package com.example.androidlib;
    public abstract class MyRunner {
        public static void run(MyRunner r) {
            r.run();
        }
    
        public abstract void run();
    }
  3. To (1), update Class1.cs to inherit from the MyRunner type and override the method. Additionally, Class1.cs (or some other type in the same project/assembly) should use IntPtr.Size:

    namespace androidlib;
    
    public class Class1 : Com.Example.Androidlib.MyRunner
    {
        public static readonly bool Is64Bit = IntPtr.Size >= 8;
    
        public override void Run ()
        {
            Console.WriteLine("androidlib.Class1.Run!  IntPtr.Size={0}", IntPtr.Size);
        }
    }
  4. Enable trimming for androidlib.csproj:

     <IsTrimmable>True</IsTrimmable>
  5. Create an Android App project: dotnet new androidlib -n android

  6. Update android.csproj to reference androdilib.csproj:

    <ProjectReference Include="..\androidlib\androidlib.csproj" />
  7. Update MainActivity.cs to use the types from (2), (3):

    Com.Example.Androidlib.MyRunner.Run(new androidlib.Class1())

Build & run the resulting app in Release configuration.

The app builds successfully:

% ../../dotnet-local.sh build -c Release -p:AndroidSdkDirectory=$HOME/android-toolchain/sdk
# no errors

The app runs successfully:

% ../../dotnet-local.sh build -c Release -p:AndroidSdkDirectory=$HOME/android-toolchain/sdk -t:Install
# side-note: why does `Install` *re-optimize* assemblies?
MSBuild version 17.7.0-preview-23316-03+0fdab8fb8 for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.
  androidlib -> …/intrinsics+mm/androidlib/bin/Release/net8.0-android/androidlib.dll
  android -> …/intrinsics+mm/android/bin/Release/net8.0-android/android.dll
  androidlib -> …/intrinsics+mm/androidlib/bin/Release/net8.0-android/androidlib.dll
  Optimizing assemblies for size may change the behavior of the app. Be sure to test after publishing. See: https://aka.ms/dotnet-illink
  Optimizing assemblies for size may change the behavior of the app. Be sure to test after publishing. See: https://aka.ms/dotnet-illink
…
  [1/11] _Microsoft.Android.Resource.Designer.dll -> _Microsoft.Android.Resource.Designer.dll.so
  [1/11] android.dll -> android.dll.so
…

% ../../dotnet-local.sh build -c Release -p:AndroidSdkDirectory=$HOME/android-toolchain/sdk -t:StartAndroidActivity

However, the app is "wrong". Because we set $(IsTrimmable)=True on androidlib.dll, the linker will "inline" IntPtr.Size to a constant value based on the architecture, 8 for 64-bit platforms, 4 for 32-bit platforms. But…

% ikdasm obj/Release/net8.0-android/android-arm64/linked/shrunk/androidlib.dll
…
  .method public hidebysig virtual instance void 
          Run() cil managed
  {
    // Code size       21 (0x15)
    .maxstack  8
    IL_0000:  ldstr      "androidlib.Class1.Run!  IntPtr.Size={0}"
    IL_0005:  ldc.i4     0x4
    IL_000a:  box        [System.Private.CoreLib]System.Int32
    IL_000f:  call       void [System.Console]System.Console::WriteLine(string,
                                                                        object)
    IL_0014:  ret
  } // end of method Class1::Run

We can see that IntPtr.Size was inlined to 4 for arm64, where it should be 8!

Additionally, all of the androidlib.dll assemblies are identical!

% find obj -iname androidlib.dll | xargs shasum 
0dc7a87d79f4507abf22e258f4c7c5a151ec8321  obj/Release/net8.0-android/android-arm/linked/shrunk/androidlib.dll
0dc7a87d79f4507abf22e258f4c7c5a151ec8321  obj/Release/net8.0-android/android-arm/linked/androidlib.dll
0dc7a87d79f4507abf22e258f4c7c5a151ec8321  obj/Release/net8.0-android/android-x86/linked/shrunk/androidlib.dll
0dc7a87d79f4507abf22e258f4c7c5a151ec8321  obj/Release/net8.0-android/android-x86/linked/androidlib.dll
0dc7a87d79f4507abf22e258f4c7c5a151ec8321  obj/Release/net8.0-android/android-arm64/linked/shrunk/androidlib.dll
0dc7a87d79f4507abf22e258f4c7c5a151ec8321  obj/Release/net8.0-android/android-arm64/linked/androidlib.dll
0dc7a87d79f4507abf22e258f4c7c5a151ec8321  obj/Release/net8.0-android/android-x64/linked/shrunk/androidlib.dll
0dc7a87d79f4507abf22e258f4c7c5a151ec8321  obj/Release/net8.0-android/android-x64/linked/androidlib.dll

When running the app on a Pixel 6 (64-bit device!), we see the same erroneous behavior:

I DOTNET  : androidlib.Class1.Run!  IntPtr.Size=4

😱

Did you find any workaround?

Don't enable trimming; either don't set $(IsTrimmable) at all, or set $(IsTrimmable)=False.

I DOTNET  : androidlib.Class1.Run!  IntPtr.Size=8

Relevant log output

No response

@jonpryor jonpryor added Area: App Runtime Issues in `libmonodroid.so`. needs-triage Issues that need to be assigned. labels Jun 28, 2023
@jonpryor
Copy link
Member Author

If I update android.csproj to contain:

<RuntimeIdentifiers>android-x64;android-arm64</RuntimeIdentifiers>

i.e. only 64-bit platforms, then ikdasm obj/Release/net8.0-android/android-arm64/linked/shrunk/androidlib.dll shows that IntPtr.Size was inlined to 8, as expected.

@jonpryor
Copy link
Member Author

If I remove use of marshal methods from androidlib by removing MyRunner.java and updating Class1.cs to just use IntPtr.Size (while keeping trimming enabled):

namespace androidlib;

public class Class1
{
    public static readonly bool Is64Bit = IntPtr.Size >= 8;
}

Then the resulting app IL is as expected:

% find obj -iname androidlib.dll | xargs shasum 
289d3b268c35fc56fe28ee1658845e66666332f4  obj/Release/net8.0-android/android-arm/linked/shrunk/androidlib.dll
289d3b268c35fc56fe28ee1658845e66666332f4  obj/Release/net8.0-android/android-arm/linked/androidlib.dll
289d3b268c35fc56fe28ee1658845e66666332f4  obj/Release/net8.0-android/android-x86/linked/shrunk/androidlib.dll
289d3b268c35fc56fe28ee1658845e66666332f4  obj/Release/net8.0-android/android-x86/linked/androidlib.dll
a1da91d2d5e9ca6b38d8a5670bb16e9f06a86a9a  obj/Release/net8.0-android/android-arm64/linked/shrunk/androidlib.dll
a1da91d2d5e9ca6b38d8a5670bb16e9f06a86a9a  obj/Release/net8.0-android/android-arm64/linked/androidlib.dll
a1da91d2d5e9ca6b38d8a5670bb16e9f06a86a9a  obj/Release/net8.0-android/android-x64/linked/shrunk/androidlib.dll
a1da91d2d5e9ca6b38d8a5670bb16e9f06a86a9a  obj/Release/net8.0-android/android-x64/linked/androidlib.dll

32-bit assemblies differ from 64-bit assemblies, and:

  .field public static initonly bool Is64Bit
  .method private hidebysig specialname rtspecialname static 
          void  .cctor() cil managed
  {
    // Code size       17 (0x11)
    .maxstack  8
    IL_0000:  ldc.i4     0x8
    IL_0005:  ldc.i4.8
    IL_0006:  clt
    IL_0008:  ldc.i4.0
    IL_0009:  ceq
    IL_000b:  stsfld     bool androidlib.Class1::Is64Bit
    IL_0010:  ret
  } // end of method Class1::.cctor

on the 64-bit androidlib.dll, IntPtr.Size is inlined to 8.

@jonpryor
Copy link
Member Author

Disabling LLVM marshal methods by setting $(AndroidEnableMarshalMethods)=False is another workaround, and will result in per-arch assemblies.

@jonpryor
Copy link
Member Author

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Jun 28, 2023
jonpryor pushed a commit that referenced this issue Jul 13, 2023
Context: 929e701
Context: ce2bc68
Context: #7473
Context: #8155

The managed linker can produce assemblies optimized for the target
`$(RuntimeIdentifier)` (RID), which means that they will differ
between different RIDs.  Our "favorite" example of this is
`IntPtr.Size`, which is inlined by the linker into `4` or `8` when
targeting 32-bit or 64-bit platforms.  (See also #7473 and 929e701.)

Another platform difference may come in the shape of CPU intrinsics
which will change the JIT-generated native code in ways that will
crash the application if the assembler instructions generated for the
intrinsics aren't supported by the underlying processor.

In addition, the per-RID assemblies will have different [MVID][0]s
and **may** have different type and method metadata token IDs, which
is important because typemaps *use* type and metadata token IDs; see
also ce2bc68.

All of this taken together invalidates our previous assumption that
all the managed assemblies are identical.  "Simply" using
`IntPtr.Size` in an assembly that contains `Java.Lang.Object`
subclasses will break things.

This in turn could cause "mysterious" behavior or crashes in Release
applications; see also Issue #8155.

Prevent the potential problems by processing each per-RID assembly
separately and output correct per-RID LLVM IR assembly using the
appropriate per-RID information.

Additionally, during testing I found that for our use of Cecil within
`<GenerateJavaStubs/>` doesn't consistently remove the fields,
delegates, and methods we remove in `MarshalMethodsAssemblyRewriter`
when marshal methods are enabled, or it generates subtly broken
assemblies which cause **some** applications to segfault at run time
like so:

	I monodroid-gc: 1 outstanding GREFs. Performing a full GC!
	F libc    : Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x8 in tid 12379 (t6.helloandroid), pid 12379 (t6.helloandroid)
	F DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
	F DEBUG   : Build fingerprint: 'google/raven_beta/raven:14/UPB3.230519.014/10284690:user/release-keys'
	F DEBUG   : Revision: 'MP1.0'
	F DEBUG   : ABI: 'arm64'
	F DEBUG   : Timestamp: 2023-07-04 22:09:58.762982002+0200
	F DEBUG   : Process uptime: 1s
	F DEBUG   : Cmdline: com.microsoft.net6.helloandroid
	F DEBUG   : pid: 12379, tid: 12379, name: t6.helloandroid  >>> com.microsoft.net6.helloandroid <<<
	F DEBUG   : uid: 10288
	F DEBUG   : tagged_addr_ctrl: 0000000000000001 (PR_TAGGED_ADDR_ENABLE)
	F DEBUG   : signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0000000000000008
	F DEBUG   : Cause: null pointer dereference
	F DEBUG   :     x0  0000000000000000  x1  0000007ba1401af0  x2  00000000000000fa  x3  0000000000000001
	F DEBUG   :     x4  0000007ba1401b38  x5  0000007b9f2a8360  x6  0000000000000000  x7  0000000000000000
	F DEBUG   :     x8  ffffffffffc00000  x9  0000007b9f800000  x10 0000000000000000  x11 0000007ba1400000
	F DEBUG   :     x12 0000000000000000  x13 0000007ba374ad58  x14 0000000000000000  x15 00000013ead77d66
	F DEBUG   :     x16 0000007ba372f210  x17 0000007ebdaa4a80  x18 0000007edf612000  x19 000000000000001f
	F DEBUG   :     x20 0000000000000000  x21 0000007b9f2a8320  x22 0000007b9fb02000  x23 0000000000000018
	F DEBUG   :     x24 0000007ba374ad08  x25 0000000000000004  x26 0000007b9f2a4618  x27 0000000000000000
	F DEBUG   :     x28 ffffffffffffffff  x29 0000007fc592a780
	F DEBUG   :     lr  0000007ba3701f44  sp  0000007fc592a730  pc  0000007ba3701e0c  pst 0000000080001000
	F DEBUG   : 8 total frames
	F DEBUG   : backtrace:
	F DEBUG   :       #00 pc 00000000002d4e0c  /data/app/~~Av24J15xbf20XdrY3X2_wA==/com.microsoft.net6.helloandroid-4DusuNWIAkz1Ssi7fWVF-g==/lib/arm64/libmonosgen-2.0.so (BuildId: 761134f2369377582cc3a8e25ecccb43a2e0a877)
	F DEBUG   :       #1 pc 00000000002c29e8  /data/app/~~Av24J15xbf20XdrY3X2_wA==/com.microsoft.net6.helloandroid-4DusuNWIAkz1Ssi7fWVF-g==/lib/arm64/libmonosgen-2.0.so (BuildId: 761134f2369377582cc3a8e25ecccb43a2e0a877)
	F DEBUG   :       #2 pc 00000000002c34bc  /data/app/~~Av24J15xbf20XdrY3X2_wA==/com.microsoft.net6.helloandroid-4DusuNWIAkz1Ssi7fWVF-g==/lib/arm64/libmonosgen-2.0.so (BuildId: 761134f2369377582cc3a8e25ecccb43a2e0a877)
	F DEBUG   :       #3 pc 00000000002c2254  /data/app/~~Av24J15xbf20XdrY3X2_wA==/com.microsoft.net6.helloandroid-4DusuNWIAkz1Ssi7fWVF-g==/lib/arm64/libmonosgen-2.0.so (BuildId: 761134f2369377582cc3a8e25ecccb43a2e0a877)
	F DEBUG   :       #4 pc 00000000002be0bc  /data/app/~~Av24J15xbf20XdrY3X2_wA==/com.microsoft.net6.helloandroid-4DusuNWIAkz1Ssi7fWVF-g==/lib/arm64/libmonosgen-2.0.so (BuildId: 761134f2369377582cc3a8e25ecccb43a2e0a877)
	F DEBUG   :       #5 pc 00000000002bf050  /data/app/~~Av24J15xbf20XdrY3X2_wA==/com.microsoft.net6.helloandroid-4DusuNWIAkz1Ssi7fWVF-g==/lib/arm64/libmonosgen-2.0.so (BuildId: 761134f2369377582cc3a8e25ecccb43a2e0a877)
	F DEBUG   :       #6 pc 00000000002a53a4  /data/app/~~Av24J15xbf20XdrY3X2_wA==/com.microsoft.net6.helloandroid-4DusuNWIAkz1Ssi7fWVF-g==/lib/arm64/libmonosgen-2.0.so (mono_gc_collect+44) (BuildId: 761134f2369377582cc3a8e25ecccb43a2e0a877)
	F DEBUG   :       #7 pc 000000000000513c  <anonymous:7ec716b000>

This is because we generate Java Callable Wrappers over a set of
original (linked or not) assemblies, then we scan them for classes
derived from `Java.Lang.Object` and use that set as input to the
marshal methods rewriter, which makes the changes (generates wrapper
methods, decorates wrapped methods with `[UnmanagedCallersOnly]`,
removes the old delegate methods as well as delegate backing fields)
to all the `Java.Lang.Object` subclasses, then writes the modified
assembly to a `new/<assembly.dll>` location (efa14e2), followed by
copying the newly written assemblies back to the original location.
At this point, we have the results returned by the subclass scanner
in memory and **new** versions of those types on disk, but they are
out of sync, since the types in memory refer to the **old** assemblies,
but AOT is ran on the **new** assemblies which have a different layout,
changed MVIDs and, potentially, different type and method token IDs
(because we added some methods, removed others etc) and thus it causes
the crashes at the run time.  The now invalid set of "old" types is
passed to the typemap generator.  This only worked by accident, because
we (incorrectly) used only the first linked assembly which happened
to be the same one passed to the JLO scanner and AOT - so everything
was fine at the execution time.

Address this by *disabling* LLVM Marshal Methods (8bc7a3e) for .NET 8,
setting `$(AndroidEnableMarshalMethods)`=False by default.
We'll attempt to fix these issues for .NET 9.

[0]: https://learn.microsoft.com/dotnet/api/system.reflection.module.moduleversionid?view=net-7.0
@jpobst jpobst removed the needs-triage Issues that need to be assigned. label Jan 9, 2024
jonpryor pushed a commit that referenced this issue Jan 18, 2024
Context: c927026
Context: 6836818
Context: 929e701
Context: #8478
Context: #8155
Context: #8168

Our build process has a bit of a "consistent sanity" problem: large
portions of the build process assume that the output of a `.csproj`
is a single assembly, and that single assembly is (eventually)
embedded into the `.apk`, to be loaded at runtime.

Unfortunately, that wasn't strictly true starting with .NET 5:
there were *multiple* `System.Private.CoreLib.dll` assemblies, which
needed to be treated specially; see also c927026.

We discovered that this assumption was even *less* true because of
the linker, which would quite happily *replace* `IntPtr.get_Size()`
method invocations with a *constant*, specific for the target ABI;
see also 6836818, 929e701.  This in turn could be responsible for
all sorts of weirdness if e.g. a 64-bit assembly were used in a
32-bit process, or vice versa.

Which brings us to the original assumption: there is (usually) only
one "source" assembly, which is (optionally) linked into one "linked"
assembly, which is packaged into one assembly in the `.apk`.

With the linker, though, we can have one "source" assembly, which
when linked becomes *N* assemblies, which *should be* separately
packaged as per-ABI assemblies within the `.apk`, but
*probably aren't*, which we *think* is the "root cause" of #8155.

PR #8478 is attempting fix this assumption, imbuing the build system
with knowledge that the linker may produce *multiple outputs* for a
single input assembly.

Unfortunately, in trying to fix things, various intermediate assembly
locations have *changed*, which in turn breaks
[AndroidX Migration in Xamarin.Forms][0], as it makes assumptions
about where various assemblies are located within `obj`:

	(_AndroidXCecilfy target) ->
	/Users/runner/.nuget/packages/xamarin.androidx.migration/1.0.8/buildTransitive/monoandroid90/Xamarin.AndroidX.Migration.targets(227,9): error : Source assembly does not exist: 'obj/Debug/android/assets/UnnamedProject.dll'.

as [`@(_AndroidXFileToCecilfy)`][1] uses
`$(MonoAndroidIntermediateAssetsDir)`, which does not account for the
ABI which is now in the path:

	<ItemGroup>
	  <_AndroidXFileToCecilfy Include="@(ResolvedUserAssemblies->'$(MonoAndroidIntermediateAssetsDir)%(Filename)%(Extension)')"
	    Condition="('%(ResolvedUserAssemblies.TargetFrameworkIdentifier)' == 'MonoAndroid' or '%(ResolvedUserAssemblies.HasMonoAndroidReference)' == 'true') and ('%(ResolvedUserAssemblies.AndroidXSkipAndroidXMigration)' != 'true')" />
	</ItemGroup>

Given that AndroidX Migration is mostly for Xamarin.Forms customers
(and *kinda* buggy, and unmaintained), and MAUI doesn't support the
Android Support libraries, and thus doesn't need AndroidX Migration,
we'd like to just *not worry about this*.

The problem?  The above error message is not actionable, and doesn't
tell anybody how to fix it.

Introduce a new `XA1039` *actionable* error in .NET 9:

	error XA1039: The Android Support libraries are not supported in .NET 9 and later,
	please migrate to AndroidX. See https://aka.ms/xamarin/androidx for more details.

The XA1039 error is generated if any NuGet packages are found matching:

  * `Xamarin.Android.Support.*`
  * `Xamarin.Android.Arch.*`

TODO: "port" XA1039 to .NET 8 *as a warning*, so that customers will
have some time to migrate off of the Android Support libraries before
.NET 9 is released in 2024-Nov.

	--<AndroidError Code="XA1039"
	++<AndroidWarning Code="XA1039"
	    ResourceName="XA1039"
	    Condition=" '@(_AndroidUnsupportedPackages->Count())' != '0' "
	/>

The biggest impact here is going to be many of our old tests, which
use the support libraries in various forms.

Improvements & general cleanup:

  * Removed all old/unused packages in `KnownPackages.cs`

  * Updated `KnownPackages.cs` to latest versions, including Xamarin.Forms

  * [Android Wear tests are now migrated from support to AndroidX][2]

  * `AndroidUpdateResourcesTest.CheckEmbeddedSupportLibraryResources()`
    is renamed to `.CheckEmbeddedAndroidXResources()`.

  * `BuildTest2.BuildHasNoWarnings()` was not appropriately applying
    `IsRelease`.

  * `Android.Support.v8.RenderScript` removed in favor of an inline
    `.so` file.

  * A few tests that used support libraries to create a project large
    numbers of dependencies, moved to
    `XamarinFormsAndroidApplicationProject`.

  * `IncrementalBuildTest.ResolveLibraryProjectImports()` now sorts
    items before comparing them.

  * `XamarinFormsAndroidApplicationProject` has a workaround for Guava:  
    <dotnet/android-libraries#535>

  * Fix a bug in `AndroidFastDeploymentType=Assemblies::Dexes`;
    see "AndroidFastDeploymentType=Assemblies::Dexes" section, below.

Removed tests:

  * `AndroidXMigration`, `AndroidXMigrationBug`

  * `ResolveLibraryImportsWithReadonlyFiles`, seemed duplicate of other
    Android Wear tests, and used support libraries.

  * `ExtraAaptManifest` as it depends on `Xamarin.Android.Fabric` and
    `Xamarin.Android.Crashlytics`. These are deprecated and depend on
    support libraries.

  * `BuildProguardEnabledProjectSource` now only tests `Release` mode.
    Since updating to AndroidX, `Debug` mode was triggering multi-dex.
    Making it difficult to assert contents of `*.dex` files.


~~ AndroidFastDeploymentType=Assemblies::Dexes ~~

The runtime test in `ApplicationRunsWithDebuggerAndBreaks()` was
crashing at runtime with:

	E monodroid-assembly: typemap: failed to stat TypeMap index file '/data/user/0/com.xamarin.applicationrunswithdebuggerandbreaks/files/.__override__/typemaps/typemap.index': No such file or directory
	F monodroid-assembly: typemap: unable to load TypeMap data index from '/data/user/0/com.xamarin.applicationrunswithdebuggerandbreaks/files/.__override__/typemaps/typemap.index'

This only happens when `AndroidFastDeploymentType=Assemblies::Dexes` is
used, as it is the case when typemap files like this are fast deployed
and used at runtime.

What was even more odd, was the file seems to exist after a
`-t:Install`, but ends up missing after `-t:Run`:

	> adb shell run-as com.xamarin.applicationrunswithdebuggerandbreaks ls -la files/.__override__/typemaps/typemap.index
	ls: files/.__override__/typemaps/typemap.index: No such file or directory

It appears that `-t:Install` successfully deploys the file:

	Pushed 3969 to /data/local/tmp/.xatools/typemap.index
	DEBUG RunShellCommand emulator-5554 "run-as" "com.xamarin.applicationrunswithdebuggerandbreaks" "--user" "0" "files/.__tools__/xamarin.cp" "/data/local/tmp/.xatools/typemap.index" "files/.__override__/typemaps/typemap.index" "1705432079367" [5ms]
	files/.__tools__/xamarin.cp returned: moved [/data/local/tmp/.xatools/typemap.index] to [files/.__override__/typemaps/typemap.index] modifieddate [1705432079367]
	moved /data/local/tmp/.xatools/typemap.index to files/.__override__/typemaps/typemap.index
	Installed files/.__override__/typemaps/typemap.index. [12ms]
	NotifySync CopyFile obj\Debug\android\typemaps\typemap.index. [0ms]

But then `-t:Run` deletes the file!

	Remove redundant file files/.__override__/typemaps/typemap.index
	DEBUG RunShellCommand 0A041FDD400327 "run-as" "com.xamarin.applicationrunswithdebuggerandbreaks" "rm" "-Rf" "files/.__override__/typemaps/typemap.index" [29ms]

This happens because the `@(_AndroidTypeMapping)` item group is empty
during an incremental build:

  * The `<GenerateJavaStubs/>` MSBuild task, during the first build
    outputs `@(_AndroidTypeMapping)` items

  * During an incremental build, the `_GenerateJavaStubs` MSBuild
    *target* is skipped, and so the `@(_AndroidTypeMapping)` item
    group is empty!

  * The `<FastDeploy/>` task happily deletes files that it thinks
    should be removed.

For now, let's add logic to the `_GenerateJavaStubs` target to fill
in the `@(_AndroidTypeMapping)` item group during incremental builds:

	<ItemGroup Condition=" '$(_InstantRunEnabled)' == 'True' and '@(_AndroidTypeMapping->Count())' == '0' ">
	  <_AndroidTypeMapping Include="$(_NativeAssemblySourceDir)typemaps\*" />
	</ItemGroup>

`<ItemGroup>`s are still evaluated when a target is *skipped*,
solving the problem.

I assume this is working in `main`, because Xamarin.AndroidX.Migration
package was involved.  It likely was running the `_GenerateJavaStubs`
target on every build.

[0]: https://learn.microsoft.com/xamarin/xamarin-forms/platform/android/androidx-migration
[1]: https://github.com/xamarin/AndroidX/blob/17e596fafe20331d7feb69240c38e0fbdc3ea640/source/migration/BuildTasks/Xamarin.AndroidX.Migration.targets#L205-L206
[2]: https://android-developers.googleblog.com/2016/04/build-beautifully-for-android-wear.html
jonpryor pushed a commit that referenced this issue Mar 15, 2024
Fixes: #8168

Context: #4337
Context: #8155

Context: 55e5c34
Context: 6836818
Context: 929e701
Context: c927026
Context: 2f19238

Issue #8155 noted a *fundamental* mismatch in
expectations between the Classic Xamarin.Android packaging worldview
and the .NET worldview: In Classic Xamarin.Android, all assemblies
are presumed to be architecture agnostic ("AnyCPU"), while in .NET:

 1. `System.Private.CoreLib.dll` was *always* an architecture-specific
    assembly (see #4337), and

 2. The .NET Trimmer is extensible and can apply ABI-specific changes
    to IL which *effectively* results in an architecture-specific
    assembly (#8155).  Meanwhile, there is no
    way of knowing that this is happening, and the trimmer doesn't
    mark the resulting assembly as architecture-specific.

We long tried to "paper over" this difference, by trying to find --
and preserve the "nature" of -- architecture-agnostic assemblies
(55e5c34, …).  Unfortunately, all attempts at trying to preserve the
concept of architecture-agnostic assemblies have failed; we're
fighting against .NET tooling in attempting to do so.

In commit 6836818 this came to a head: a long worked-on feature
LLVM Marshal Methods (8bc7a3e) had to be disabled because of
hangs within MAUI+Blazor Hybrid+.NET Android apps, and we suspect
that treating an assembly as architecture-agnostic when it was
"actually" architecture-specific is a plausible culprit.

Bite the bullet: there is no longer such a thing as an architecture-
agnostic assembly.  Treat *all* assemblies as if they were
architecture-specific.

Additionally, alter assembly packaging so that instead of using
`assemblies/assemblies*.blob` files (c927026), we instead store the
assemblies within `lib/ABI` of the `.apk`/`.aab`.

The Runtime config blob `rc.bin` is stored as `lib/ABI/libarc.bin.so`.

When `$(AndroidUseAssemblyStore)`=true, assemblies will be stored
within `lib/ABI/libassemblies.ABI.blob.so`, e.g.
`lib/arm64-v8a/libassemblies.arm64-v8a.blob.so`.

When `$(AndroidUseAssemblyStore)`=false and Fast Deployment is *not*
used, then assemblies are stored individually within `lib/ABI` as
compressed assembly data, with the following "name mangling"
convention:

  * Regular assemblies: `lib_` + Assembly File Name + `.so`
  * Satellite assemblies:
    `lib-` + culture + `-` + Assembly File Name + `.so`

For example, consider this selected `unzip -l` output:

	% unzip -l bin/Release/net9.0-android/*-Signed.apk | grep lib/arm64-v8a
	   723560  01-01-1981 01:01   lib/arm64-v8a/libSystem.IO.Compression.Native.so
	    70843  01-01-1981 01:01   lib/arm64-v8a/lib_Java.Interop.dll.so
	   157256  01-01-1981 01:01   lib/arm64-v8a/libaot-Java.Interop.dll.so
	     1512  01-01-1981 01:01   lib/arm64-v8a/libarc.bin.so

  * `libSystem.IO.Compression.Native.so` is a native shared library
    from .NET
  * `lib_Java.Interop.dll.so` is compressed assembly data for
    `Java.Interop.dll`
  * `libaot-Java.Interop.dll.so` contains Profiled AOT output for
    `Java.Interop.dll`
  * `libarc.bin.so` is the `rc.bin` file used by .NET runtime startup

Additionally, note that Android limits the characters that can be
used in native library filenames to the regex set `[-._A-Za-z0-9]`.

TODO: No error checking is done to ensure that "Assembly File Name"
stays within the limits of `[-.A-Za-z0-9]`, e.g. if you set
`$(AssemblyName)=Emoji😅` *and `$(AndroidUseAssemblyStore)`=false,
then we'll try to add `lib/arm64-v8a/lib_Emoji😅.dll.so`, which will
fail at runtime.  This works when `$(AndroidUseAssemblyStore)`=true,
which is the default.

Pros:

  * We're no longer fighting against .NET tooling features such as
    ILLink Substitutions.

  * While `.aab` files will get larger, we expect that the actual
    `.apk` files sent to Android devices from the Google Play
    Store will be *smaller*, as the Google Play Store would always
    preserve/transmit *all* `assemblies/assemblies*.blob` files,
    while now it will be able to remove `lib/ABI/*` for unsupported
    ABIs.
    
Cons:

  * `.apk` files containing more than one ABI ***will get larger***,
    as there will no longer be "de-duping" of architecture-agnostic
    assembly data.  We don't consider this a significant concern, as
    we believe `.aab` is the predominant packaging format.

~~ All assemblies are architecture-specific ~~

Assembly pre-processing changes so that every assembly ends up in
every target architecture batch, regardless of whether its MVID
differs from its brethren or not.  This is done very early in the
build process on our side, where we make sure that each assembly
either has the `%(Abi)` metadata or is given one, and is placed in
the corresponding batch.  Further processing of those batches is
"parallel", in that no code attempts to de-duplicate the batches.


~~ Impact on Fast Deployment, `$(IntermediateOutputPath)` ~~

The changes also required us to place all the assemblies in new
locations on disk within `$(IntermediateOutputPath)` when building
the application.  (Related: 2f19238.)  Assemblies are now placed in
subdirectories named after either the target architecture/ABI or the
.NET `$(RuntimeIdentifier)`, e.g.
`obj/Release/netX.Y-android/android-arm64`.  This, in turn, affects
e.g. Fast Deployment as now the synchronized content is in the
`…/.__override__/ABI` directory on device, instead of just in
`…/.__override__`.


~~ File Formats ~~

The assembly store format (c927026) is updated to use the following
structures:

	struct AssemblyStoreHeader {
	    uint32_t magic;
	    uint32_t version;
	    uint32_t entry_count;               // Number of assemblies in the store
	    uint32_2 index_entry_count;
	    uint32_t index_size;
	};
	struct AssemblyStoreIndexEntry {
	    intptr_t name_hash;	                // xxhash of assembly filename
	    uint32_t descriptor_index;          // index into `descriptors` array
	};
	struct AssemblyStoreEntryDescriptor {
	    uint32_t mapping_index;             // index into an internal runtime array
	    uint32_t data_offset;               // index into `data` for assembly `.dll`
	    uint32_t data_size;                 // size of assembly, in bytes
	    uint32_t debug_data_offset;         // index into `data` for assembly `.pdb`; 0 if not present
	    uint32_t debug_data_size;           // size of `.pdb`, in bytes; 0 if not present
	    uint32_t config_data_offset;        // index into `data` for assembly `.config`; 0 if not present
	    uint32_t config_data_size;          // size of `.config`, in bytes; 0 if not present
	};
	struct AssemblyStoreAssemblyInfo {
	    uint32_t length;                    // bytes
	    uint8_t  name[length];
	};

`libassemblies.ABI.blob.so` has the following format, and is *not* a
valid ELF file:

	AssemblyStoreHeader                 header {…};
	AssemblyStoreIndexEntry             index [header.index_entry_count];
	AssemblyStoreAssemblyDescriptor     descriptors [header.entry_count];
	AssemblyStoreAssemblyInfo           names [header.entry_count];
	uint8_t data[];
@jonpryor
Copy link
Member Author

I believe this was fixed by #8253.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Area: App Runtime Issues in `libmonodroid.so`.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants