[native] Real shared libraries in APK `/lib` directories by grendello · Pull Request #9154 · dotnet/android

grendello · 2024-07-29T15:28:38Z

No description provided.

jonathanpeppers · 2024-08-27T13:44:20Z

+				Console.WriteLine (message);
+			} else {
+				Console.WriteLine ($"Extracted content from ELF image '{path}'");
+			}


Should these be going to an MSBuild log instead of Console? They won't appear in log files.

I'm on a fence about these. I find them useful when looking for reasons why a test failed because of changes in the assembly store etc formats when I run the tests locally, but they're not going to be useful in general so they'd just pollute logs for no real gain.

The only downside is they will be emitting stuff on users machines as well, especially those building on VScode or CLI. And no amount of setting the log verbosity will get rid of them.

They are only used in tests, this code doesn't run as part of a regular app build.

jonathanpeppers · 2024-08-27T13:47:15Z

    "lib/arm64-v8a/lib__Microsoft.Android.Resource.Designer.dll.so": {
-      "Size": 1114
+      "Size": 18208
    },
    "lib/arm64-v8a/lib_Java.Interop.dll.so": {
-      "Size": 69368
+      "Size": 86456
    },
    "lib/arm64-v8a/lib_Mono.Android.dll.so": {
-      "Size": 98691
+      "Size": 115720
    },


Why are these so much larger now?

Should we actually remove settings like AndroidUseAssemblyStore=false and test the defaults instead?

android/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest2.cs

Line 157 in 0297b5c

proj.SetProperty ("AndroidUseAssemblyStore", "False");

They are larger because they not only have the real ELF header and sections, they are also aligned to the 16k boundary between sections.

A far as defaults are concerned, there are other tests which use them, so I think it's fine to leave the assembly store disabled here.

jonpryor · 2024-09-10T19:21:48Z

+					onOutput?.Invoke (s, e);
+					stdoutLines.Add (e.Data);
+				} else
+					stdout_completed.Set ();


e.Data is null when the process exits?! I was not aware of that, nor do I see that documented at Process.OutputDataReceived.

The following code also feels odd, as it suggests that events may still be raised after proc.WaitForExit(), hence the need for e.g. stderr_completed.WaitOne(), which just feels odd.

This pattern was established some 10-11 years ago, we've seen all these conditions happen along the years (especially when I worked with the Xamarin Installer, but also in Mono Develop/Xamarin Studio). e.Data == null is documented here:

When the redirected stream is closed, a null line is sent to the event handler. Ensure your event handler checks the [Data](https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.datareceivedeventargs.data?view=net-8.0) property appropriately before accessing it. For example, you can use the static method [String.IsNullOrEmpty](https://learn.microsoft.com/en-us/dotnet/api/system.string.isnullorempty?view=net-8.0) to validate the [Data](https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.datareceivedeventargs.data?view=net-8.0) property in your event handler.

Process exiting means only that the process itself is gone, its output streams may still have data that needs to be piped to us, hence the need to wait.

jonpryor · 2024-09-10T19:26:01Z

+
+			if (proc.ExitCode != 0) {
+				var sb = MergeStdoutAndStderrMessages (stdoutLines, stderrLines);
+				log.LogError ($"Command '{psi.FileName} {psi.Arguments}' failed.\n{sb.ToString ()}");


I think there should be a slight re-think on this, as this will be an uncoded error message, if and when it happens.

"Better" might be to do one of two things:

Add a string taskCode parameter, allowing us to log.LogCodedError(taskCode, …), or

Replace the TaskLoggingHelper parameter with an AndroidTask parameter, allowing this method access to .Log and .TaskPrefix. We'd still have to "concoct" an error code -- ${task.TaskPrefix}0000? -- but it's better than no code at all, as error codes are logged via telemetry.

TODO: config files, runtime config blob

Handling individual assemblies loaded from the filesystem and the apk is next.

...let's see what breaks on CI

jonpryor · 2024-09-12T01:48:26Z

+					elf_header->e_ident[EI_MAG1] != ELFMAG1 ||
+					elf_header->e_ident[EI_MAG2] != ELFMAG2 ||
+					elf_header->e_ident[EI_MAG3] != ELFMAG3) {
+						log_debug (LOG_ASSEMBLY, "Not an ELF image");


Could this include the filename/etc. of what exactly is "not an ELF image"?

jonpryor · 2024-09-12T01:51:35Z

+namespace xamarin::android {
+	struct ArchiveDSOStubConfig
+	{
+		static inline constexpr size_t PayloadSectionAlignment = @ARCHIVE_DSO_STUB_PAYLOAD_SECTION_ALIGNMENT@;


Why a .hh.in file instead of #ifdefs or if constexpr?

Fields are assigned values that are detected by CMakeLists.txt after the stub is built. The stub size, layout etc may differ even within the same target platform if the compiler (e.g. a flag change which affects sections that are present/absent) or linker (e.g. order of section and, thus, size of the entire stub) change, and we must absolutely have the correct values here. Getting these values require reading the stub ELF file and doing some calculations after the payload section is added, since the stub itself is compiled without it (we want to ship the stub without the payload section) - this is what code in CMakeLists.txt does.

jonpryor · 2024-09-12T01:54:09Z

Draft WIP commit message:

Context: https://developer.android.com/guide/practices/page-sizes
Context: https://android-developers.googleblog.com/2024/08/adding-16-kb-page-size-to-android.html
Context: 86260ed36dfe1a90c8ed6a2bb1cd0607d637f403

Android will "soon" require that native libraries on 64-bit platforms
(arm64-v8a, x64) be aligned on a 16KB page size.  Our *suspicion* is
that this will in turn require that anything stored within
`lib/ABI/lib*.so` entries within the `.apk` *must also* be *real*
native libraries.

Currently that is *not* the case (86260ed3).

Bundle non-native library data into real native libraries by using
[`llvm-objcopy --add-section payload=FILE`][0], which adds an ELF
section named `payload` with the contents of `FILE`, a'la:

	cp path/to/libarchive-dso-stub.so libarc.bin.so
	llvm-objcopy --add-section payload=rc.bin libarc.bin.so && \
	  llvm-objcopy --set-section-flags payload=readonly,data --set-section-alignment payload=ALIGNMENT libarc.bin.so

Note the use of `llvm-objcopy --set-section-alignment` which allows
the resulting `lib*.so` to have e.g. 16KB alignment.

If any of the the `llvm-objcopy` commands fail, an XA0142 error will
be generated:

	Command 'llvm-objcopy …' failed.
	<stdout and stderr from failing command…>

To make this work:

 1. Build a "stub" `libarchive-dso-stub.so` for each supported ABI,
    and include this in the .NET for Android workload.

 2. Update the .NET for Android workload to now include the
    OS-specfic `llvm-objcopy` command, built separately from
    [dotnet/android-native-tools][1]; see also e.g. 3eec7e9f.

Note: `*.apkdesc` files show size increases, because assembly &
related data are now wrapped in a 16KB-aligned `.so`, which causes
each such entry to increase in size by ~16KB.

[0]: https://www.llvm.org/docs/CommandGuide/llvm-objcopy.html
[1]: https://github.com/dotnet/android-native-tools

Fix indentation

…)" This reverts commit 5ebcb1d.

)" This reverts commit b795a65.

grendello force-pushed the dev/grendel/real-dsos-in-lib branch 2 times, most recently from e3b5ff1 to 0b83566 Compare July 30, 2024 20:27

grendello force-pushed the dev/grendel/real-dsos-in-lib branch 5 times, most recently from 59c52cb to 09114b5 Compare August 23, 2024 10:44

grendello force-pushed the dev/grendel/real-dsos-in-lib branch 2 times, most recently from e8016b7 to 93d5741 Compare August 27, 2024 08:47

grendello marked this pull request as ready for review August 27, 2024 11:55

grendello requested review from dellis1972, jonathanpeppers and jonpryor as code owners August 27, 2024 11:55

jonathanpeppers reviewed Aug 27, 2024

View reviewed changes

grendello force-pushed the dev/grendel/real-dsos-in-lib branch from 93d5741 to 6e6e24c Compare September 10, 2024 19:17

jonpryor reviewed Sep 10, 2024

View reviewed changes

grendello added 13 commits September 11, 2024 12:22

DSO stub

cc9b685

Build archive DSO stub in a separate step

d55e170

Wrapping works for some items already

e6cacf0

TODO: config files, runtime config blob

Wrapping continued

44fdba4

Handling individual assemblies loaded from the filesystem and the apk is next.

Things appear to work fine locally...

f9b71fa

...let's see what breaks on CI

Add support for stores inside real ELF libraries

dbc7e46

Don't use generics here

259e4b7

Well. OK. Umm. Fixed.

38e0bd5

Let's see what that'll do

fcc7d6e

Fixes

525fcf0

Bump LLVM toolchain to 18.1.8

38ae1bc

Back to LLVM 18.1.6

3f7b329

Update apkdesc

f3f19c2

Use a coded error

d55e91a

grendello force-pushed the dev/grendel/real-dsos-in-lib branch from 6e6e24c to d55e91a Compare September 11, 2024 11:00

jonpryor reviewed Sep 12, 2024

View reviewed changes

grendello and others added 3 commits September 12, 2024 11:45

Address feedback

e20ec1d

Update TOC.yml

2377f07

Update embedded-assemblies.hh

cc8fffa

Fix indentation

jonpryor merged commit 5ebcb1d into main Sep 24, 2024

jonpryor deleted the dev/grendel/real-dsos-in-lib branch September 24, 2024 14:38

jonathanpeppers added a commit that referenced this pull request Sep 26, 2024

Revert "[native] Real shared libraries in APK /lib directories (#9154…

b795a65

…)" This reverts commit 5ebcb1d.

jonathanpeppers added a commit that referenced this pull request Sep 30, 2024

Reapply "[native] Real shared libraries in APK /lib directories (#9154

18ccf84

)" This reverts commit b795a65.

jonathanpeppers added a commit that referenced this pull request Sep 30, 2024

Reapply "[native] Real shared libraries in APK /lib directories (#9154

328f681

)" This reverts commit b795a65.

jonathanpeppers added a commit that referenced this pull request Sep 30, 2024

Reapply "[native] Real shared libraries in APK /lib directories (#9154

944c794

)" This reverts commit b795a65.

github-actions Bot locked and limited conversation to collaborators Oct 25, 2024

Uh oh!

Conversation

grendello commented Jul 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonpryor commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonpryor commented Sep 12, 2024 •

edited

Loading