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] Let Android know what is our reason to abort() #9314

Merged
merged 10 commits into from
Oct 14, 2024

Conversation

grendello
Copy link
Contributor

@grendello grendello commented Sep 19, 2024

Android C API includes a function which allows the caller to set a
text message indicating the reason why the application is aborting
its execution using the abort(3) function, since the call doesn't
accept any parameters. The message is then used in the native stack
trace (as well as in the application tombstone), which may be helpful
for us when reading crash reports which contain just the stack trace
and no context information.

Given the following code:

 Helpers::abort_application (LOG_ASSEMBLY, "Testing abort stuff");

We will see the following logged in logcat:

 F monodroid-assembly: Testing abort stuff
 F monodroid-assembly: Abort at monodroid-glue.cc:825:2 ('void xamarin::android::internal::MonodroidRuntime::init_android_runtime(JNIEnv *, jclass, jobject)')

 F libc    : Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 8803 (XAPerfTest.net9), pid 8803 (XAPerfTest.net9)
 I crash_dump64: obtaining output fd from tombstoned, type: kDebuggerdTombstoneProto
 I tombstoned: received crash request for pid 8803
 I crash_dump64: performing dump of process 8803 (target tid = 8803)
 F DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
 F DEBUG   : Build fingerprint: 'google/raven/raven:14/AP2A.240805.005.F1/12043167:user/release-keys'
 F DEBUG   : Revision: 'MP1.0'
 F DEBUG   : ABI: 'arm64'
 F DEBUG   : Timestamp: 2024-09-24 12:00:57.245408791+0200
 F DEBUG   : Process uptime: 1s
 F DEBUG   : Cmdline: com.xamarin.XAPerfTest.net9
 F DEBUG   : pid: 8803, tid: 8803, name: XAPerfTest.net9  >>> com.xamarin.XAPerfTest.net9 <<<
 F DEBUG   : uid: 10456
 F DEBUG   : tagged_addr_ctrl: 0000000000000001 (PR_TAGGED_ADDR_ENABLE)
 F DEBUG   : signal 6 (SIGABRT), code -1 (SI_QUEUE), fault addr --------
 F DEBUG 👉: Abort message: 'Testing abort stuff'
 F DEBUG   :     x0  0000000000000000  x1  0000000000002263  x2  0000000000000006  x3  0000007fc7ed5530
 F DEBUG   :     x4  3139343137396262  x5  3139343137396262  x6  3139343137396262  x7  7f7f7f7f7f7f7f7f
 F DEBUG   :     x8  00000000000000f0  x9  000000721c528350  x10 0000000000000001  x11 000000721c579170
 F DEBUG   :     x12 0000007fc7ed3e50  x13 0000000000000088  x14 0000007fc7ed5068  x15 000000057ae2d47e
 F DEBUG   :     x16 000000721c5dffd0  x17 000000721c5cb560  x18 0000007230490000  x19 0000000000002263
 F DEBUG   :     x20 0000000000002263  x21 00000000ffffffff  x22 0000006f69e1d7d6  x23 0000007fc7ed58a4
 F DEBUG   :     x24 0000007fc7ed58c8  x25 0000000000000000  x26 0000000000000000  x27 b400007218a0a060
 F DEBUG   :     x28 0000000000000000  x29 0000007fc7ed55b0
 F DEBUG   :     lr  000000721c5628b8  sp  0000007fc7ed5510  pc  000000721c5628e4  pst 0000000000001000

(the rest of stack trace is omitted for brevity)

We log the indicated message before calling abort(), but it also
appears on the Abort message: line in the stack trace.

Additionally, don't log the full path to the source file containing the
abort call, a file name is just enough.

@grendello grendello force-pushed the dev/grendel/set-abort-message branch from 7adac89 to 138a195 Compare September 24, 2024 10:10
@grendello grendello changed the title WIP: use android_set_abort_message [native] Let Android know what is our reason to abort() Sep 24, 2024
@grendello grendello marked this pull request as ready for review September 24, 2024 10:12
@grendello grendello requested a review from jonpryor as a code owner September 24, 2024 10:12
log_fatal (category, message);

// ...let android include it in the tombstone, debuggerd output, stack trace etc
android_set_abort_message (message);
Copy link
Member

Choose a reason for hiding this comment

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

Do we not want sloc in this message either?

It feels like what we might want is C#-esque ${message}{(log_location ? $"\nabort at {sloc}" : "")}, which would allow the location to be part of the abort message.

Question: can the message contain a newline? Does that work and what does the logcat output look like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alas, it appears the maximum length of the message is 120 characters, so we can't really stuff the location info in there :(

@grendello grendello force-pushed the dev/grendel/set-abort-message branch 3 times, most recently from f79b485 to 60ec530 Compare October 1, 2024 10:08
log_fatal (LOG_DEFAULT, "Please either set android:minSdkVersion >= 10 or use a build without the shared runtime (like default Release configuration).");
Helpers::abort_application ();
Helpers::abort_application (
"Do you have a shared runtime build of your app with AndroidManifest.xml android:minSdkVersion < 10 while running on a 64-bit Android 5.0 target? This combination is not supported. "
Copy link
Member

Choose a reason for hiding this comment

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

If the maximum assertion message length is 120 characters, doesn't this message -- at 305 characters -- vastly exceed it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, but the message is logged separately, too, so we'll get the full message in logcat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This particular message might no longer be needed, since we don't support API10 anymore.

Copy link
Member

@jonpryor jonpryor left a comment

Choose a reason for hiding this comment

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

Because there is a maximum assert message length of 120 characters, every call to Helpers::abort_application() must be reviewed to ensure that the most important parts of the message come first. This may involve leaving the preceding log_fatal() calls, as their message length is much longer, and having a "most important parts" message within the assert message.

@grendello
Copy link
Contributor Author

Because there is a maximum assert message length of 120 characters, every call to Helpers::abort_application() must be reviewed to ensure that the most important parts of the message come first. This may involve leaving the preceding log_fatal() calls, as their message length is much longer, and having a "most important parts" message within the assert message.

I agree that the most important info must come first, but there's no reason to leave the preceding log_fatal calls since the message is logged in full in Helpers::abort_application

@grendello grendello force-pushed the dev/grendel/set-abort-message branch from 60ec530 to 44ef5f7 Compare October 2, 2024 18:44
grendello and others added 10 commits October 14, 2024 10:59
Android C API includes a function which allows the caller to set a
text message indicating the reason why the application is aborting
its execution using the `abort(3)` function, since the call doesn't
accept any parameters.  The message is then used in the native stack
trace (as well as in the application tombstone), which may be helpful
for us when reading crash reports which contain just the stack trace
and no context information.

Given the following code:

   Helpers::abort_application (LOG_ASSEMBLY, "Testing abort stuff");

We will see the following logged in logcat:

     F monodroid-assembly: Testing abort stuff
     F monodroid-assembly: Abort at monodroid-glue.cc:825:2 ('void xamarin::android::internal::MonodroidRuntime::init_android_runtime(JNIEnv *, jclass, jobject)')

     F libc    : Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 8803 (XAPerfTest.net9), pid 8803 (XAPerfTest.net9)
     I crash_dump64: obtaining output fd from tombstoned, type: kDebuggerdTombstoneProto
     I tombstoned: received crash request for pid 8803
     I crash_dump64: performing dump of process 8803 (target tid = 8803)
     F DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
     F DEBUG   : Build fingerprint: 'google/raven/raven:14/AP2A.240805.005.F1/12043167:user/release-keys'
     F DEBUG   : Revision: 'MP1.0'
     F DEBUG   : ABI: 'arm64'
     F DEBUG   : Timestamp: 2024-09-24 12:00:57.245408791+0200
     F DEBUG   : Process uptime: 1s
     F DEBUG   : Cmdline: com.xamarin.XAPerfTest.net9
     F DEBUG   : pid: 8803, tid: 8803, name: XAPerfTest.net9  >>> com.xamarin.XAPerfTest.net9 <<<
     F DEBUG   : uid: 10456
     F DEBUG   : tagged_addr_ctrl: 0000000000000001 (PR_TAGGED_ADDR_ENABLE)
     F DEBUG   : signal 6 (SIGABRT), code -1 (SI_QUEUE), fault addr --------
     F DEBUG 👉: Abort message: 'Testing abort stuff'
     F DEBUG   :     x0  0000000000000000  x1  0000000000002263  x2  0000000000000006  x3  0000007fc7ed5530
     F DEBUG   :     x4  3139343137396262  x5  3139343137396262  x6  3139343137396262  x7  7f7f7f7f7f7f7f7f
     F DEBUG   :     x8  00000000000000f0  x9  000000721c528350  x10 0000000000000001  x11 000000721c579170
     F DEBUG   :     x12 0000007fc7ed3e50  x13 0000000000000088  x14 0000007fc7ed5068  x15 000000057ae2d47e
     F DEBUG   :     x16 000000721c5dffd0  x17 000000721c5cb560  x18 0000007230490000  x19 0000000000002263
     F DEBUG   :     x20 0000000000002263  x21 00000000ffffffff  x22 0000006f69e1d7d6  x23 0000007fc7ed58a4
     F DEBUG   :     x24 0000007fc7ed58c8  x25 0000000000000000  x26 0000000000000000  x27 b400007218a0a060
     F DEBUG   :     x28 0000000000000000  x29 0000007fc7ed55b0
     F DEBUG   :     lr  000000721c5628b8  sp  0000007fc7ed5510  pc  000000721c5628e4  pst 0000000000001000

(the rest of stack trace is omitted for brevity)

We log the indicated message before calling `abort()`, but it also
appears on the `Abort message:` line in the stack trace.

Additionally, don't log the full path to the source file containing the
abort call, a file name is just enough.
As abort messages are limited to ~120 chars, move full paths to end of message.
Format string *had* 4 args, but only 3 were provided!

Update format string so it only reads 3 args.
Shorten message so that it is likely to fit within 120 chars.
@grendello grendello force-pushed the dev/grendel/set-abort-message branch from 59bb5aa to e35c7c9 Compare October 14, 2024 09:17
@grendello grendello requested a review from jonpryor October 14, 2024 15:07
@jonpryor jonpryor merged commit 594d090 into main Oct 14, 2024
58 checks passed
@jonpryor jonpryor deleted the dev/grendel/set-abort-message branch October 14, 2024 21:27
@github-actions github-actions bot locked and limited conversation to collaborators Nov 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants