Repository navigation
Conversation
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
There was a problem hiding this comment.
Pull request overview
Adjusts libunwind’s configure-time Windows-specific header shims so they only apply to Windows-target builds, aiming to avoid shadowing real target headers during Windows-host cross-compiles.
Changes:
- Narrows the
configure.cmakeWindows branch fromCLR_CMAKE_HOST_WIN32toCLR_CMAKE_HOST_WIN32 AND CLR_CMAKE_TARGET_WIN32. - Sends non-Windows targets built from Windows hosts through the normal header-detection path (
elf.h,endian.h, etc.). - Keeps the fake
stdatomic.h/stdalign.hand_Thread_localworkaround scoped to Windows-target builds.
Comments suppressed due to low confidence (1)
src/native/external/libunwind_extras/configure.cmake:20
- This only stops creating the fake C11 headers on new configures; it doesn't remove
artifacts/obj/external/libunwind/include/stdatomic.h/stdalign.hif they were already generated by a prior Windows-target or pre-fix Android configure.libunwind_extras/CMakeLists.txtstill puts that obj include directory on the compiler search path for Windows-host builds, so an incremental Android build from an existing tree will keep picking up the stale fake header until the user manually cleansartifacts/obj. That means the reported cross-build break remains reproducible unless the build starts from a clean obj tree.
if(CLR_CMAKE_HOST_WIN32 AND CLR_CMAKE_TARGET_WIN32)
# Our posix abstraction layer will provide these headers
set(HAVE_ELF_H 1)
set(HAVE_ENDIAN_H 1)
# MSVC compiler is currently missing C11 stdalign.h header
# Fake it until support is added
check_include_files(stdalign.h HAVE_STDALIGN_H)
if (NOT HAVE_STDALIGN_H)
configure_file(${CLR_SRC_NATIVE_DIR}/external/libunwind/include/remote/win/fakestdalign.h.in ${CMAKE_CURRENT_BINARY_DIR}/include/stdalign.h COPYONLY)
endif (NOT HAVE_STDALIGN_H)
# MSVC compiler is currently missing C11 stdatomic.h header
check_c_source_compiles("#include <stdatomic.h> void main() { _Atomic int a; }" HAVE_STDATOMIC_H)
if (NOT HAVE_STDATOMIC_H)
configure_file(${CLR_SRC_NATIVE_DIR}/external/libunwind/include/remote/win/fakestdatomic.h.in ${CMAKE_CURRENT_BINARY_DIR}/include/stdatomic.h COPYONLY)
Isn't this the actual problem? We should not be including the Windows-hosted path when building code that is going to run on Android device. |
jkotas
left a comment
There was a problem hiding this comment.
This is not the right fix. It breaks runtime (Build windows-x64 release CrossDac) build as you can see in the CI.
The fake stdatomic.h / stdalign.h / _Thread_local headers exist because MSVC historically lacks those C11 headers. Two changes: 1. Gate their generation on cmake's MSVC variable (was CLR_CMAKE_HOST_WIN32) so non-MSVC compilers (e.g. Android NDK clang on a Windows host) don't generate fakes that would shadow the toolchain's real headers when the obj include dir is on the compile path. 2. Place the fakes under a libunwind-private include/msvc-shim/ subdir, only on libunwind's own compile include path. This structurally prevents Windows-host-shaped fakes from being reachable by any libunwind consumer (e.g. CoreCLR PAL when cross-compiling for Android), even if a future fake is added. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
83c00b5 to
8810db4
Compare
The comment referred to a 'libunwind-private' subdir that doesn't exist; the actual subdir is named msvc-shim. Update the wording to reference msvc-shim directly and clarify that the privacy is enforced by the include-path scoping in libunwind_extras/CMakeLists.txt (cmake's include_directories is directory-scoped and doesn't propagate to libunwind consumers). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The msvc-shim include directory was added to libunwind's -I path whenever CLR_CMAKE_HOST_WIN32 is true, but the fakes themselves are generated only when MSVC is true. A non-MSVC cross-build (e.g. Android NDK clang on a Windows host) sharing the same artifacts/obj tree as a prior MSVC build would still see the stale msvc-shim/stdatomic.h and shadow the toolchain's real header. Match the include-path gate to the generation gate so non-MSVC builds never search msvc-shim. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
161ad13 to
4cf1fa5
Compare
|
/cc @am11 |
| # Our posix abstraction layer will provide these headers | ||
| set(HAVE_ELF_H 1) | ||
| set(HAVE_ENDIAN_H 1) | ||
|
|
||
| # MSVC compiler is currently missing C11 stdalign.h header | ||
| # Fake it until support is added | ||
| # Fake it until support is added. Place fakes under a msvc-shim |
There was a problem hiding this comment.
# Fake it until support is added.
Has an issue been filed?
There was a problem hiding this comment.
There was a problem hiding this comment.
No, I haven't filed an issue yet, and I don't know if it was filed when the fakes were initially added.
It looks like there are some relevant issues
https://developercommunity.visualstudio.com/t/stdatomich-does-not-work-C11/11051100 suggests that using /std:c11 and /experimental:c11atomics would resolve needing to fake stdatomic.h.
And it seems like #include <stdalign.h> works in MSVC 19.44 without requiring flags, so we could remove the logic for faking it.
Given this, should we pivot to cleaning up those stdalign.h and stdatomic.h fakes and trying to add /std:c11 and /experimental:c11atomics to libunwind_extras/configure.cmake and libunwind_extras/CMakeLists.txt
|
We should add a build-only leg in the outerloop |
| @@ -2,36 +2,41 @@ include(CheckCSourceCompiles) | |||
| include(CheckIncludeFiles) | |||
| include(CheckFunctionExists) | |||
|
|
|||
| if(CLR_CMAKE_HOST_WIN32) | |||
| if(MSVC) | |||
There was a problem hiding this comment.
Yes, android is a cross building scenario on windows and in this case we want the else block to run.
There was a problem hiding this comment.
Sounds like we need CLR_CMAKE_TARGET_WIN32. We generally tend to use our own flags for consistency, which is why they are provided in the first place. :)
There was a problem hiding this comment.
We use standard host/target conventions: https://gcc.gnu.org/onlinedocs/gccint/Configure-Terms.html
HOST is meant to describe the platform where the binary is going to run. If you are cross-building binaries that are going to run on Android device, it should not be defined. If you are seeing CLR_CMAKE_HOST_WIN32 defined when building binaries that are going to run Android device, we may have a problem in the build setup.
There was a problem hiding this comment.
We use standard host/target conventions: gcc.gnu.org/onlinedocs/gccint/Configure-Terms.html
That convention has three parts: build, host and target while we only have host and target. In their convention host=null means host=build. We are trying host=null to mean the code is for foreign target (crossbuild).
There was a problem hiding this comment.
HOSTis meant to describe the platform where the binary is going to run. If you are cross-building binaries that are going to run on Android device, it should not be defined. If you are seeing CLR_CMAKE_HOST_WIN32 defined when building binaries that are going to run Android device, we may have a problem in the build setup.
I don't think there are any others in the android path that I can see. There are some others lurking that would only be felt if you were targeting windows-like platforms.
There was a problem hiding this comment.
I went through a few build scenarios, and it looks like a clean android cross-build does not reproduce the bug, the values set during the build were:
DIAG_HOST_WIN32=
HOST_OS=android
TARGET_OS=android
CMAKE_SYSTEM_NAME=Android
MSVC=
CMAKE_HOST_SYSTEM_NAME=Windows
It seems like I had some stale artifacts from either building CrossDac or building all subsets at somepoint, leading to the fake stdatomic.h and affecting incremental android coreclr builds.
So, if we want to prevent stale-state shadowing in mixed Windows/Android dev workflows, this msvc-shim isolation change would reduce someone else also hitting the issue in the description.
Otherwise, we can just expect folks to recognize the issue as a stale-state issue and clean their artifacts and close this PR.
Or look to do away with the fake stdalign.h + stdatomic.h altogether (#127814 (comment))
|
Alternative #129246 that would close this PR |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "4cf1fa5997fe38255da6b3593aefd38a30e75932",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "87523393fdb14746ceb529ab308f11047819fd01",
"last_reviewed_commit": "4cf1fa5997fe38255da6b3593aefd38a30e75932",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "87523393fdb14746ceb529ab308f11047819fd01",
"last_recorded_worker_run_id": "29676135155",
"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": "4cf1fa5997fe38255da6b3593aefd38a30e75932",
"review_id": 4730524656
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Cross-compiling CoreCLR for Android from a Windows host fails to build libunwind (call to undeclared function 'atomic_compare_exchange_strong' in Gaddress_validator.c). Root cause: libunwind_extras/configure.cmake gated its fake C11 header generation on CLR_CMAKE_HOST_WIN32 (host-is-Windows) as a proxy for "compiler is MSVC and lacks C11 stdatomic.h". When the host is Windows but the active compiler is the Android NDK clang, the block still runs, emits a fake stdatomic.h into artifacts/obj/external/libunwind/include/, and that path shadows the NDK's real Bionic header. The fake defines the non-standard atomic_compare_and_exchange_strong, so real libunwind code calling the C11 atomic_compare_exchange_strong fails to compile. This is a genuine build break not caught by CI because official Android cross-builds run from Linux hosts.
Approach: Two complementary changes. (1) Gate the fake-header block on cmake's built-in MSVC variable instead of CLR_CMAKE_HOST_WIN32. MSVC is true only when the active compiler defines _MSC_VER (cl.exe or clang-cl), which is precisely the original intent expressed by the surrounding comments, and matches the convention used elsewhere in CoreCLR cmake. (2) Redirect the generated fakes into an include/msvc-shim/ subdirectory that is added to the include path only inside libunwind_extras/CMakeLists.txt's Windows-host block, and only when MSVC is true. Because cmake's directory-scoped include_directories propagates only to targets in the same subdirectory, this structurally prevents the fakes from ever reaching libunwind consumers (PAL, JIT, VM, src/native/libs/*), providing defense in depth even if a future fake header is added.
Summary: The diagnosis is correct and the fix is minimal and well-targeted. Switching from a host-OS proxy to the MSVC compiler check is the right predicate, and isolating the shims under msvc-shim/ addresses the earlier CrossDac regression that an initial version of this PR caused (the runtime (Build windows-x64 release CrossDac) and Installer_Build_And_Test legs now report success on the head commit 4cf1fa5). The else(CLR_CMAKE_HOST_WIN32) -> else(MSVC) rename is benign (cmake ignores the endif/else argument text). Note the behavioral corollary of change (1): a non-MSVC Windows host now falls into the else(MSVC) branch and runs check_include_files for elf.h/endian.h/etc. rather than assuming the POSIX abstraction layer provides them — this is the correct behavior for a real target sysroot (e.g. Android/Bionic) and is what makes the fix work. Two maintainers (jkotas, AaronRobinsonMSFT) have approved. The remaining red CI legs (browser-wasm library tests, a checked coreclr x86 libraries run, maccatalyst smoke, osx installer) are unrelated to a Windows/MSVC cmake gating change and appear to be infrastructure/flaky failures rather than regressions from this diff. A follow-up build-only outerloop leg for Android-on-Windows (suggested in the PR discussion) would be valuable to prevent regressions, and note an alternative PR (#129246) has been proposed that may supersede this one. No actionable code defects found in the diff.
Detailed Findings
No blocking issues. The change is correct, minimal, and consistent with existing CoreCLR cmake conventions.
- 💡 The two changes are coupled: gating fake generation on
MSVCalone would already fix the Android-on-Windows break, but pairing it with themsvc-shim/include isolation is what prevents any future fake from leaking to consumers and preserves the CrossDac scenario. Keeping both is the right call. - 💡 (non-blocking, already raised in discussion) Consider the suggested build-only outerloop leg for Android-cross-from-Windows so this class of break is caught by CI going forward.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 87.8 AIC · ⌖ 10.8 AIC · ⊞ 10K
When building Android CoreCLR on Windows in development of #126916, I encountered:
Problem
After #126155 (Update llvm-libunwind to 22.1.1), cross-compiling CoreCLR for Android from a Windows host fails with the above. Reproduces from a Windows machine with NDK r27.2:
CI doesn't catch this because the official Android cross-builds run from Linux hosts.
Root cause
libunwind_extras/configure.cmakegated its fake C11-header generation onCLR_CMAKE_HOST_WIN32— using "host is Windows" as a proxy for "compiler is MSVC and lacks C11 stdatomic.h" (the same file's comments confirm: "MSVC compiler is currently missing C11 stdatomic.h header").The proxy breaks when the host is Windows but the active compiler isn't MSVC, e.g. the Android NDK clang invoked by
android.toolchain.cmake. The block enters anyway, generates a fakestdatomic.hatartifacts/obj/external/libunwind/include/stdatomic.h, and that path shadows the NDK's real Bionic header on the include search path. The fake definesatomic_compare_and_exchange_strong(note_and_) instead of the standard C11atomic_compare_exchange_strong, breakingGaddress_validator.c:251.Fix
Two complementary changes:
Gate fake generation on cmake's
MSVCvariable (wasCLR_CMAKE_HOST_WIN32).MSVCis TRUE for MSVC or any compiler defining_MSC_VER(e.g. clang-cl) — exactly the original intent, and the convention used elsewhere in CoreCLR cmake (src/coreclr/CMakeLists.txt:12,72,jit/CMakeLists.txt:15, etc.).Isolate the fakes under
include/msvc-shim/— added to libunwind's own compile include path only (inlibunwind_extras/CMakeLists.txt's Windows-host block), not to any libunwind consumer's path. cmake's directory-scopedinclude_directoriesonly propagates to targets defined in the same subdir, so this structurally prevents Windows-host-shaped fakes from being reachable by any libunwind consumer (e.g. CoreCLR PAL, JIT, VM,src/native/libs/*) — even if a future fake header is added.MSVCcl.exeinclude/msvc-shim/(libunwind-only)cl.exeinclude/msvc-shim/(libunwind-only)Validation
Locally on a Windows host with NDK 27.2.12479018:
Gaddress_validator.c.obuilds cleanly ([58/108]), 0 errors.artifacts/obj/external/libunwind/include/contains only the legitimate generated headers (config.h,libunwind.h,libunwind-common.h,tdep/libunwind_i.h) — no fakes, nomsvc-shim/subdir (since this build is not MSVC).