[Native] Join paths without dynamic strings

[simonrozsival]

Summary

Replace shared CoreCLR/NativeAOT path-building local strings with C buffers while preserving the original distinction between static and dynamic storage.

Changes

• make the low-level path formatter return the written length or the negative required capacity including NUL
• use a single non-template retry overload with an explicit stack-buffer capacity
• return the selected stack or exact-size malloc() buffer directly
• free returned storage only when it differs from the caller-owned stack buffer
• keep the former static_local_string XDG path stack-only
• migrate fallback log paths, environment overrides, libmonodroid.so detection, and timing output paths
• remove obsolete CLR local-string path helpers and dependencies

Validation

• Local native builds intentionally skipped; relying on CI validation

[Copilot]

Pull request overview

Replaces dynamic fallback-path construction with bounded, caller-owned storage for CoreCLR/NativeAOT logging.

Changes:

• Adds Util::join_paths with separator handling and capacity checks.
• Uses fixed stack storage for gref/lref fallback paths.
• Skips and logs paths exceeding buffer capacity.

Show a summary per file

File	Description
src/native/clr/runtime-base/logger.cc	Builds fallback log paths without dynamic strings.
src/native/clr/include/runtime-base/util.hh	Adds the bounded path-joining helper.

Review details

Suppressed comments (4)

src/native/clr/include/runtime-base/util.hh:343

• 💡 Maintainability — The separator behavior here is intentionally different from path_combine: it strips one leading / only when first already ends with /, while empty components are copied unchanged. Please document this contract next to the helper, including that buffer_size includes space for the terminating NUL, so future callers do not infer a different normalization policy.

			bool remove_duplicate_separator = first.ends_with ('/') && second.starts_with ('/');
			bool add_separator = !first.empty () && !second.empty () && !first.ends_with ('/') && !second.starts_with ('/');
			size_t second_offset = remove_duplicate_separator ? 1uz : 0uz;

src/native/clr/include/runtime-base/util.hh:363

• 💡 Native C++ — <cstring> guarantees the declaration in std, but this unqualified call relies on implementation-provided global namespace injection. Use std::memcpy here to keep the new helper portable.

Rule: Use C++ standard headers and names

				memcpy (destination, second.data () + second_offset, second_length);

src/native/clr/include/runtime-base/util.hh:355

• 💡 Native C++ — <cstring> guarantees the declaration in std, but this unqualified call relies on implementation-provided global namespace injection. Use std::memcpy here to keep the new helper portable.

Rule: Use C++ standard headers and names

				memcpy (destination, first.data (), first.length ());

src/native/clr/runtime-base/logger.cc:84

• 💡 Error message — When the fallback path is rejected, this is the only diagnostic, but it does not identify the directory or the capacity that was exceeded. Include the relevant lengths and path_buffer.size() so users can determine which configured path caused reference logging to be skipped.

		log_warnf (category, "Unable to open fallback log file: path is too long");

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Lite

[simonrozsival]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

Generated by Android PR Reviewer for #12511

[github-actions]

⚠️ Needs Changes — 0 errors, 1 warning, 0 suggestions.

The bounded helper is a clear improvement over implicit dynamic path storage, and all 44 CI checks are green. The remaining concern is direct boundary coverage for the new duplicated path primitive before it becomes a broader replacement for path_combine.

Generated by Android PR Reviewer for #12511 · gpt56 · 101.4 AIC · ⌖ 17.1 AIC · ⊞ 25.7K
_{Comment /review to run again}

[jonathanpeppers]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

Generated by Android PR Reviewer for #12511

[github-actions]

⚠️ Needs Changes

Findings: 0 errors · 1 warning · 0 suggestions

The exact-size calculation, overflow checks, separator handling, and NUL termination are clear, and the migrated call sites pair their allocations correctly. The new temporary-buffer API should preserve RAII so later early returns cannot leak heap-backed paths. The prior unresolved review feedback requesting focused boundary tests also still applies.

CI is still in progress: 13 of 44 checks have completed successfully, with 30 running and 1 queued; no failures are currently reported.

Generated by Android PR Reviewer for #12511 · gpt56 · 98.8 AIC · ⌖ 8.91 AIC · ⊞ 25.7K
_{Comment /review to run again}

[simonrozsival]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

Generated by Android PR Reviewer for #12511

[github-actions]

⚠️ Needs Changes

Findings: 0 errors · 0 warnings · 1 suggestion.

The stack-first/exact-size fallback contract is applied consistently across the migrated call sites, and cleanup covers each exit path. One redundant startup filesystem operation is called out inline. CI is still in progress (2 checks passed, 6 running, 1 queued), so this cannot be considered green yet.

Generated by Android PR Reviewer for #12511 · gpt56 · 97.5 AIC · ⌖ 19.6 AIC · ⊞ 25.7K
_{Comment /review to run again}