Skip to content

[Build] Limit libunwind fake stdatomic/stdalign generation to Windows-target builds #127814

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mdh1418 wants to merge 3 commits into
base: main
Choose a base branch
from
+18 −6
Open

[Build] Limit libunwind fake stdatomic/stdalign generation to Windows-target builds #127814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8810db4
Isolate libunwind fake C11 headers to MSVC-private include path
mdh1418 May 11, 2026
28282be
Fix misleading 'libunwind-private' reference in fake-header comment
mdh1418 May 15, 2026
4cf1fa5
Gate msvc-shim include path on MSVC
mdh1418 May 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/native/external/libunwind_extras/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,13 @@ if(CLR_CMAKE_HOST_WIN32)
include_directories(${CLR_ARTIFACTS_OBJ_DIR}/external/libunwind/include/tdep)
include_directories(${CLR_ARTIFACTS_OBJ_DIR}/external/libunwind/include)

# MSVC-only fake C11 headers (stdalign.h, stdatomic.h) generated by
# configure.cmake. Gated on MSVC so a non-MSVC build sharing this obj
# tree doesn't pick up stale fakes left behind by a prior MSVC build.
if(MSVC)
include_directories(${CLR_ARTIFACTS_OBJ_DIR}/external/libunwind/include/msvc-shim)
endif(MSVC)

# files for cross os compilation
include_directories(${CLR_SRC_NATIVE_DIR}/external/libunwind/include/remote)
include_directories(${CLR_SRC_NATIVE_DIR}/external/libunwind/include/remote/win)
Expand Down
17 changes: 11 additions & 6 deletions src/native/external/libunwind_extras/configure.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,36 +2,41 @@ include(CheckCSourceCompiles)
include(CheckIncludeFiles)
include(CheckFunctionExists)

if(CLR_CMAKE_HOST_WIN32)
if(MSVC)
Copy link
Copy Markdown
Member

@am11 am11 May 15, 2026

Choose a reason for hiding this comment

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

Are these changes needed?

Copy link
Copy Markdown
Member

@steveisok steveisok May 15, 2026

Choose a reason for hiding this comment

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

Yes, android is a cross building scenario on windows and in this case we want the else block to run.

Copy link
Copy Markdown
Member

@am11 am11 May 15, 2026

Choose a reason for hiding this comment

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

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. :)

Copy link
Copy Markdown
Member

@jkotas jkotas May 15, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@am11 am11 May 15, 2026

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Member

@steveisok steveisok May 15, 2026

Choose a reason for hiding this comment

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

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.

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.

Copy link
Copy Markdown
Member Author

@mdh1418 mdh1418 May 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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))

# 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
Copy link
Copy Markdown
Member

@AaronRobinsonMSFT AaronRobinsonMSFT May 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

# Fake it until support is added.

Has an issue been filed?

Copy link
Copy Markdown
Member

@AaronRobinsonMSFT AaronRobinsonMSFT May 15, 2026

Choose a reason for hiding this comment

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

https://developercommunity.visualstudio.com/home

Copy link
Copy Markdown
Member Author

@mdh1418 mdh1418 May 15, 2026

Choose a reason for hiding this comment

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

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

# subdir that is added to the include path only for libunwind's own
# compile (see libunwind_extras/CMakeLists.txt) and not for libunwind
# consumers (e.g. CoreCLR PAL when cross-compiling for Android), which
# would otherwise risk shadowing the real headers from the target
# sysroot.
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)
configure_file(${CLR_SRC_NATIVE_DIR}/external/libunwind/include/remote/win/fakestdalign.h.in ${CMAKE_CURRENT_BINARY_DIR}/include/msvc-shim/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)
configure_file(${CLR_SRC_NATIVE_DIR}/external/libunwind/include/remote/win/fakestdatomic.h.in ${CMAKE_CURRENT_BINARY_DIR}/include/msvc-shim/stdatomic.h COPYONLY)
endif (NOT HAVE_STDATOMIC_H)
Comment thread
mdh1418 marked this conversation as resolved.

# MSVC compiler is currently missing C11 _Thread_local
check_c_source_compiles("void main() { _Thread_local int a; }" HAVE_THREAD_LOCAL)
if (NOT HAVE_THREAD_LOCAL)
add_definitions(-D_Thread_local=)
endif (NOT HAVE_THREAD_LOCAL)
else(CLR_CMAKE_HOST_WIN32)
else(MSVC)
check_include_files(elf.h HAVE_ELF_H)
check_include_files(sys/elf.h HAVE_SYS_ELF_H)

check_include_files(endian.h HAVE_ENDIAN_H)
check_include_files(sys/endian.h HAVE_SYS_ENDIAN_H)
endif(CLR_CMAKE_HOST_WIN32)
endif(MSVC)

check_include_files(link.h HAVE_LINK_H)
check_include_files(sys/link.h HAVE_SYS_LINK_H)
Expand Down
Loading