-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fixes/cleanup for use-system-unwind make flags #55639
Conversation
cleanup libunwind make flags
Make.inc
Outdated
ifeq ($(USE_SYSTEM_LIBUNWIND), 1) | ||
ifneq ($(OS),Darwin) | ||
LIBUNWIND:=-lunwind | ||
# Only for linux since we want to use not yet released libunwind features |
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 says "only for linux" here, but wouldn't this also be applied for OpenBSD and FreeBSD if USE_SYSTEM_LIBUNWIND=1
holds? Or maybe that can't happen? (Of course this is a question about the existing code, not something specific to this PR, so it is irrelevant for the acceptance of this PR)
Looking at the new logic, wouldn't the following happen on OpenBSD: if But perhaps this fine and/or this combo can not happen? |
Yeah seems ok to me. The system libunwind in macos is LLVM's anyway |
I think this one
This cannot happen anymore now (but I think would be fine anyway). |
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.
Looks good to me, thanks!
Assuming non-windows and libunwind not disabled: The flag `-DLLVMLIBUNWIND` is currently set on macos only for `USE_SYSTEM_UNWIND=0` which seems wrong to me and causes build issues for macos on Yggdrasil in combination with the recent #55049 which should only affect gnu libunwind (`error: call to undeclared function 'unw_ensure_tls'`). This flag is now set independently of the system-libunwind flag (on Darwin and OpenBSD as before). `LIBUNWIND=-lunwind` is set for `USE_SYSTEM_UNWIND=0` || `USE_SYSTEM_UNWIND=1` && `OS != Darwin`. I don't think the check for Darwin make sense and might be a leftover from using osxunwind a (long) while ago. Changed that to always set `-lunwind` if enabled. x-ref: JuliaPackaging/Yggdrasil#9331
Assuming non-windows and libunwind not disabled:
The flag
-DLLVMLIBUNWIND
is currently set on macos only forUSE_SYSTEM_UNWIND=0
which seems wrong to me and causes build issues for macos on Yggdrasil in combination with the recent #55049 which should only affect gnu libunwind (error: call to undeclared function 'unw_ensure_tls'
).This flag is now set independently of the system-libunwind flag (on Darwin and OpenBSD as before).
LIBUNWIND=-lunwind
is set forUSE_SYSTEM_UNWIND=0
||USE_SYSTEM_UNWIND=1
&&OS != Darwin
.I don't think the check for Darwin make sense and might be a leftover from using osxunwind a (long) while ago.
Changed that to always set
-lunwind
if enabled.x-ref: JuliaPackaging/Yggdrasil#9331