Symbolicate JIT frames in Android native crash handler - #231
Conversation
…ash handler Agent-Logs-Url: https://github.com/winnerspiros/osu/sessions/2246a408-aec9-460c-9b2b-ecb666b6fb37 Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com>
Agent-Logs-Url: https://github.com/winnerspiros/osu/sessions/2246a408-aec9-460c-9b2b-ecb666b6fb37 Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves Android native crash diagnostics by adding fallback symbolication for Mono JIT/trampoline frames when dladdr() can’t resolve symbols, aiming to make native_crash.log actionable for managed-code and trampoline-related crashes.
Changes:
- Add perfmap (
MONO_ENV_OPTIONS=--jitmap) and/proc/self/maps-based fallback symbolication paths incrash_handler.cppwhendladdr()fails. - Add
mono.envand wire it into the Android project viaAndroidEnvironmentto enable--jitmapand redirectTMPDIRfor perfmap placement. - Adjust frame writing/unwind output to use the new fallback resolvers before printing
<unresolved>.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| osu.Android/osu.Android.csproj | Includes mono.env via AndroidEnvironment so Mono env vars are packaged into the APK. |
| osu.Android/mono.env | Sets MONO_ENV_OPTIONS=--jitmap and redirects TMPDIR to place Mono perfmaps alongside crash logs. |
| osu.Android/Native/crash_handler.cpp | Implements perfmap and /proc/self/maps fallback symbolication and integrates it into backtrace output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| size_t n = g_perfmapSize; | ||
| size_t i = 0; | ||
| while (i < n) { | ||
| // Each line: "<hex_start> <hex_size> <name>\n" | ||
| size_t lineStart = i; | ||
| size_t p = i; |
There was a problem hiding this comment.
resolveViaPerfmap() does a full linear scan of the (up to) 64MB perfmap for every unresolved frame. In the worst case (many JIT frames) this can turn crash handling into a multi-hundred-ms / multi-second operation, increasing the chance the process is killed before the log is fully written. Consider reducing repeated work per crash (e.g. stop early if the perfmap is sorted by address, or add a small fixed-size cache of the last N lookups).
| outBuf[bp < (int)sizeof(outBuf) - 1 ? bp++ : bp] = ']'; | ||
| if (path[0] != '\0') { | ||
| appendLit(outBuf, sizeof(outBuf), &bp, " "); | ||
| for (int k = 0; path[k] && bp < (int)sizeof(outBuf) - 1; ++k) | ||
| outBuf[bp++] = path[k]; | ||
| } else if (perms[2] == 'x') { | ||
| // Anonymous executable mapping: classic Mono JIT/trampoline region. | ||
| appendLit(outBuf, sizeof(outBuf), &bp, | ||
| " [Mono JIT/trampoline (anon rwxp)]"); | ||
| } else { | ||
| appendLit(outBuf, sizeof(outBuf), &bp, " [anon]"); | ||
| } | ||
| outBuf[bp] = '\0'; |
There was a problem hiding this comment.
The outBuf[...] = ']' write is hard to reason about and can drop the closing bracket when the buffer is full (it writes ] at bp but then outBuf[bp] = '\0' overwrites it when bp didn't increment). Consider simplifying this to an explicit bounds check that always preserves either the bracket+NUL or just a NUL, to keep formatting predictable.
| # TMPDIR=/storage/emulated/0/Android/data/sh.ppy.osulazer/files | ||
| # Redirects Mono's perfmap output into the same external-files directory | ||
| # that we already write native_crash.log to. This is the *only* path that | ||
| # is (a) writable by the app, (b) readable post-mortem by the user via | ||
| # the Files app on an unrooted device, and (c) survives app uninstall on | ||
| # most devices' "external files" semantics. | ||
| MONO_ENV_OPTIONS=--jitmap | ||
| TMPDIR=/storage/emulated/0/Android/data/sh.ppy.osulazer/files |
There was a problem hiding this comment.
TMPDIR is hardcoded to /storage/emulated/0/Android/data/sh.ppy.osulazer/files. This will break perfmap output if the applicationId/package name changes (debug variants, forks, or future renames), and may be incorrect on devices where the external storage path differs. Consider generating this value from the actual package name at build time (MSBuild property) or avoiding the hardcoded TMPDIR by having the native side search dirname(g_logPath) only (since it already points at the external-files dir).
| # TMPDIR=/storage/emulated/0/Android/data/sh.ppy.osulazer/files | |
| # Redirects Mono's perfmap output into the same external-files directory | |
| # that we already write native_crash.log to. This is the *only* path that | |
| # is (a) writable by the app, (b) readable post-mortem by the user via | |
| # the Files app on an unrooted device, and (c) survives app uninstall on | |
| # most devices' "external files" semantics. | |
| MONO_ENV_OPTIONS=--jitmap | |
| TMPDIR=/storage/emulated/0/Android/data/sh.ppy.osulazer/files | |
| # TMPDIR is intentionally not set here. Hardcoding the external-files path | |
| # would bake in a specific package name and storage root, which breaks for | |
| # debug variants, forks, future renames, or devices where external storage | |
| # is mounted elsewhere. Perfmap discovery should instead rely on the native | |
| # crash handler resolving it relative to dirname(g_logPath). | |
| MONO_ENV_OPTIONS=--jitmap |
| // Candidate directories, in priority order. The dir containing g_logPath | ||
| // is checked first so a build that sets `TMPDIR=<external-files-dir>` | ||
| // (the recommended config) finds its perfmap immediately. | ||
| const char* tmpEnv = getenv("TMPDIR"); | ||
| char logDir[kMaxLogPathLen] = {}; | ||
| if (g_logPath[0] != '\0') { |
There was a problem hiding this comment.
ensurePerfmapLoaded() calls getenv("TMPDIR") inside the crash signal handler path. getenv() is not async-signal-safe (can take libc locks / touch malloc state), which risks deadlock or re-entrancy crashes while handling SIGSEGV. Consider removing the getenv() lookup from the handler (e.g. precompute/copy TMPDIR at install time, or only search dirname(g_logPath), /tmp, /data/local/tmp). Also update the async-signal-safety comment block to match the actual syscalls used (mmap/fstat/etc.).
| size_t nameStart = p; | ||
| while (p < n && d[p] != '\n') ++p; | ||
| size_t nameLen = p - nameStart; | ||
| if (size != 0 && pc >= start && pc < start + size) { |
There was a problem hiding this comment.
In resolveViaPerfmap(), the range check pc < start + size can overflow uint64_t if start is large, which would incorrectly treat unrelated PCs as within-range. Safer pattern is to compare as pc >= start && (pc - start) < size (after ensuring pc >= start).
| if (size != 0 && pc >= start && pc < start + size) { | |
| if (size != 0 && pc >= start && (static_cast<uint64_t>(pc) - start) < size) { |
The uploaded
build_test/native_crash.logshows a realSIGSEGV pc=0(null function-pointer call) onSDLThread, but all 32 backtrace frames sit in Mono JIT/trampolinerwxpmappings wheredladdrcannot resolve symbols — every frame prints<unresolved>, so the crash cannot be diagnosed.Changes
osu.Android/Native/crash_handler.cpp— two new fallbacks tried in order whendladdrfails:resolveViaPerfmap()lazily mmaps a Mono--jitmapfile (searched in<dirname(g_logPath)>,$TMPDIR,/tmp,/data/local/tmp) and writes[JIT] <managed_method>+0xOFF.resolveViaProcMaps()scans/proc/self/mapsand writes[perms start-end +offset] <path>, tagging anonymous executable mappings as[Mono JIT/trampoline (anon rwxp)]. Always works, no setup required.writeFrame()andunwindCallback(). All new code is async-signal-safe (open/read/mmap/write only; fixed stack buffers; no malloc/stdio).osu.Android/mono.env+<AndroidEnvironment>reference inosu.Android.csproj— setsMONO_ENV_OPTIONS=--jitmapand redirectsTMPDIRto the app's external-files dir so the perfmap lands next tonative_crash.log(reachable via Files app on unrooted devices).Result
A frame that previously printed:
will now print one of:
depending on whether the perfmap is present.
Notes
/proc/self/mapsline parsers caught and corrected an off-by-one in the maps field-skip count (was eating the path).build_test/left untouched per instruction.