-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
Fix build with the new MacOS SDK. (define UNW_AARCH64 aliases conditionally) #84591
Conversation
The libunwind change seems have been introduced in this change: https://reviews.llvm.org/D107996 |
check_cxx_source_compiles(" | ||
#include <libunwind.h> | ||
|
||
int main(int argc, char **argv) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to not to use compilation check, as it would require adding this HAVE_UNW_AARCH64_X19
to tryrun.cmake. We should use check_symbol_exists
instead (I hope it will work for this symbol on both mac and Linux) which cmake can perform against target frame headers even in cross build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I tried, but check_symbol_exists
did not work for some reason. I concluded it does not work for enum elements, but perhaps I just used it incorrectly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I basically just did:
check_symbol_exists(UNW_AARCH64_X19 libunwind.h HAVE_UNW_AARCH64_X19)
similar to the other checks above, but maybe this is not the way for enums?
check_cxx_source_compiles works though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I guess this is the reason:
CheckSymbolExists
Provides a macro to check if a symbol exists as a function, variable, or macro in C
Actually, I was wrong, the check_cxx_source_compiles is actually still performed dynamically in cross build, the one that is not is the CheckCXXSourceRuns
. So this is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
Thanks!! |
define UNW_AARCH64 aliases conditionally (dotnet#84591)
Can we considering back porting this to release/7.0 and release/6.0? I'm unable to build those branches without this change. |
yes, I will start backporting PRs |
/backport to release/7.0-staging |
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/4705256183 |
/backport to release/6.0-staging |
Started backporting to release/6.0-staging: https://github.com/dotnet/runtime/actions/runs/4705265354 |
@VSadov backporting to release/7.0-staging failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: define UNW_AARCH64 aliases conditionally
Using index info to reconstruct a base tree...
M src/coreclr/pal/src/config.h.in
M src/coreclr/pal/src/configure.cmake
M src/coreclr/pal/src/exception/seh-unwind.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/pal/src/exception/seh-unwind.cpp
CONFLICT (content): Merge conflict in src/coreclr/pal/src/exception/seh-unwind.cpp
Auto-merging src/coreclr/pal/src/configure.cmake
Auto-merging src/coreclr/pal/src/config.h.in
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 define UNW_AARCH64 aliases conditionally
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
@VSadov an error occurred while backporting to release/7.0-staging, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
@VSadov backporting to release/6.0-staging failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: define UNW_AARCH64 aliases conditionally
Using index info to reconstruct a base tree...
M src/coreclr/pal/src/config.h.in
M src/coreclr/pal/src/configure.cmake
M src/coreclr/pal/src/exception/seh-unwind.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/pal/src/exception/seh-unwind.cpp
CONFLICT (content): Merge conflict in src/coreclr/pal/src/exception/seh-unwind.cpp
Auto-merging src/coreclr/pal/src/configure.cmake
Auto-merging src/coreclr/pal/src/config.h.in
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 define UNW_AARCH64 aliases conditionally
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
@VSadov an error occurred while backporting to release/6.0-staging, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
uh oh, will need to backport manually |
Fixes: #84557
It looks like libunwind.h that comes with the new MacOS SDK now has the enum for things like
UNW_AARCH64_X19
, so the workaround for not having those constantsruntime/src/coreclr/pal/src/exception/seh-unwind.cpp
Line 58 in 4ece8f0
now causes build failures due to duplicate definition.
Since this is an enum (not a define), we can't do simple
#ifndef
and need to do a configure test.