Skip to content
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

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Apr 11, 2023

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 constants

// MacOS uses ARM64 instead of AARCH64 to describe these registers

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.

@VSadov
Copy link
Member Author

VSadov commented Apr 11, 2023

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)
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@VSadov
Copy link
Member Author

VSadov commented Apr 11, 2023

Thanks!!

@VSadov VSadov merged commit b014ab8 into dotnet:main Apr 11, 2023
@VSadov VSadov deleted the osxBuild branch April 11, 2023 18:52
UnityAlex pushed a commit to Unity-Technologies/runtime that referenced this pull request Apr 14, 2023
UnityAlex added a commit to Unity-Technologies/runtime that referenced this pull request Apr 14, 2023
@vcsjones
Copy link
Member

Can we considering back porting this to release/7.0 and release/6.0? I'm unable to build those branches without this change.

@VSadov
Copy link
Member Author

VSadov commented Apr 15, 2023

yes, I will start backporting PRs

@VSadov
Copy link
Member Author

VSadov commented Apr 15, 2023

/backport to release/7.0-staging

@github-actions
Copy link
Contributor

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/4705256183

@VSadov
Copy link
Member Author

VSadov commented Apr 15, 2023

/backport to release/6.0-staging

@github-actions
Copy link
Contributor

Started backporting to release/6.0-staging: https://github.com/dotnet/runtime/actions/runs/4705265354

@github-actions
Copy link
Contributor

@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!

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

@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!

@github-actions
Copy link
Contributor

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

@VSadov
Copy link
Member Author

VSadov commented Apr 15, 2023

uh oh, will need to backport manually

@ghost ghost locked as resolved and limited conversation to collaborators May 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't build with the new Apple SDK
3 participants