Repository navigation
Conversation
The fake C11 headers under src/native/external/libunwind/include/remote/win/ were added in 2020 (dotnet#37521 "Add libunwind to cross DAC") as a workaround for MSVC's missing C11 stdalign.h and stdatomic.h support. Both have since become either redundant or replaceable with a compile flag: - stdalign.h: shipping in MSVC for years. check_include_files(stdalign.h ...) returns TRUE on every supported MSVC, so the fake-generation branch was already dead code that never ran. - stdatomic.h: still gated behind /experimental:c11atomics, but the flag has been stable since VS 2022 17.5 (2023). Enabling it on the libunwind MSVC compile lets the real <stdatomic.h> from MSVC be used directly. The atomic_compare_exchange_strong call in Gaddress_validator.c compiles against the real header. Changes: - Delete the fake-header configure blocks in libunwind_extras/configure.cmake - Delete fakestdalign.h.in and fakestdatomic.h.in templates - Add /experimental:c11atomics to MSVC compile options in libunwind_extras/CMakeLists.txt (gated on MSVC, not HOST_WIN32, so clang-cl is excluded since it does not accept the flag). The _Thread_local workaround block is intentionally left alone: its check_c_source_compiles probe is missing the C-standard-required `static` qualifier, so it would still fail on modern MSVC and the -D_Thread_local= fallback continues to be applied (libunwind on Windows is UNW_REMOTE_ONLY, so no actual thread-local storage is needed). Side benefit: this also structurally eliminates the cross-build stale-fake shadowing bug (dotnet#127814 territory) because no fake exists to shadow real NDK/Bionic headers in a mixed CrossDac + Android build environment. Validation (Windows host, MSVC 19.44.35211 / VS 18): - build.cmd -s linuxdac -c Release: SUCCEEDS (1:25, 0 errors, 0 warnings). mscordaccore.dll built; Gaddress_validator.c.obj compiled (192720 bytes). - build.cmd clr.runtime -os android -arch x64 -c checked -lc release: SUCCEEDS (6:49, 0 errors, 0 warnings). libcoreclr.so for Android built. - artifacts/obj/external/libunwind/include/ contains only the real generated headers (config.h, libunwind-common.h, libunwind.h, tdep/libunwind_i.h); no stdatomic.h, no stdalign.h, no msvc-shim/ subdir. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the dotnet/runtime vendored libunwind build to stop generating MSVC “fake” C11 headers (stdalign.h / stdatomic.h) and instead rely on real toolchain headers (with MSVC C11 atomics enabled via a compiler flag).
Changes:
- Remove the libunwind_extras CMake configure-time generation of fake
stdalign.h/stdatomic.h. - Delete the fake-header template files from
include/remote/win/. - Add MSVC’s
/experimental:c11atomicsoption to allow compiling against MSVC’s real<stdatomic.h>.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/native/external/libunwind/include/remote/win/fakestdatomic.h.in | Deletes the fake stdatomic.h template used by older MSVC workarounds. |
| src/native/external/libunwind/include/remote/win/fakestdalign.h.in | Deletes the fake stdalign.h template used by older MSVC workarounds. |
| src/native/external/libunwind_extras/configure.cmake | Removes fake-header generation logic from the libunwind_extras configure step. |
| src/native/external/libunwind_extras/CMakeLists.txt | Adds MSVC compile option to enable C11 atomics support. |
| # Enable C11 atomics so libunwind's <stdatomic.h> usage | ||
| # (e.g. atomic_compare_exchange_strong in Gaddress_validator.c) compiles | ||
| # against MSVC's real header instead of a generated fake. | ||
| # /std:c11 is already implied by CMAKE_C_STANDARD=11 set in libunwind/CMakeLists.txt. | ||
| if(MSVC) | ||
| add_compile_options(/experimental:c11atomics) | ||
| endif() |
| if(CLR_CMAKE_HOST_WIN32) | ||
| # Our posix abstraction layer will provide these headers | ||
| set(HAVE_ELF_H 1) | ||
| set(HAVE_ENDIAN_H 1) | ||
|
|
Two fixes for PR dotnet#129246 review (Copilot reviewer): 1. libunwind_extras/CMakeLists.txt: gate /experimental:c11atomics more precisely. CMake's MSVC variable means "MSVC-like command-line syntax" and is also TRUE for clang-cl, which does not accept the flag. Use `CMAKE_C_COMPILER_ID STREQUAL "MSVC"` so only real cl.exe gets it. Also scope the flag to C sources via `$<$<COMPILE_LANGUAGE:C>:...>` so it is not passed to C++ compilation (e.g. remote-unwind.cpp). 2. libunwind/CMakeLists.txt: remove the `if (NOT HAVE_STDALIGN_H)` and `if (NOT HAVE_STDATOMIC_H)` blocks that still referenced the deleted fakestdalign.h.in / fakestdatomic.h.in templates. The CoreCLR build enters libunwind via libunwind_extras/CMakeLists.txt and never reads libunwind/CMakeLists.txt, so the dangling references survived local CrossDac/Android/Windows validation; they would break anyone configuring libunwind directly from the repo. Validated: `build.cmd -s linuxdac -c Release` succeeds (1:19, 0 errors), Gaddress_validator.c.obj = 192720 bytes (byte-identical to prior successful builds, proves the c11atomics flag is in effect), no fake headers generated, no remaining fakestd* references anywhere in the repo. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Please upstream libunwind changes to https://github.com/libunwind/libunwind/ and update libunwind-version.txt file with that PR's URL. |
| # (e.g. atomic_compare_exchange_strong in Gaddress_validator.c) compiles | ||
| # against MSVC's real header instead of a generated fake. | ||
| # /std:c11 is already implied by CMAKE_C_STANDARD=11 set in libunwind/CMakeLists.txt. | ||
| # CMAKE_C_COMPILER_ID is used instead of MSVC so that clang-cl (which sets |
There was a problem hiding this comment.
We do not support clang-cl and I expect we would need number of changes to actually support it. This can be simplified to just use /experimental:c11atomics unconditionally.
|
@mdh1418 This is goodness that would be great to get in. Let me know what you need to address the outstanding comments. |
IIRC while the compiler handles C11 atomics, the IDE IntelliSense still breaks on them. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "1305dba1bb1e18702666b8a8da703b28aaa7cf85",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "ad50b412069ee7f274c585d191df797ac5548525",
"last_reviewed_commit": "1305dba1bb1e18702666b8a8da703b28aaa7cf85",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "ad50b412069ee7f274c585d191df797ac5548525",
"last_recorded_worker_run_id": "29679137442",
"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": "1305dba1bb1e18702666b8a8da703b28aaa7cf85",
"review_id": 4730524942
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: The fake C11 stdalign.h/stdatomic.h headers under src/native/external/libunwind/include/remote/win/ were added in 2020 (#37521) to work around MSVC's missing C11 support. They are now either dead code (check_include_files(stdalign.h ...) returns TRUE on every supported MSVC, so the fake branch never ran) or replaceable by a compiler flag. The fake stdatomic.h also caused error: call to undeclared function 'atomic_compare_exchange_strong' in Gaddress_validator.c when building Android CoreCLR on a Windows host. This PR removes the workaround at its root.
Approach: Delete the two fake-header configure_file blocks from both libunwind/CMakeLists.txt and libunwind_extras/configure.cmake, delete the .h.in templates, and add /experimental:c11atomics to the MSVC C-compile options in libunwind_extras/CMakeLists.txt so libunwind consumes MSVC's real <stdatomic.h>. The flag is correctly gated on CMAKE_C_COMPILER_ID STREQUAL "MSVC" (excluding clang-cl, which sets MSVC=TRUE but rejects the flag) and scoped to C sources via $<$<COMPILE_LANGUAGE:C>:...> so it is not passed to C++ compilation such as remote-unwind.cpp. /experimental:c11atomics has been stable since VS 2022 17.5.
Summary: LGTM. The change is well-scoped, correctly targeted, and validated by the author on both the linux DAC and Android CoreCLR-on-Windows builds. The compiler-flag gating is precise and the removed configure logic was verifiably redundant or replaceable. The only remaining stdalign.h/stdatomic.h reference (include_directories(include/remote/win)) points at a directory that still contains other shims (pthread.h, signal.h, etc.), so removing the two headers does not break other includes. I left one non-blocking nit inline about an inaccurate explanatory comment referencing CMAKE_C_STANDARD from a CMakeLists file that isn't part of this build path.
Detailed Findings
No blocking issues. One minor documentation nit is noted inline on libunwind_extras/CMakeLists.txt regarding the /std:c11 / CMAKE_C_STANDARD comment.
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.5 AIC · ⌖ 10.7 AIC · ⊞ 10K
| # Enable C11 atomics so libunwind's <stdatomic.h> usage | ||
| # (e.g. atomic_compare_exchange_strong in Gaddress_validator.c) compiles | ||
| # against MSVC's real header instead of a generated fake. | ||
| # /std:c11 is already implied by CMAKE_C_STANDARD=11 set in libunwind/CMakeLists.txt. |
There was a problem hiding this comment.
Nit: this comment states /std:c11 is implied by CMAKE_C_STANDARD=11 set in libunwind/CMakeLists.txt, but that upstream file is not part of the cross-DAC build. src/coreclr/CMakeLists.txt adds libunwind_extras via add_subdirectory(...external/libunwind_extras ...), and this libunwind_extras/CMakeLists.txt does not set CMAKE_C_STANDARD. The flag works because current MSVC defaults to a C11/C17 language mode for .c files, not because of that setting. Consider correcting the comment so future readers don't rely on a CMAKE_C_STANDARD value that isn't actually in effect here.
From #127814 (comment), this aims to address the root of
src/native/external/libunwind/src/mi/Gaddress_validator.c:251:11: error: call to undeclared function 'atomic_compare_exchange_strong'from building Android CoreCLR on Windows host.The fake C11 headers under src/native/external/libunwind/include/remote/win/ were added in 2020 (#37521 "Add libunwind to cross DAC") as a workaround for MSVC's missing C11 stdalign.h and stdatomic.h support. Both have since become either redundant or replaceable with a compile flag.
stdalign.h: shipping in MSVC for years. check_include_files(stdalign.h ...) returns TRUE on every supported MSVC, so the fake-generation branch was already dead code that never ran.
stdatomic.h: still gated behind /experimental:c11atomics, but the flag has been stable since VS 2022 17.5 (2023). Enabling it on the libunwind MSVC compile lets the real <stdatomic.h> from MSVC be used directly. The atomic_compare_exchange_strong call in Gaddress_validator.c compiles against the real header.
Changes:
Validation (Windows host, MSVC 19.44.35211 / VS 18):