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

Native call tracing infrastructure + native build system overhaul #8857

Merged
merged 36 commits into from
May 16, 2024

Conversation

grendello
Copy link
Member

@grendello grendello commented Apr 4, 2024

Migrating code from #8800

This PR implements native method call tracing infrastructure based on
libunwind, initially used in
tracing the reasons for why Blazor apps break when marshal methods are
in use. Utility of such infrastructure, however, is beyond just that
particular task, it should be available for general use by both us and
applications.

This PR does not enable the tracing support, it merely provides the
entire infrastructure to do so at a later date. Part of that infrastructure
is the new layout of the native code build system:

  • All of the native bits we build are now placed under src/native/
  • A lot of code which was part of src/monodroid/ is now in several
    directories under src/native/.
  • A single top-level src/native/CMakeLists.txt file together with
    CMake presets file (CMakePresets.json.in) is used to build all
    the target configurations
  • The presets file is processed by xaprepare, to create CMakePresets.json
    file which contains all the version-specific bits replacements such as
    Android API levels, paths to utilities etc.
  • One can create a CMakeUserPresets.json file in the same directory to
    provide one's own local modifications to various configs found in the
    CMakePresets.json file. The user presets file must not be checked into
    git.

The subdivision of code previously found in src/monodroid/ is due to desire
to have a single src/native/ subdirectory per each shared library we
create as part of the build process (libmonodroid.so, libxamarin-app.so,
libxamarin-debug-helper.so and possibly others in the future). The shared
libraries, however, share set of sources and those sources have been placed
in src/native/ subdirectories, to produce static archives, subsequently
linked into the shared libraries.

This subdivision allows for nice separation of concerns, but also for easy
sharing of compilation flags, static source analysis (via clang-check), and
creates a flexible set of components which can be combined to produce the final
shared libraries with different sets of features/APIs/etc.

The hierarchy of CMakeLists.txt files is also easier to follow and modify than
it was previously.

grendello and others added 30 commits April 4, 2024 13:27
Changes: https://discourse.llvm.org/t/llvm-18-1-0-released/77448
Changes: https://discourse.llvm.org/t/llvm-18-1-1-released/77540
Changes: https://discourse.llvm.org/t/18-1-2-released/77821
Changes: https://discourse.llvm.org/t/18-1-3-released/78136

Changes interesting for us:

   * AArch64 backend
      * Added support for Cortex-A520, Cortex-A720 and Cortex-X4 CPUs.
      * Assembler/disassembler support has been added for 2023 architecture extensions.
      * Support has been added for Stack Clash Protection. During function frame creation
         and dynamic stack allocations, the compiler will issue memory accesses at regular
         intervals so that a guard area at the top of the stack can’t be skipped over.
   * x86 backend
       * The i128 type now matches GCC and clang’s __int128 type. This mainly benefits external
          projects such as Rust which aim to be binary compatible with C, but also fixes code
          generation where LLVM already assumed that the type matched and called into libgcc
          helper functions.

**Full Changelog**: xamarin/xamarin-android-binutils@L_17.0.6-7.2.1...L_18.1.3-8.0.0
Bumps [external/Java.Interop](https://github.com/xamarin/java.interop) from `651de42` to `e1c7832`.
- [Commits](xamarin/java.interop@651de42...e1c7832)

---
updated-dependencies:
- dependency-name: external/Java.Interop
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Updates the list of submodules that dependabot ignores when checking for
updates.  Repos which are no longer valid have been removed and recently
added xxhash repos have been added to this list.
* main:
  Bump to dotnet/installer@0bfd2dd757 9.0.100-preview.4.24208.2 (#8862)
  [ci] Update dependabot ignore list (#8864)
  Bump external/Java.Interop from `651de42` to `e1c7832` (#8836)
  Bumps LLVM to v18.1.3 and XA utils version to 8.0.0 (#8852)
* main:
  Don't use azureedge.net CDN (#8846)
Neither runtime nor other native libraries are built yet
Some libs build already. monodroid still not built.
* main:
  Bump to dotnet/installer@7380c301c1 9.0.100-preview.4.24215.2 (#8874)
  [Mono.Android] Commit baseline PublicAPI files for API-35 (#8840)
  Add a unit test to check environment processing (#8856)
* main:
  Bump external/xamarin-android-tools from `37d79c9` to `05f9a90` (#8869)
  Bump external/Java.Interop from `e1c7832` to `06214ff` (#8878)
...remove unused p/invoke + associated code (`monodroid_store_package_name`)
...fix endless recursion caused by a typo in `Util::ends_with` overload
...fix p/invoke include generation script
...update p/invoke dispatcher accordingly
To include `libxamarin-native-tracing.so`, one has to request it by
setting the `$(_AndroidEnableNativeStackTracing)` MSBuild property
to `true`
Using `std::source_location` we can now report on, well, source location
without having to resort to C macros.
* main:
  Bump to xamarin/xamarin-android-binutils@758d2e7 (#8885)
  [Mono.Android] Bind API-VanillaIceCream Beta 1 (#8891)
  [AndroidToolTask] Log tool output as error  (#8861)
  [Xamarin.Android.Build.Tasks] Remove "Xamarin" from messages (#8884)
  [Mono.Android] Bind API-VanillaIceCream Developer Preview 2 (#8741)
  Bump to dotnet/installer@22ffa42d6c 9.0.100-preview.4.24221.5 (#8887)
* main:
  Bump to xamarin/monodroid@a5742221b3 (#8893)
  Remove MonoArchive_BaseUri from Configurables.cs (#8890)
* main:
  [docs] Reorganize public Documentation (#8889)
  Bump external/Java.Interop from `06214ff` to `6cfa78c` (#8879)
  Localized file check-in by OneLocBuild Task (#8894)
  $(AndroidPackVersionSuffix)=preview.5; net9 is 34.99.0.preview.5 (#8896)
* main:
  [build] Bump $(AndroidNetPreviousVersion) to 34.0.95 (#8898)
* main:
  Update README (#8913)
  Bumps to xamarin/java.interop@4e893bf (#8924)
  Bump to dotnet/installer@fa261b952d 9.0.100-preview.5.24253.16 (#8921)
  [Mono.Android] Dispose of the `RunnableImplementor` on error (#8907)
  Bump NDK to r26d (#8868)
  Bump to dotnet/installer@d301a122c4 9.0.100-preview.5.24229.2 (#8912)
  Localized file check-in by OneLocBuild Task (#8910)
  LEGO: Merge pull request 8909
  [api-merge] Add `removed-since` info (#8897)
  Bump to xamarin/monodroid@9ca6d9f6 (#8895)
  [Xamarin.Android.Build.Tasks] fix detection of "Android libraries" (#8904)
@grendello
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

* main:
  It's ".NET for Android", not ".NET Android" (#8933)
  Bump to xamarin/java.interop@78d5937 (#8935)
  [docs] Add "Getting Started" docs (#8934)
  [Xamarin.Android.Build.Tests] Fix ActionBarSherlock URL (#8926)
* main:
  [trimming] fix custom applications for `TrimMode=full` (#8936)
@@ -200,7 +200,8 @@ function(xa_common_prepare)
-fstack-protector-strong
-fstrict-return
-fno-strict-aliasing
-ffunction-sections
-fno-function-sections
Copy link
Member

Choose a reason for hiding this comment

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

What does this do? Why add it? From the clang -ffunction-sections docs:

Place each function in its own section

I thus infer that -fno-function-sections doesn't place each function into its own section? I guess that could make the .so smaller?

Likewise the following line, clang -fno-data-sections implies that each data is not placed into its own section.

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory it can make the app/library smaller, but at a (slight) cost of speed of execution (functions might end up far from each other and force long instead of short calls, for instance) and it might impede LTO (link-time optimizations), so I opted for the "traditional" section sharing.

@@ -98,5 +98,5 @@
"Size": 1904
}
},
"PackageSize": 2689557
"PackageSize": 2742805
Copy link
Member

Choose a reason for hiding this comment

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

Increase by 53KB is somewhat odd, especially as I'm not sure what it's from. Most changes appear negligible: libmonosgen-2.0.so is only ~5KB, System.Private.CoreLib.dll is only 4KB.

The change appears to be mostly due to libmonodroid.so, which is up by 122KB!

That may warrant further investigation. Why is libmonodroid.so so much larger now?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's the effect of two changes - the change to use static methods (which you note farther down) may cause the compiler to more eagerly inline code, and the removal of per-function sections may have caused LTO to inline more code.

set(PKG_MINOR "${UNWIND_MINOR_VERSION}")
set(PKG_EXTRA "${UNWIND_EXTRA_VERSION}")
set(PACKAGE_STRING "libunwind-xamarin")
set(PACKAGE_BUGREPORT "")
Copy link
Member

Choose a reason for hiding this comment

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

Why not set PACKAGE_BUGREPORT?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's set to override the bug report address in libunwind, we don't want to have their address in our code base.

Copy link
Member

Choose a reason for hiding this comment

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

Let me rephrase: I'm not wondering why we're setting it at all. I'm wondering why we're setting it to the empty string instead of github.com/xamarin/xamarin.android/issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're setting it to empty string because it sets bug report address for libunwind, not us - so we shouldn't put our address in their codebase (even though nobody's probably going to see it there). Just a matter of keeping stuff tidy, I guess.

@@ -0,0 +1,145 @@
/* libunwind - a platform-independent unwind library
Copy link
Member

Choose a reason for hiding this comment

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

Given that this file is being copied into our repo, shouldn't it go into src-ThirdParty?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because it's not part of the current release of the library - it's a local workaround for an issue, hopefully to be removed at some later date when the issue is fixed by upstream.

Copy link
Member

Choose a reason for hiding this comment

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

What is the upstream issue? Is there a URL?

Copy link
Member Author

Choose a reason for hiding this comment

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

WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */

//#include "unwind_i.h"
#include "../../../../external/libunwind/src/aarch64/unwind_i.h"
Copy link
Member

Choose a reason for hiding this comment

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

Isn't there a way to instead "alter" the include dirs so that the desired unwind_i.h is appropriately included?

That would certainly be cleaner than copying this file to make a one-line change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cleaner, but less explicit. Consider:

$ find -name unwind_i.h
./external/libunwind/src/hppa/unwind_i.h
./external/libunwind/src/x86_64/unwind_i.h
./external/libunwind/src/ia64/unwind_i.h
./external/libunwind/src/s390x/unwind_i.h
./external/libunwind/src/ppc64/unwind_i.h
./external/libunwind/src/aarch64/unwind_i.h
./external/libunwind/src/x86/unwind_i.h
./external/libunwind/src/riscv/unwind_i.h
./external/libunwind/src/mips/unwind_i.h
./external/libunwind/src/sh/unwind_i.h
./external/libunwind/src/arm/unwind_i.h
./external/libunwind/src/ppc32/unwind_i.h
./external/libunwind/src/loongarch64/unwind_i.h

I prefer to use an exact path which, if moved, will cause a clear build error instead of (for instance) including a file for a different architecture.

using namespace xamarin::android;
using namespace xamarin::android::internal;

extern "C" const char *__get_debug_mono_log_property (void)
Copy link
Member

Choose a reason for hiding this comment

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

Pity I didn't notice this in e99b02b, but generally double-underscore __ is reserved for the compiler + toolchain, and should not be used in "user" code (that's us! user code!).

I also don't understand why DEBUG_MONO_LOG_PROPERTY is "special" here in getting a __get_ function + macro, vs. e.g. DEBUG_MONO_ENV_PROPERTY which is a static inline constexpr.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we shouldn't use __ in macros, functions, classes etc, but it's "legacy" so I opted not to touch it. As for the logic behind it? I have no idea, it's another "legacy" thing - I assume that this code was called by something else (either managed or native, likely the former) so I felt it safer to leave it alone. And the macro calling this function is to have one location where the name is fetched, "just in case".

bool found = false;
void *handle = androidSystem.load_dso_from_any_directories (libname.get (), dlopen_flags);
Copy link
Member

Choose a reason for hiding this comment

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

Rationale for turning instance methods into static methods? This feels unrelated to the overall intent of the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I left it in, because it's something I've been wanting to do for ages, but I stopped myself before I replaced all the instance methods with static methods, to keep the diff smaller. However, I didn't want to undo what I've already changed.

The rationale is simple - we don't need instances of those classes. They exist throughout the entire application lifetime, there's just one instance of each class. In our case, classes are merely a way to encapsulate things and present a cleaner interface to the callers of that code, we don't need to instantiate the classes at all. Currently, the instances are stored in global variables and not only it's poor design (I have no idea what I thought when I implemented it this way years ago), it also means that at the library load time, the constructors are called, thus wasting precious cycles for no gain for us.

A follow up PR will be opened at some point which will convert every class we have to use just static methods.

@@ -363,6 +357,15 @@ _monodroid_lookup_replacement_method_info (const char *jniSourceType, const char
return JniRemapping::lookup_replacement_method_info (jniSourceType, jniMethodName, jniMethodSignature);
}

static void
monodroid_log_traces (uint32_t kind, const char *first_line)
Copy link
Member

Choose a reason for hiding this comment

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

There's no mention of monodroid_log_traces in the commit message. What's it for?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no mention because it's not actually used here - this PR which sets up infrastructure for tracing. The PR started this way (thus the branch name), but then I decided that it should only introduce "neutral" changes and prepare infrastructure for follow-up PRs which actually use the infrastructure (like Perfetto implementation)

It's part of code introduced in src/native/monodroid/monodroid-tracing.cc, also not really used by anything in this PR.

@@ -876,7 +879,7 @@ get_link_address (const struct nlmsghdr *message, struct _monodroid_ifaddrs **if
}

if (payload_size > 0) {
size_t alloc_size = ADD_WITH_OVERFLOW_CHECK (size_t, payload_size, room_for_trailing_null);
Copy link
Member

Choose a reason for hiding this comment

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

the move away from the ADD_WITH_OVERFLOW_CHECK() macro to Helpers::add_with_overflow_check<T>() is also an unmentioned change. Why is it part of this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not mentioned because it doesn't change anything in the way this code works, it's functionally equivalent to the macro. The reason for his change is twofold - first, I much prefer functions than preprocessor macros, since they're typesafe. Second, this way I can take advantage of the source_location header and report the code position in a much cleaner, type-safe and better way. We had the macro before only because this was the only way to report on the source:line location (taking advantage of the __FILE__ and __LINE__ preprocessor macros, respectively)


<Touch Files="@(_BuildAndroidAnalyzerRuntimesOutputs)" />
</Target>

<Target Name="_CleanRuntimes"
AfterTargets="Clean">
Copy link
Member

Choose a reason for hiding this comment

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

This original indentation was correct…

@@ -0,0 +1,61 @@
set(LIB_NAME runtime-base)
Copy link
Member

Choose a reason for hiding this comment

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

What is "runtime-base"? There's no mention in the commit message. Is this a .a linked into libmonodroid.so?

Copy link
Member Author

Choose a reason for hiding this comment

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

Every subdirectory of native with CMakeLists.txt produces a libXYZ.a where XYZ is usually the subdirectory name. runtime-base is used by three separate runtime libraries - libmonodroid.so, libxamarin-native-tracing.so and libxamarin-debug-app-helper.so

@@ -0,0 +1,77 @@
set(LIB_NAME xa-shared-bits)
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the runtime-base question, what is xa-shared-bits?

Copy link
Member

Choose a reason for hiding this comment

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

…and is there a good way to know when this is for a static lib vs. a shared lib vs.…

Copy link
Member Author

Choose a reason for hiding this comment

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

Similar to runtime-base, it's a library that's shared by even more components than runtime-base

static constexpr std::array<const char*, 12> log_names = {
"*none*",
"monodroid",
"monodroid-assembly",
Copy link
Member

Choose a reason for hiding this comment

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

The "duplication" between LOG_CATEGORY_NAME_MONODROID_ASSEMBLY and this feels odd, especially when LOG_CATEGORY_NAME_MONODROID_ASSEMBLY isn't used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seeing as this PR was getting bigger and bigger, I didn't implement the changes I want to make to all the log* functions in this file. Treat this as "infrastructure preparation" as well.

@@ -83,6 +98,19 @@ namespace xamarin::android::internal

// Documented in NDK's <android/log.h> comments
static constexpr size_t MAX_LOGCAT_MESSAGE_LENGTH = 1023;

static constexpr std::string_view LOG_CATEGORY_NAME_NONE { "*none*" };
Copy link
Member

Choose a reason for hiding this comment

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

Why do these constexprs exist? see also

Copy link
Member Author

Choose a reason for hiding this comment

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

They will replace the literals in log_functions.cc eventually

* main:
  [build] bump to `$(AndroidNetPreviousVersion)` 34.0.113 (#8943)
@jonpryor
Copy link
Member

jonpryor commented May 15, 2024

Draft commit message:

Context: https://github.com/xamarin/xamarin-android/pull/8800
Context: 8bc7a3e84f95e70fe12790ac31ecd97957771cb2
Context: 68368189d67c46ddbfed4e90e622f635c4aff11e
Context: https://github.com/libunwind/libunwind/issues/702

Begin adding native method call tracing infrastructure based on
[libunwind][0], initially used in tracing the reasons for why
[Blazor][1] apps break when LLVM marshal methods are enabled
(8bc7a3e8, 68368189).  The utility of such infrastructure, however,
is beyond just that particular task; it should be available for
general use by both us and applications.

Method call tracing support is not enabled by default.
To enable it, set the `$(_AndroidEnableNativeStackTracing)` MSBuild
property to `true`.  This will enable the ability to print native, 
managed and Java stack traces by invoking the `monodroid_log_traces`
function from either managed or native application code.

Additionally, change the directory layout for native code.

Previously, `src/monodroid` would produce *five* native libs:

  * `libmono-android.debug.so`
  * `libmono-android.release.so`
  * `libxa-internal-api.so`
  * `libxamarin-app.so`
  * `libxamarin-debug-app-helper.so`

This made for a large `src/monodroid/CMakeLists.txt`, complicating
maintenance.  We had considered trying to move these into separate
`src/LIBRARY-NAME` directories, but it makes things easier with
CMake if we introduce an intermediate directory.

Introduce a new `src/native` directory, to hold native binaries:

  * `src/native/monodroid`: new location for `libmono-android.*.so`

  * `src/native/xamarin-debug-app-helper`: source for
    `libxamarin-debug-app-helper.so`, containing code moved from
    previous `src/monodroid`

  * `src/native/xamarin-app-stub`: source for the "stub"
    `libxamarin-app.so`; the "real" one is produced in the app build.

Some `src/native/*` directories produce libraries (`.so` files). and
some produce *static* archives (`.a` files) to simplify using code
across native libraries.

A [cmake-presets file][2] is processed by `xaprepare`, to create a
`src/native/CMakePresets.json` which contains all the version-specific
bits replacements such as Android API levels, paths to utilities etc.

Developers can create a `src/native/CMakeUserPresets.json` file to
provide local modifications to config options within
`src/native/CMakePresets.json.in`.

Other changes:

  * `src/native/libunwind/fixes/aarch64/Gos-linux.c` is an altered
    copy of [`libunwind/src/aarch64/Gos-linux.c`][3] in order to
    workaround libunwind/libunwind#702.

  * *Begin* turning some instance member functions into static member
    functions, as we don't otherwise need instances of those classes.

  * Begin using `std::source_location` for better crash messages,
    instead of using `__FILE__` and `__LINE__`.

  * Fix endless recursion from a typo in `Util::ends_with()` overload.

[0]: https://github.com/libunwind/libunwind
[1]: https://learn.microsoft.com/aspnet/core/blazor/hybrid/tutorials/maui?view=aspnetcore-8.0
[2]: https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html
[3]: https://github.com/libunwind/libunwind/blob/9cc4d98b22ae57bc1d8c253988feb85d4298a634/src/aarch64/Gos-linux.c

@jonpryor jonpryor merged commit a7b5768 into main May 16, 2024
48 checks passed
@jonpryor jonpryor deleted the dev/grendel/native-tracing branch May 16, 2024 14:08
grendello added a commit that referenced this pull request May 16, 2024
* main:
  [Mono.Android] AndroidMessageHandler should follow HTTP-308 redirects (#8951)
  [Microsoft.Android.Templates] Add icons to templates (#8883)
  [native] Native call tracing infra + native build system overhaul (#8857)
  [build] fix code-flow from dotnet/installer, .NET 9.0.100-preview.5.24262.2 (#8949)
  [ci] Re-enable to push to dotnet9 feed (#8950)
  LEGO: Merge pull request 8952
  [ci] Improve maestro artifact publishing (#8945)
grendello added a commit that referenced this pull request May 16, 2024
* main:
  [Mono.Android] AndroidMessageHandler should follow HTTP-308 redirects (#8951)
  [Microsoft.Android.Templates] Add icons to templates (#8883)
  [native] Native call tracing infra + native build system overhaul (#8857)
  [build] fix code-flow from dotnet/installer, .NET 9.0.100-preview.5.24262.2 (#8949)
  [ci] Re-enable to push to dotnet9 feed (#8950)
  LEGO: Merge pull request 8952
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.

None yet

3 participants