[Native] Migrate configuration diagnostics to printf#12155
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
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
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
Convert host environment and integer parsing diagnostics to #12140 printf helpers while preserving MonoVM formatting branches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70f63eb7-6599-414c-a947-d860705aa0fa
There was a problem hiding this comment.
Pull request overview
Migrates native configuration and integer-parsing diagnostics in the CoreCLR/NativeAOT code paths from std::format-style logging to the printf-checked log_*f() helpers introduced in #12140, reducing formatted-template instantiations while keeping MonoVM behavior unchanged.
Changes:
- Convert
HostEnvironmentdiagnostics inhost-environment.hhtolog_debugf()/log_warnf()with%sformatting andoptional_string()for null safety. - Convert
string_segment::to_integer()diagnostics instrings.hhto runtime-guardedlog_errorf()for CoreCLR/NativeAOT (MonoVM retains existinglog_error()+{}formatting). - Add an explicit
shared/log_types.hhinclude for non-MonoVM builds instrings.hhto avoid reliance on caller include order.
Show a summary per file
| File | Description |
|---|---|
| src/native/common/include/runtime-base/strings.hh | Adds non-MonoVM logging header include and migrates integer-parsing diagnostics to log_errorf() under runtime guards. |
| src/native/clr/include/host/host-environment.hh | Migrates CoreCLR/NativeAOT host environment debug/warn diagnostics to printf-style log_*f() calls. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
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
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 — ✅ LGTM (with 1 minor suggestion)
Clean, well-scoped continuation of the printf-logging migration (#12139). Verified independently against the diff and full file context.
What I checked
%ssafety —optional_string()returnsconst char*and theto_integerparse buffersis explicitly NUL-terminated before logging, so all%sconversions are well-defined.- Format widths —
%zuforsize_t start_index,%dfor theint base, and%lld/%llufor the range bounds all match their argument types. The double casts (int64_t→long long,uint64_t→unsigned long long) correctly bridge the fixed-width types to theprintfpromotion types across ABIs. - Range semantics — signed/unsigned min/max printing preserves the original
std::formatoutput for both signed and unsignedT. - Forward declaration — the manual
log_errorfdeclaration exactly matches the canonical one inshared/log_types.hh(signature +format(printf,2,3)attribute), andLogCategoriesis in scope viahelpers.hh→java-interop-util.h. - Behavior parity — errno handling, conversion logic, and XDG directory diagnostics are unchanged.
Notes
- 💡 One inline suggestion about the duplicated forward declaration (drift risk) — non-blocking.
- CI shows
pending/no checks yet, expected since this PR targets thedev/simonrozsival/nativeaot-printf-loggingbase branch (dependency on #12140). Not mergeable until #12140 lands and CI is green.
Nice reduction in template instantiation with measured object-size wins. 👍
Generated by Android PR Reviewer for #12155 · 77.7 AIC · ⌖ 12.8 AIC · ⊞ 6.8K
Comment /review to run again
## 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 - add `log_writev()`, `log_writef()`, `log_debugf()`, `log_infof()`, `log_warnf()`, and `log_errorf()` alongside the existing `std::format` APIs; - implement the same printf helpers in the CLR and MonoVM shared logging backends so shared runtime code can use one logging path; - add `Helpers::abort_applicationf()` for formatted fatal messages without truncating tombstone text; - include `<cstdio>` explicitly for `vasprintf()`; - omit an unused `log_fatalf()` wrapper; formatted fatal termination uses `abort_applicationf()` instead; - replace the NativeAOT GC-user-peer initialization error's `std::format`; - replace the NativeAOT JNI on-load debug message's `{}` formatting; - avoid `std::format` inside `Helpers::abort_application()`. The existing `std::format` APIs 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 - NativeAOT and CoreCLR use the CLR shared logging implementation. - MonoVM uses its own shared logging implementation with the same printf helper surface. - category filtering and Android log priorities remain unchanged. - null format pointers are logged as `<null>` rather than passed to `__android_log_vprint()`. ## Validation - `git diff --check`; - Release build of `src/native/native-nativeaot.csproj`; - Release build of `src/native/native-clr.csproj`; - NDK Clang C++23 syntax compilation of the CLR helper changes across NativeAOT/CoreCLR configurations; - NDK Clang C++23 syntax compilation of the MonoVM printf implementation in 8 Android configurations; - focused review of `va_list`, category filtering, format checking, source-location behavior, and Android log-priority mapping; - latest head: `09f0690da`. Part of #12139.
## Summary Continue the printf-style logging migration introduced by #12140 in the shared native timing infrastructure. The timing headers are used by MonoVM, CoreCLR, and NativeAOT. All three runtimes now use the same printf-style path for variable timing diagnostics rather than maintaining MonoVM-only `std::format` branches. **Base/dependency:** #12140 must merge first. This PR targets `dev/simonrozsival/nativeaot-printf-logging` so its diff contains only the timing migration. Part of #12139. ## Scope Only two shared timing headers change: - `src/native/common/include/runtime-base/timing.hh` - `src/native/common/include/runtime-base/timing-internal.hh` These files do not overlap #12141, #12142, #12145, #12148, #12150, or #12155. ## Changes ### Managed timing records - replace the owning `std::format` result in `Timing::do_log()` with `log_writef()` for every runtime; - preserve the exact `message; elapsed: seconds:milliseconds::nanoseconds` field order; - pass duration values as explicitly converted `unsigned long long` values matching `%llu`. ### Fast timing diagnostics Migrate variable diagnostics to `log_warnf()` for every runtime: - timing event buffer reallocation sizes; - `CLOCK_MONOTONIC_RAW` errors; - unknown event-kind values; - invalid event-index source method names. Constant warning messages continue using the existing non-formatting `string_view` overload. ## Behavior preserved - timing enablement and category gating are unchanged; - timing sequence acquisition/release is unchanged; - elapsed duration units, values, and output order are unchanged; - event-buffer growth, clock error handling, unknown-event fallback, and index validation are unchanged; - no timing data structures, locks, vectors, strings, or event lifecycle code change. ## Runtime behavior | Runtime | Result | |---|---| | NativeAOT | Uses #12140 printf helpers for variable timing diagnostics | | CoreCLR | Uses #12140 printf helpers for variable timing diagnostics | | MonoVM | Uses the matching MonoVM printf helper implementation from #12140 | ## Measured impact Representative Android arm64 Release objects compiled before the MonoVM path was unified: | Runtime/object | Before | After | Difference | |---|---:|---:|---:| | CoreCLR `internal-pinvokes-clr.cc.o` | 145,472 B | 17,080 B | -128,392 B (-88.26%) | | NativeAOT `host.cc.o` | 176,104 B | 175,960 B | -144 B | CoreCLR's representative object drops from 58 formatting symbols to zero. NativeAOT's representative object still contains formatting symbols from other included functionality, but the timing call sites migrated here no longer instantiate them. ## Non-goals - no new timing tests or logging test framework; - no change to timing data ownership or synchronization; - no changes to the logging helper implementation introduced by #12140; - no attempt to migrate unrelated timing output/file-generation code outside these shared timing headers. ## Validation - `git diff --check`; - NDK Clang C++23 syntax compilation of all 36 Android arm64 Release NativeAOT source variants; - NDK Clang C++23 syntax compilation of all 38 Android arm64 Release CoreCLR source variants; - NDK Clang C++23 syntax compilation of all 31 Android arm64 Release MonoVM source variants; - representative before/after object compilation, size comparison, and symbol inspection; - focused review of chrono values, integer format widths, source-location output, and category semantics; - latest head: `21ff66b26`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f0df1cb-536b-4c53-902e-cbbb9d8e9a15
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f0df1cb-536b-4c53-902e-cbbb9d8e9a15
Resolve the logging declaration conflicts by retaining the lightweight canonical header, and preserve the reviewed NativeAOT status string constants. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f0df1cb-536b-4c53-902e-cbbb9d8e9a15
Include the lightweight declaration header from both implementations and the printf-only abort helper so signatures remain checked without adding the std::format surface. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f0df1cb-536b-4c53-902e-cbbb9d8e9a15
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 Code Review — ⚠️ Minor notes (no blocking issues)
Reviewed the printf-logging migration for native configuration/parsing diagnostics. The core changes are correct and safe.
✅ Verified correct
- Format specifiers match argument types:
%zuforsize_t start_index;%lld/%lluwith explicitstatic_cast<long long>/static_cast<unsigned long long>for the range bounds;%dfor theint base;%sfor C-strings. All call sites are further guarded by the__attribute__((format(printf, ...)))declarations in the newlog_functions.hh, so any mismatch would fail the build. - No null-
%sUB:optional_string()returns"<null>"(nevernullptr) for null inputs, and the parse buffersinstrings.hhis always NUL-terminated before logging, so%sis safe on bionic. - Behavior preserved: integer conversion, range checks, errno handling, XDG directory creation, and debug-category gating are unchanged. Dropping the
svliterals is correct for printf-style formatting. - Header refactor is sound: centralizing the
log_*fdeclarations insrc/native/common/include/shared/log_functions.hhand including it from the per-runtimelog_types.hhremoves duplication cleanly;strings.hhnow only depends on the function declarations it actually uses.
⚠️ Notes (non-blocking)
- PR description is out of date with the diff. The description states "Only two files change" (
host-environment.hh,strings.hh) and references head72cad5d01, but this PR actually touches 9 files at head71cfe50— including the newcommon/include/shared/log_functions.hhand thelog_types.hh/log_functions.cc/helpers.ccrefactors and thebridge-processing.ccconstant dedup. Worth updating the description so reviewers aren't surprised by the broader scope. - See the inline 💡 on
bridge-processing.ccabout scoping theABSENT/PRESENTconstants inside the failure branch.
CI
Checks are still pending (no completed required checks at review time). Since format-string correctness here relies on the compiler's printf attribute checks, the native build must go green before merge — please confirm the NativeAOT/CoreCLR/MonoVM builds pass before merging.
Nice, low-risk migration overall. 👍
Generated by Android PR Reviewer for #12155 · 104.3 AIC · ⌖ 13.1 AIC · ⊞ 6.8K
Comment /review to run again
## Summary Continue the printf-style native logging migration started by #12140. This stacked PR converts formatted logging in NativeAOT-reachable shared runtime utilities to the `log_*f()` and `Helpers::abort_applicationf()` APIs introduced by #12140, avoiding additional `std::format` instantiations in those paths. **Base/dependency:** #12155 must merge first. This PR targets `dev/simonrozsival/nativeaot-config-printf-logging` rather than `main` so its diff remains limited to this runtime utility migration. Part of #12139. ## Scope The change is deliberately limited to four files that are compiled into the Android NativeAOT host: - `src/native/clr/include/runtime-base/util.hh` - `src/native/clr/runtime-base/util.cc` - `src/native/clr/runtime-base/android-system-shared.cc` - `src/native/clr/host/host-shared.cc` These files do not overlap the currently open sibling work: - #12141 — POSIX GC bridge synchronization; - #12142 — owning logging/path state and source-location parsing; - #12145 — indexed temporary-peer storage. ## Changes ### Runtime and environment utilities - migrate file-stat, directory-creation, `fopen`, `chmod`, and `fchmod` diagnostics to printf-style logging; - migrate environment-variable set/create diagnostics while preserving bounded `std::string_view` output through `%.*s`; - preserve null-safe output for raw C-string inputs through `optional_string()`. ### Memory-mapped APK diagnostics - replace the `std::format` allocation in the fatal `mmap()` error path with `Helpers::abort_applicationf()`; - migrate the detailed mmap range/length diagnostic to `%p`, `%zu`, `%d`, and `%.*s` formatting; - migrate the debug-only non-ELF diagnostic without assuming that its `std::string_view` is NUL-terminated. ### Android system properties - migrate the property-buffer size warning using the correct `size_t` format; - migrate invalid `debug.mono.max_grefc` values while preserving the compile-time property-name length; - migrate the effective maximum-gref count using the correct `long` format. ### Shared host diagnostics - migrate Java-class-name lookup failures, including pointer output, to printf-style logging. ## Formatting and behavior preservation - raw pointers use `%p`, are explicitly passed as `void*`, and retain the existing `%-8p` minimum-width alignment; - `size_t` uses `%zu`, `long` uses `%ld`, and descriptors use `%d`; existing mmap lengths retain their `%-12zu` alignment; - non-owning strings use `%.*s` with an explicit length; - debug/info category gating remains inside the #12140 helpers; - warning/error/fatal messages remain unconditional, matching the existing logging behavior; - no logging category, error path, JNI behavior, or control flow is changed. ## Non-goals - This PR does not change the logging implementation introduced in #12140. - It does not touch GC bridge files or the files changed by #12141, #12142, or #12145. - It does not migrate gref/lref line construction in `os-bridge.cc`; that path also writes the formatted text to files and needs a separate design. - It does not attempt to remove all remaining `std::format` use in one change. ## Validation - `git diff --check`; - no overlap with the file sets changed by #12141, #12142, or #12145; - NDK Clang C++23 syntax compilation of all 36 Android arm64 Release NativeAOT compile variants; - NDK Clang C++23 syntax compilation of all 38 Android arm64 Release CoreCLR compile variants; - focused read-only review of printf format types, `std::string_view` lengths, pointer conversions, category semantics, and varargs use. The higher-level native project builds were also attempted, but the local Gradle 9.4.1 distribution failed while starting the unrelated `manifestmerger` build, before native compilation. The direct checks above use the repository-generated production NDK compile commands and generated headers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
Summary
Continue the printf-style logging migration introduced by #12140 for native configuration and parsing diagnostics.
This PR converts CoreCLR/NativeAOT host-environment messages and the shared MonoVM/CoreCLR/NativeAOT integer-parsing messages to the printf helpers from #12140. Shared code now has one logging path instead of runtime-specific format branches.
Base/dependency: #12140 must merge first. This PR targets
dev/simonrozsival/nativeaot-printf-loggingso its diff contains only this migration.Part of #12139.
Scope
Only two files change:
src/native/clr/include/host/host-environment.hhsrc/native/common/include/runtime-base/strings.hhThese files do not overlap #12141, #12142, #12145, #12148, #12150, or #12153.
Changes
Host environment diagnostics
host-environment.hhis shared by CoreCLR and NativeAOT, but not MonoVM.%sformatting;%sformatting;optional_string()handling for null C-string inputs.Integer parsing diagnostics
strings.hhis shared by all three runtimes. Its variable diagnostics now uselog_errorf()without runtime-specific branches:%zuforsize_t;%lld/%lluvalues;%dbase;The header forward-declares only
log_errorf()instead of including the heavyweight formatting declarations fromshared/log_types.hh.Behavior preserved
%slogging;Runtime behavior
log_errorf()implementation for shared parsing diagnosticsMeasured impact
Representative Android arm64 Release objects compiled before the MonoVM path was unified:
host-environment.cc.otiming-internal.cc.oThese call sites did not contain explicit
std::formatexpressions, so the impact is smaller than #12150/#12153; the change prevents their formatted logging templates from being instantiated.Non-goals
Validation
git diff --check;72cad5d01.