Convert non-Windows apphost to C#126367
Conversation
Create C source files for apphost that replace the C++ implementation: - apphost_main.c: main entry point - apphost_pal.c/h: platform abstraction layer (Unix) - apphost_trace.c/h: tracing system - apphost_utils.c/h: utility functions - apphost_fx_ver.c/h: semantic version parsing - apphost_fxr_resolver.c/h: hostfxr path resolution - apphost_bundle_marker.c/h: bundle detection - apphost_hostfxr_resolver.c/h: hostfxr loading Uses dn-vector from src/native/containers for collections. Windows builds retain existing C++ sources. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/2c33d365-8b70-4325-8eee-7f004ba86639 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
- Add stddef.h includes for size_t in headers - Fix HAVE_DIRENT_D_TYPE guard to use config.h - Fix dn_vector_at calls to pass element size - Add test_only_getenv for test install location overrides - Add test overrides in pal_get_dotnet_self_registered_dir and pal_get_default_installation_dir - Add ctype.h include for tolower Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/2c33d365-8b70-4325-8eee-7f004ba86639 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
- Remove dead code block (always-false DIR_SEPARATOR check) in apphost_main.c - Remove unused free_string_entry function in apphost_fxr_resolver.c - Add explicit errno.h include in apphost_pal.c Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/2c33d365-8b70-4325-8eee-7f004ba86639 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Move C source files from apphost/standalone/ to hostmisc/: - apphost_trace.c/h, apphost_pal.c/h, apphost_utils.c/h - apphost_fx_ver.c/h, apphost_fxr_resolver.c/h, apphost_bundle_marker.c/h Create hostmisc_c OBJECT library target in hostmisc/CMakeLists.txt with proper include directories and dependencies. Update apphost/standalone/CMakeLists.txt to use hostmisc_c and dn-containers CMake targets instead of compiling C files directly. Add dn-containers subdirectory to corehost and singlefilehost builds for non-Windows platforms. Update apphost_fxr_resolver.c to use <dn-vector.h> include path consistent with the dn-containers CMake target. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6be87a85-902f-4646-92e6-2fb6783a5f57 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Rewrite bundle_marker.h as a C++ wrapper around the C implementation in apphost_bundle_marker.c. Add extern "C" guards to apphost_bundle_marker.h for C++ compatibility. Remove bundle_marker.cpp since the C implementation in hostmisc_c (apphost_bundle_marker.c) is now the single source of truth. Update standalone/CMakeLists.txt and static/CMakeLists.txt to use apphost_bundle_marker.c instead of the removed bundle_marker.cpp. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/65fd3a5e-99f8-4acc-830f-644976f2cfb1 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
…mutex - Removed trace.cpp (C++ implementation) - Updated apphost_trace.h: Added pal_char_t typedef (wchar_t on Windows, char on Unix), extern C guards, va_list function variants - Updated apphost_trace.c: Added Windows support (wide chars, OutputDebugStringW, WriteConsoleW), replaced spinlock with minipal mutex, added _v versions of variadic functions for C++ forwarding - Updated trace.h: Now a thin C++ wrapper calling C functions via va_list - Updated CMakeLists.txt: Use apphost_trace.c instead of trace.cpp, add minipal/mutex.c to both hostmisc and hostmisc_c" Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/2c57cab1-6ebb-4ab7-bbbf-38835b4336b3 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Inline the getenv and xtoi helpers directly in apphost_trace.c to avoid depending on apphost_pal.c which is only in hostmisc_c." Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/2c57cab1-6ebb-4ab7-bbbf-38835b4336b3 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
- trace_error_v: Copy va_list before any consumption to avoid using a consumed va_list (undefined behavior per C11 §7.16.1) - trace_warning: Delegate to trace_warning_v instead of duplicating the implementation inline Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/2c57cab1-6ebb-4ab7-bbbf-38835b4336b3 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
- fx_ver.cpp: Rewrite to delegate to apphost_fx_ver.c for parse(), compare(), and as_str() operations. Handles char/wchar_t conversion for Windows compatibility. - utils.cpp: On non-Windows, delegate get_current_arch_name(), get_runtime_id(), get_host_version_description(), and get_dotnet_root_env_var_for_arch() to apphost_utils.c. - apphost_fx_ver.h: Add extern "C" guards, rename to c_fx_ver_t to avoid collision with C++ fx_ver_t struct. - apphost_utils.h: Add extern "C" guards, guard macros with #ifndef to avoid conflicts with utils.h. - apphost_pal.h: Add extern "C" guards. - CMakeLists.txt: Add apphost_fx_ver.c to all platforms, add apphost_pal.c and apphost_utils.c on non-Windows. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/15a555ca-6237-4c3e-9a6a-93b3c993d1e9 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
- Revert get_dotnet_root_env_var_for_arch to C++ implementation since the C version ignores the arch parameter - Replace 'auto' with explicit 'pal::char_t' in palstr_to_narrow Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/15a555ca-6237-4c3e-9a6a-93b3c993d1e9 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
|
Tagging subscribers to this area: @agocke, @jeffschwMSFT, @elinor-fung |
Rename all hostmisc C files per review feedback: - apphost_trace.c → trace.c, apphost_trace.h → trace_c.h - apphost_fx_ver.c → fx_ver.c, apphost_fx_ver.h → fx_ver_c.h - apphost_pal.c → pal.c, apphost_pal.h → pal_c.h - apphost_utils.c → utils.c, apphost_utils.h → utils_c.h - apphost_fxr_resolver.c → fxr_resolver.c, apphost_fxr_resolver.h → fxr_resolver_c.h - apphost_bundle_marker.c → bundle_marker.c, apphost_bundle_marker.h → bundle_marker_c.h Update all #include directives, CMakeLists.txt references, header guards, and comments to use the new names. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/869a7b2a-c34a-4b37-ab96-28caca72225e Co-authored-by: elinor-fung <47805090+elinor-fung@users.noreply.github.com>
Done in 7b1a73c. Renamed all hostmisc C files to drop the |
Move the pal_char_t typedef from trace_c.h to pal_c.h as the canonical location. Define it cross-platform (wchar_t on Windows, char on non-Windows) with #ifndef guards to avoid redefinition conflicts with pal.h (C++ PAL). Update function signatures in pal_c.h, pal.c, utils_c.h, and utils.c to use pal_char_t instead of raw char for platform string parameters. Make trace_c.h include pal_c.h for the typedef instead of defining it locally. Restructure pal_c.h to be includable on all platforms (cross- platform types at top, POSIX-specific parts guarded with #if !defined(_WIN32)). Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/88526fd5-afd6-42dc-b551-5b6cd904b449 Co-authored-by: elinor-fung <47805090+elinor-fung@users.noreply.github.com>
Change hostfxr_resolver_t struct to use char* instead of char[4096]. Change fxr_resolver_try_get_path API to return malloc'd strings via char** instead of writing to fixed-size caller-provided buffers. Convert local path buffers in apphost_main.c and fxr_resolver.c to use malloc/free instead of stack-allocated fixed arrays. Add utils_file_exists_in_dir_alloc and utils_get_directory_alloc helpers that return dynamically allocated strings. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/84b0096a-a181-477e-a6b7-04df74e9919c Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
The dotnet muxer's fxr_resolver_try_get_path ignores the search location argument, so the single-use s_dotnet_search_location constant added no value. Pass fxr_search_location_default directly instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Cache the bundle_marker_is_bundle() result in exe_start instead of calling it twice. - Use pal_strdup in the static resolver instead of open-coding the size calculation, allocation, and copy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Hoist the .NET search embed size to a file-scope EMBED_DOTNET_SEARCH_SIZE define so the embed array and the app_relative_dotnet buffer share one constant instead of a duplicated literal. - Drop the unused <cinttypes> include from the dotnet muxer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Define pal_proc_t (FARPROC on Windows, void* on Unix) mirroring pal::proc_t, and return it from pal_get_symbol. On Windows this removes the union in pal_get_symbol and avoids any data/function pointer conversion: the resolved symbol stays a function pointer that callers cast directly to the target function type. The pal_dll_t and pal_proc_t typedefs live in the existing platform ifdef sections. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- pal_load_library takes a pal_dll_t* instead of void**, so the C++ pal::load_library wrapper no longer reinterpret_casts an HMODULE* to void** and writes a void* through a mismatched pointer type. The apphost resolver's hostfxr handle becomes pal_dll_t to match. - Remove the redundant pal::proc_t typedef; pal::get_symbol returns the C pal_proc_t directly, so no reinterpret_cast is needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot what is the size impact of this change on non-Windows apphosts? |
On a local
So the earlier size regression was transient while |
Missing the 'reason' why we can't avoid this divergence? |
Honestly, no real reason - apphost for non-Windows is the scope Jeremy started with and I didn't change it. But since I started with the scope of nethost for all platforms, most of the shared helpers and such should be converted now - doing the rest of apphost for Windows shouldn't be much more. |
|
Can we ask copilot for a rebase and remeasure with #129762? The previous values seem to be without that. |
I've the same feeling, it'll be easier to reason about when they are on the same plan. |
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
| void pal_unload_library(void* library); | ||
|
|
||
| // Resolve an exported symbol from a loaded library, or NULL if not found. | ||
| pal_proc_t pal_get_symbol(void* library, const char* name); | ||
|
|
Port apphost.windows.cpp to apphost.windows.c so the standalone Windows apphost links hostmisc_c (C only) instead of the C++ hostmisc, matching the non-Windows apphost. singlefilehost continues to use the same C entry point. Consolidate previously duplicated helpers into the C PAL/utils: - pal_err_print_line/pal_out_vprint_line/pal_file_vprintf now live in the PAL (pal.windows.c/pal.unix.c); trace.c and the C++ pal:: inlines that duplicated them are removed. - utils_starts_with/utils_ends_with added to utils.c; utils.cpp redirects to them. Adds pal_strncasecmp for the case-insensitive path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d32b6ad3-17a7-41e7-9d2a-09e337d3bf37
These took void* even though the strongly-typed pal_dll_t (HMODULE on Windows, void* on Unix) already exists and is used by pal_load_library. Using it here improves type-safety on Windows and lets pal_get_symbol drop the internal HMODULE cast. Callers already pass pal_dll_t, so no call sites change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d32b6ad3-17a7-41e7-9d2a-09e337d3bf37
| // Size of the embedded .NET search options buffer. | ||
| #define EMBED_DOTNET_SEARCH_SIZE 512 | ||
|
|
| if(CLR_CMAKE_TARGET_WIN32) | ||
| add_compile_definitions(UNICODE) | ||
| list(APPEND SOURCES | ||
| ../apphost.windows.cpp) | ||
|
|
||
| ../apphost.windows.c) | ||
| list(APPEND HEADERS |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "a2d778f3b75100e2ce3062f55b28eb5c6279acc5",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "e220bf0be5ec6d43445c6af075a8c7729ba01f7d",
"last_reviewed_commit": "a2d778f3b75100e2ce3062f55b28eb5c6279acc5",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "e220bf0be5ec6d43445c6af075a8c7729ba01f7d",
"last_recorded_worker_run_id": "29689447368",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "a2d778f3b75100e2ce3062f55b28eb5c6279acc5",
"review_id": 4730875069
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Clear and well-justified. The apphost/corehost was a mix of C++ that pulls in the STL and C++ runtime; rewriting the non-Windows (and now Windows) apphost as pure C shrinks the statically-linked host and removes C++ runtime dependencies. This is a legitimate, host-team-driven cleanup, and the maintainers who own this area (elinor-fung, jkoritzinsky, am11) drove the direction through many review rounds.
Approach: Sound. The PR consolidates duplicated print/string/library-loading logic into C pal_* helpers and makes the C++ pal:: wrappers delegate to them, avoiding two parallel implementations. Ownership contracts for the new heap-allocated buffers (host_path, app_path, app_root, resolver dotnet_root/fxr_path) are documented and the free() paths in exe_start and hostfxr_resolver_cleanup look correct and leak-free on the paths I traced. The static/standalone/dotnet resolver split cleanly separates the singlefilehost, dynamic-apphost, and muxer cases.
Summary: runtime CI leg currently reports failures on the head commit. A host-area maintainer should confirm those CI failures are unrelated/infra (Build Analysis + a runtime leg reporting build errors) and that single-file, GUI-apphost error dialog, and Windows event-log paths were exercised, since those are exactly the paths with no automated coverage. Minor observations below are non-blocking.
Detailed Findings
✅ PAL deduplication and ownership
The move of file_vprintf/err_print_line/out_vprint_line, load_library/get_symbol/unload_library, and is_path_fully_qualified into C pal_* functions with the C++ pal:: layer delegating is a clean, correct-by-construction dedup. pal_is_path_fully_qualified correctly folds in the old Windows is_path_not_fully_qualified logic (UNC, DOS-device \\?\, and drive-absolute) and adds the missing NULL guard used by callers.
✅ Buffer and encoding handling
pal_utf8_to_palstr is length-checked on both platforms (required > out_len on Unix; MultiByteToWideChar size probe on Windows), and the dynamic buffer growth in pal_load_library's trace path and format_alloc/buffering_trace_writer in apphost.windows.c handle realloc failure without leaking. report_outdated_framework_error and the bundle/startupinfo dispatch preserve the original error-writer propagation semantics.
💡 Minor: dead length check in apphost.c
In is_exe_enabled_for_execution, the early guard binding_len == 0 || binding_len >= app_dll_len (with app_dll_len == EMBED_MAX) already guarantees binding_len < EMBED_MAX, so the subsequent if (binding_len > EMBED_MAX - 1) check is unreachable. Harmless, but could be dropped for clarity. Non-blocking.
💡 Follow-up: test coverage for host startup
The host startup paths reworked here (single-file bundle detection, GUI apphost Task Dialog / applaunch URL, Windows Event Log reporting, v1 fallback) are not covered by unit tests and rely on the existing host functional tests plus manual validation. Given the blast radius, a maintainer should confirm the relevant host functional tests pass on all target platforms before merge.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 196.9 AIC · ⌖ 10.8 AIC · ⊞ 10K
Convert non-Windows apphost to C. On windows, we still have
apphost.windows.cppand it remains C++.dotnetandsinglefilehostalso remain C++.apphostanddotnetapphostentry point isapphost.c, shared by the standaloneapphostandsinglefilehost.dotnetentry point isdotnet.cppcorehost.cpphostfxrresolvers:apphostresolver is now Csinglefilehostkeeps its C++ resolver for statically-linked hostfxrdotnetresolver is separate and under the dotnet foldercc @dotnet/appmodel @AaronRobinsonMSFT
linux x64 Release
macOS arm64 Release