-
Notifications
You must be signed in to change notification settings - Fork 533
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
Conversation
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**: dotnet/android-native-tools@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](dotnet/java-interop@651de42...e1c7832) --- updated-dependencies: - dependency-name: external/Java.Interop dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> 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)
Build is broken atm
* 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)
...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/L_18.1.4-8.0.0@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: [build] Bump $(AndroidNetPreviousVersion) to 34.0.95 (#8898)
* main: Update README (#8913) Bumps to xamarin/Java.Interop/main@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)
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
* main: [trimming] fix custom applications for `TrimMode=full` (#8936)
build-tools/cmake/xa_macros.cmake
Outdated
@@ -200,7 +200,8 @@ function(xa_common_prepare) | |||
-fstack-protector-strong | |||
-fstrict-return | |||
-fno-strict-aliasing | |||
-ffunction-sections | |||
-fno-function-sections |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 "") |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.…
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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*" }; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
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 |
* 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)
* 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
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 arein 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:
src/native/
src/monodroid/
is now in severaldirectories under
src/native/
.src/native/CMakeLists.txt
file together withCMake presets file (
CMakePresets.json.in
) is used to build allthe target configurations
xaprepare
, to createCMakePresets.json
file which contains all the version-specific bits replacements such as
Android API levels, paths to utilities etc.
CMakeUserPresets.json
file in the same directory toprovide one's own local modifications to various configs found in the
CMakePresets.json
file. The user presets file must not be checked intogit.
The subdivision of code previously found in
src/monodroid/
is due to desireto have a single
src/native/
subdirectory per each shared library wecreate as part of the build process (
libmonodroid.so
,libxamarin-app.so
,libxamarin-debug-helper.so
and possibly others in the future). The sharedlibraries, however, share set of sources and those sources have been placed
in
src/native/
subdirectories, to produce static archives, subsequentlylinked 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
), andcreates 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 thanit was previously.