Replace all `exit()` calls with `abort()` in native code #7734

grendello · 2023-01-24T18:18:10Z

exit() causes the application to restart, which can be both annoying and
confusing when the app seemingly shows something (usually a white screen)
and yet it doesn't work. The only way to learn what's going on is to look
at the logcat output to see the error message and a restart.

abort(), however, fully terminates the application and also shows a native
stack trace, which makes it easy to find the cause of the "crash" (the error
message will be logged typically just before the native stack trace is printed)

`exit()` causes the application to restart, which can be both annoying and confusing when the app seemingly shows something (usually a white screen) and yet it doesn't work. The only way to learn what's going on is to look at logcat output to see the error message and a restart. `abort()`, however, fully terminates the application and also shows a native stack trace, which makes it easy to find the cause of the "crash" (the error message will be logged typically just before the native stack trace is printed)

jonathanpeppers · 2023-01-24T19:16:04Z

src/monodroid/jni/debug-app-helper.cc

@@ -91,7 +91,7 @@ Java_mono_android_DebugRuntime_init (JNIEnv *env, [[maybe_unused]] jclass klass,
 	void *monosgen = dlopen (monosgen_path, RTLD_LAZY | RTLD_GLOBAL);
 	if (monosgen == nullptr) {
 		log_fatal (LOG_DEFAULT, "Failed to dlopen Mono runtime from %s: %s", monosgen_path, dlerror ());
-		exit (FATAL_EXIT_CANNOT_FIND_LIBMONOSGEN);


Are all of these FATAL_EXIT_* constants somewhere, where we could delete those, too?

They're in JI, and one or two are used there and since JI runs on desktop, the use of exit() is fine I guess. @jonpryor?

the FATAL_EXIT_* constants are in xamarin/java.interop, some of which are used there.

grendello · 2023-01-25T14:45:30Z

I'm investigating the size increase of libmonodroid.so

* main: Bump to xamarin/Java.Interop/main@1366d99 (dotnet#7718)

This avoids the 16% size increase of `libmonodroid.so` For some reason, calling `abort ()` caused the compiler to inline much more code and in a weird manner (it repeated essentially identical blocks of code from an inlined function, which were previously, when `exit` was used, folded into a single block)

jonpryor · 2023-01-25T22:03:13Z

src/monodroid/jni/debug.cc

@@ -474,7 +474,7 @@ Debug::process_cmd (int fd, char *cmd)
 		log_info (LOG_DEFAULT, "Debugger requested an exit, will exit immediately.\n");
 		fflush (stdout);
 		fflush (stderr);
-		exit (0);
+		Helpers::abort_application ();


This gives me pause; the original exit(0) is not signaling an error condition, so is calling abort_application() appropriate?

As this is part of the debugging infrastructure, I think it may be safer to keep this as exit(0), unless you want to do an end-to-end test to exercise this behavior and see what happens…

The problem here is that the intention is to terminate the application
but exit doesn't do that while abort does.

From the pov of the debugger the application dies anyway
but from the pov of the developer, it will immediately restart itself
which is rather weird

jonpryor · 2023-01-25T22:06:59Z

Context: https://discord.com/channels/732297728826277939/732297837953679412/1067806732983746621
Context: https://discord.com/channels/732297728826277939/732297837953679412/1067822882274689024

calls to abort() are replaced with Helpers::abort_application() because the abort() calls were contributing to a ~16% increase in the size of libmonodroid.so. Consolidating the abort() calls into abort_application() removed the size increase.

jonpryor · 2023-01-25T22:58:54Z

Context: https://discord.com/channels/732297728826277939/732297837953679412/1067806732983746621
Context: https://discord.com/channels/732297728826277939/732297837953679412/1067822882274689024

In certain "bad app" scenarios, we would print a message and then
**exit**(3) the app.  The problem with using `exit()` in this manner
is that it *can* result in the app constantly relaunching, "spamming"
the UI.

Improve this by calling **abort**(3) instead.  This prevents Android
from constantly re-launching the app, and also prints useful abort
information such as registers and a native stack trace to `adb logcat`.

Unexpectedly, however, "simply" replacing app calls to `exit()` with
calls to `abort()` resulted in `libmonodroid.so` increasing in size
by ~16%.  For some reason, calling `abort()` caused the compiler to
inline much more code and in a weird manner (it repeated essentially
identical blocks of code from an inlined function, which were
previously folded into a single block when `exit()` was used).

We found that consolidating the `abort()` calls into
`Helpers::abort_application()` removed the size increase.

* main: [monodroid] Replace `exit()` with `abort()` in native code (dotnet#7734) Bump to xamarin/Java.Interop/main@8a1ae57 (dotnet#7738) [build] bump `$(AndroidNet7Version)` (dotnet#7737) Bump to xamarin/Java.Interop/main@1366d99 (dotnet#7718) [Xamarin.Android.Build.Tasks] fix AndroidGenerateResourceDesigner (dotnet#7721) Bump to xamarin/monodroid@50faac94 (dotnet#7725)

* main: (32 commits) [monodroid] Replace `exit()` with `abort()` in native code (dotnet#7734) Bump to xamarin/Java.Interop/main@8a1ae57 (dotnet#7738) [build] bump `$(AndroidNet7Version)` (dotnet#7737) Bump to xamarin/Java.Interop/main@1366d99 (dotnet#7718) [Xamarin.Android.Build.Tasks] fix AndroidGenerateResourceDesigner (dotnet#7721) Bump to xamarin/monodroid@50faac94 (dotnet#7725) Revert "[Xamarin.Android.Build.Tasks] fix cases of missing `@(Reference)` (dotnet#7642)" (dotnet#7726) [marshal methods] Properly support arrays of arrays (dotnet#7707) Bump to dotnet/installer@9962c6a 8.0.100-alpha.1.23063.11 (dotnet#7677) [Actions] Add action to bump the hash used for the unified pipeline (dotnet#7712) Bump to xamarin/xamarin-android-tools/main@099fd95 (dotnet#7709) [ci] Move build stages into yaml templates (dotnet#7553) [Xamarin.Android.Build.Tasks] fix NRE in `<GenerateResourceCaseMap/>` (dotnet#7716) [ci] Pass token when building Designer tests (dotnet#7715) [Mono.Android] Android.Telecom.InCallService.SetAudioRoute() + enum (dotnet#7711) [Mono.Android] Fix some incorrect enums. (dotnet#7670) [Xamarin.Android.Build.Tasks] _Microsoft.Android.Resource.Designer namespace (dotnet#7681) LEGO: Merge pull request 7713 [Xamarin.Android.Build.Tasks] lazily populate Resource lookup (dotnet#7686) [Xamarin.Android.Build.Tasks] skip XA1034 logic in some cases (dotnet#7680) ...

Context: https://discord.com/channels/732297728826277939/732297837953679412/1067806732983746621 Context: https://discord.com/channels/732297728826277939/732297837953679412/1067822882274689024 In certain "bad app" scenarios, we would print a message and then **exit**(3) the app. The problem with using `exit()` in this manner is that it *can* result in the app constantly relaunching, "spamming" the UI. Improve this by calling **abort**(3) instead. This prevents Android from constantly re-launching the app, and also prints useful abort information such as registers and a native stack trace to `adb logcat`. Unexpectedly, however, "simply" replacing app calls to `exit()` with calls to `abort()` resulted in `libmonodroid.so` increasing in size by ~16%. For some reason, calling `abort()` caused the compiler to inline much more code and in a weird manner (it repeated essentially identical blocks of code from an inlined function, which were previously folded into a single block when `exit()` was used). We found that consolidating the `abort()` calls into `Helpers::abort_application()` removed the size increase.

* main: [Xamarin.Android.Build.Tasks] Fix issue where app will not install (dotnet#7719) Bump to dotnet/installer@779a644 8.0.100-alpha.1.23070.23 (dotnet#7728) LEGO: Merge pull request 7751 [Mono.Android] Wrap connection exceptions in HttpRequestException (dotnet#7661) [Mono.Android] Fix View.SystemUiVisibility enumification (dotnet#7730) Bump r8 from 3.3.75 to 4.0.48 (dotnet#7700) [monodroid] Prevent overlapped decompression of embedded assemblies (dotnet#7732) [xaprepare] Support arm64 emulator components (dotnet#7743) Bump SQLite to 3.40.1 (dotnet#7733) Bump to xamarin/xamarin-android-binutils/L_15.0.7-5.0.3@6721af4b (dotnet#7742) [monodroid] Replace `exit()` with `abort()` in native code (dotnet#7734) Bump to xamarin/Java.Interop/main@8a1ae57 (dotnet#7738) [build] bump `$(AndroidNet7Version)` (dotnet#7737) Bump to xamarin/Java.Interop/main@1366d99 (dotnet#7718) [Xamarin.Android.Build.Tasks] fix AndroidGenerateResourceDesigner (dotnet#7721) Bump to xamarin/monodroid@50faac94 (dotnet#7725)

* main: (112 commits) [ci] Remove classic Mono.Android-Tests runs (dotnet#7778) $(AndroidPackVersionSuffix)=preview.2; net8 is 34.0.0-preview.2 (dotnet#7761) Localized file check-in by OneLocBuild Task (dotnet#7758) Bump to dotnet/installer@dec1209 8.0.100-alpha.1.23080.11 (dotnet#7755) LEGO: Merge pull request 7756 Localized file check-in by OneLocBuild (dotnet#7752) [Xamarin.Android.Build.Tasks] Fix issue where app will not install (dotnet#7719) Bump to dotnet/installer@779a644 8.0.100-alpha.1.23070.23 (dotnet#7728) LEGO: Merge pull request 7751 [Mono.Android] Wrap connection exceptions in HttpRequestException (dotnet#7661) [Mono.Android] Fix View.SystemUiVisibility enumification (dotnet#7730) Bump r8 from 3.3.75 to 4.0.48 (dotnet#7700) [monodroid] Prevent overlapped decompression of embedded assemblies (dotnet#7732) [xaprepare] Support arm64 emulator components (dotnet#7743) Bump SQLite to 3.40.1 (dotnet#7733) Bump to xamarin/xamarin-android-binutils/L_15.0.7-5.0.3@6721af4b (dotnet#7742) [monodroid] Replace `exit()` with `abort()` in native code (dotnet#7734) Bump to xamarin/Java.Interop/main@8a1ae57 (dotnet#7738) [build] bump `$(AndroidNet7Version)` (dotnet#7737) Bump to xamarin/Java.Interop/main@1366d99 (dotnet#7718) ...

grendello requested a review from jonpryor as a code owner January 24, 2023 18:18

grendello mentioned this pull request Jan 24, 2023

Prevent overlapped decompression of embedded assemblies #7732

Merged

jonathanpeppers reviewed Jan 24, 2023

View reviewed changes

grendello added 2 commits January 25, 2023 15:53

Merge branch 'main' into replace-exit-with-abort

edc6718

* main: Bump to xamarin/Java.Interop/main@1366d99 (dotnet#7718)

jonpryor reviewed Jan 25, 2023

View reviewed changes

Restore `exit(0)'

d40fadd

jonpryor merged commit c990148 into dotnet:main Jan 25, 2023

grendello deleted the replace-exit-with-abort branch January 25, 2023 23:04

github-actions bot locked and limited conversation to collaborators Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace all `exit()` calls with `abort()` in native code #7734

Replace all `exit()` calls with `abort()` in native code #7734

grendello commented Jan 24, 2023

jonathanpeppers Jan 24, 2023

grendello Jan 24, 2023

jonpryor Jan 24, 2023

grendello commented Jan 25, 2023

jonpryor Jan 25, 2023

grendello Jan 25, 2023

jonpryor commented Jan 25, 2023 •

edited

Loading

jonpryor commented Jan 25, 2023

Replace all exit() calls with abort() in native code #7734

Replace all exit() calls with abort() in native code #7734

Conversation

grendello commented Jan 24, 2023

jonathanpeppers Jan 24, 2023

Choose a reason for hiding this comment

grendello Jan 24, 2023

Choose a reason for hiding this comment

jonpryor Jan 24, 2023

Choose a reason for hiding this comment

grendello commented Jan 25, 2023

jonpryor Jan 25, 2023

Choose a reason for hiding this comment

grendello Jan 25, 2023

Choose a reason for hiding this comment

jonpryor commented Jan 25, 2023 • edited Loading

jonpryor commented Jan 25, 2023

Replace all `exit()` calls with `abort()` in native code #7734

Replace all `exit()` calls with `abort()` in native code #7734

jonpryor commented Jan 25, 2023 •

edited

Loading