[Native] Add printf-style native logging#12140
Conversation
Introduce printf-style native logging and abort helpers while preserving the existing std::format APIs for MonoVM and CoreCLR. Migrate the NativeAOT-specific formatted call sites to the new primitives. Refs #12139 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 187b207a-083b-461e-9071-e9aab61c9d07
CI failure analysisThe public
The NativeAOT/libunwind collision is additional real-world evidence for #12139. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70f63eb7-6599-414c-a947-d860705aa0fa
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70f63eb7-6599-414c-a947-d860705aa0fa
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70f63eb7-6599-414c-a947-d860705aa0fa
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 Code Review — ⚠️ Minor suggestions (no blockers)
Reviewed the printf-style native logging primitives and the NativeAOT call-site migration. This is clean, well-scoped work.
What I verified
- printf format attributes are correct.
Helpers::abort_applicationfusesformat(printf, 3, 4); since it's a static member (no implicitthis), the 3/4 indices are right. The free functionslog_writef(3,4) andlog_debugf/log_infof/... (2,3) are also indexed correctly. - Category-guard semantics are consistent. The new
(log_categories & category) == 0early-return matches the modern CLRDO_LOG_FMTmacro (!= 0to log), so behavior is unchanged for the migratedlog_debugf/log_infofpaths.log_warnf/errorf/fatalfare unguarded, matching the existinglog_warn/log_error/log_fatal. - Null-format safety.
log_writevandabort_applicationfboth guardnullptrformats with"<null>"/safe_format, andabort_applicationfcorrectly falls back tosafe_formatwhenvasprintfreturns< 0. abort_applicationno longer routes fatal text throughstd::format— good, this preserves full tombstone text and drops the libc++ dependency as intended.- Leak of the
vasprintfbuffer inabort_applicationfis acceptable: it's a[[noreturn]]abort path, so the process dies immediately. - Host-native regression test is a nice addition — it exercises
%s/%p/%dforwarding, priority mapping, and disabled-category suppression against Android log stubs.
Suggestions (both inline, non-blocking)
- 💡 Add an explicit
#include <cstdio>inhelpers.ccforvasprintfrather than relying on a transitive include. - 💡
log_warnf/log_errorf/log_fatalfare currently unused — confirm the follow-up migration PRs consume them so they don't rot.
CI
GitHub commit status is currently pending (checks not yet reported) at 4a27f68. Not treating this as a failure, but the PR isn't mergeable until the Azure DevOps pipeline is green.
Nice, focused first step toward dropping the NativeAOT libc++ dependency. 👍
Generated by Android PR Reviewer for #12140 · 104.3 AIC · ⌖ 13.1 AIC · ⊞ 6.8K
Comment /review to run again
Keep the printf logging PR focused on the runtime implementation instead of introducing new host-native logging test infrastructure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70f63eb7-6599-414c-a947-d860705aa0fa
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70f63eb7-6599-414c-a947-d860705aa0fa
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70f63eb7-6599-414c-a947-d860705aa0fa
Summary
Introduce printf-style native logging primitives and migrate the initial NativeAOT formatted logging call sites to them.
This is the first implementation step from #12139 toward removing the Android NativeAOT dependency on libc++.
Changes
log_writev(),log_writef(),log_debugf(),log_infof(),log_warnf(), andlog_errorf()alongside the existingstd::formatAPIs;Helpers::abort_applicationf()for formatted fatal messages without truncating tombstone text;<cstdio>explicitly forvasprintf();log_fatalf()wrapper; formatted fatal termination usesabort_applicationf()instead;std::format;{}formatting;std::formatinsideHelpers::abort_application().The existing
std::formatAPIs remain available so unrelated call sites can be migrated incrementally. The current stacked migrations are #12148 (shared runtime utilities), #12150 (JNI reference logging), #12153 (timing logging), and #12155 (configuration diagnostics).Runtime behavior
<null>rather than passed to__android_log_vprint().Validation
git diff --check;src/native/native-nativeaot.csproj;src/native/native-clr.csproj;va_list, category filtering, format checking, source-location behavior, and Android log-priority mapping;09f0690da.Part of #12139.