Skip to content

[Native] Migrate configuration diagnostics to printf#12155

Open
simonrozsival wants to merge 4 commits into
dev/simonrozsival/nativeaot-printf-loggingfrom
dev/simonrozsival/nativeaot-config-printf-logging
Open

[Native] Migrate configuration diagnostics to printf#12155
simonrozsival wants to merge 4 commits into
dev/simonrozsival/nativeaot-printf-loggingfrom
dev/simonrozsival/nativeaot-config-printf-logging

Conversation

@simonrozsival

@simonrozsival simonrozsival commented Jul 17, 2026

Copy link
Copy Markdown
Member

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-logging so its diff contains only this migration.

Part of #12139.

Scope

Only two files change:

  • src/native/clr/include/host/host-environment.hh
  • src/native/common/include/runtime-base/strings.hh

These files do not overlap #12141, #12142, #12145, #12148, #12150, or #12153.

Changes

Host environment diagnostics

host-environment.hh is shared by CoreCLR and NativeAOT, but not MonoVM.

  • migrate generated system-property diagnostics to %s formatting;
  • migrate XDG directory creation diagnostics to %s formatting;
  • migrate XDG directory failure diagnostics while preserving the original errno text;
  • retain optional_string() handling for null C-string inputs.

Integer parsing diagnostics

strings.hh is shared by all three runtimes. Its variable diagnostics now use log_errorf() without runtime-specific branches:

  • an invalid starting index, using %zu for size_t;
  • signed/unsigned range failures, using explicitly converted %lld / %llu values;
  • values that do not represent an integer in the selected %d base;
  • trailing non-numeric characters.

The header forward-declares only log_errorf() instead of including the heavyweight formatting declarations from shared/log_types.hh.

Behavior preserved

  • integer conversion logic, range checks, errno handling, and output assignment are unchanged;
  • the copied parse buffer remains NUL-terminated before %s logging;
  • host environment variables and XDG directory creation behavior are unchanged;
  • debug category gating and unconditional error/warning semantics remain unchanged.

Runtime behavior

Runtime Result
NativeAOT Uses #12140 printf helpers for all migrated diagnostics
CoreCLR Uses #12140 printf helpers for shared parsing and host-environment diagnostics
MonoVM Uses the matching MonoVM log_errorf() implementation for shared parsing diagnostics

Measured impact

Representative Android arm64 Release objects compiled before the MonoVM path was unified:

Runtime/object Before After Difference
NativeAOT host-environment.cc.o 131,288 B 130,848 B -440 B
CoreCLR timing-internal.cc.o 194,048 B 193,088 B -960 B

These call sites did not contain explicit std::format expressions, so the impact is smaller than #12150/#12153; the change prevents their formatted logging templates from being instantiated.

Non-goals

  • no new configuration or parsing behavior;
  • no changes to environment-variable storage, XDG path construction, or integer parsing buffers;
  • no new logging tests;
  • no attempt to migrate unrelated configuration code outside these two 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 and size comparison;
  • focused review of signed/unsigned conversions, format widths, null termination, include dependencies, and runtime behavior;
  • latest head: 72cad5d01.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 HostEnvironment diagnostics in host-environment.hh to log_debugf() / log_warnf() with %s formatting and optional_string() for null safety.
  • Convert string_segment::to_integer() diagnostics in strings.hh to runtime-guarded log_errorf() for CoreCLR/NativeAOT (MonoVM retains existing log_error() + {} formatting).
  • Add an explicit shared/log_types.hh include for non-MonoVM builds in strings.hh to 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

Comment thread src/native/common/include/runtime-base/strings.hh Outdated
@simonrozsival simonrozsival added the drop-libcpp Work to remove the libc++ dependency from Android NativeAOT label Jul 17, 2026
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
@simonrozsival simonrozsival changed the title [CoreCLR/NativeAOT] Migrate configuration diagnostics to printf [Native] Migrate configuration diagnostics to printf Jul 17, 2026
@jonathanpeppers

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Android PR Reviewer completed successfully!

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 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

  • %s safetyoptional_string() returns const char* and the to_integer parse buffer s is explicitly NUL-terminated before logging, so all %s conversions are well-defined.
  • Format widths%zu for size_t start_index, %d for the int base, and %lld/%llu for the range bounds all match their argument types. The double casts (int64_tlong long, uint64_tunsigned long long) correctly bridge the fixed-width types to the printf promotion types across ABIs.
  • Range semantics — signed/unsigned min/max printing preserves the original std::format output for both signed and unsigned T.
  • Forward declaration — the manual log_errorf declaration exactly matches the canonical one in shared/log_types.hh (signature + format(printf,2,3) attribute), and LogCategories is in scope via helpers.hhjava-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 the dev/simonrozsival/nativeaot-printf-logging base 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


#include <shared/helpers.hh>
namespace xamarin::android {
void log_errorf (LogCategories category, const char *format, ...) noexcept __attribute__ ((format (printf, 2, 3)));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 💡 Native C++ — This forward declaration of log_errorf duplicates the canonical one in shared/log_types.hh. Deliberately avoiding that heavyweight include is reasonable, but if the signature ever changes the two can silently diverge (the mismatch only surfaces in translation units that include both headers). Consider adding a short comment pointing at the canonical declaration, e.g. // Forward-declared to avoid including shared/log_types.hh; keep in sync with it.

(Rule: Native C++ — reduce duplication drift)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drop-libcpp Work to remove the libc++ dependency from Android NativeAOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants