-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[libunwind] Call __arm_za_disable before entering EH
#160905
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
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-libunwind Author: Benjamin Maxwell (MacDue) ChangesThis is done by defining Full diff: https://github.com/llvm/llvm-project/pull/160905.diff 1 Files Affected:
diff --git a/libunwind/src/UnwindLevel1.c b/libunwind/src/UnwindLevel1.c
index f3b451ad9b730..0a1d4be976830 100644
--- a/libunwind/src/UnwindLevel1.c
+++ b/libunwind/src/UnwindLevel1.c
@@ -186,6 +186,10 @@ unwind_phase1(unw_context_t *uc, unw_cursor_t *cursor, _Unwind_Exception *except
}
extern int __unw_step_stage2(unw_cursor_t *);
+#if defined(__aarch64__)
+extern void __attribute__((weak)) __arm_za_disable(void);
+#endif
+
#if defined(_LIBUNWIND_USE_GCS)
// Enable the GCS target feature to permit gcspop instructions to be used.
__attribute__((target("+gcs")))
@@ -198,6 +202,29 @@ unwind_phase2(unw_context_t *uc, unw_cursor_t *cursor,
_LIBUNWIND_TRACE_UNWINDING("unwind_phase2(ex_obj=%p)",
(void *)exception_object);
+#if defined(__aarch64__)
+ // The platform must ensure that all the following conditions are true on
+ // entry to EH:
+ //
+ // - PSTATE.SM is 0.
+ // - PSTATE.ZA is 0.
+ // - TPIDR2_EL0 is null.
+ //
+ // The first point is ensured by routines for throwing exceptions having a
+ // non-streaming interface. TPIDR2_EL0 is set to null and ZA disabled by
+ // calling __arm_za_disable.
+ //
+ // See:
+ // https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#exceptions
+ if (__arm_za_disable) {
+ // FIXME: Is SME is available and `__arm_za_disable` is not, this should
+ // abort.
+ __arm_za_disable();
+ } else {
+ _LIBUNWIND_DEBUG_LOG("failed to call __arm_za_disable in %s", __FUNCTION__);
+ }
+#endif
+
// uc is initialized by __unw_getcontext in the parent frame. The first stack
// frame walked is unwind_phase2.
unsigned framesWalked = 1;
|
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'll leave it to code-owners/maintainers of libunwind to review or approve the PR further, but from an AArch64/SME point of view the change looks right to me.
|
Kind ping @llvm/reviewers-libunwind, @MaskRay 🙂 |
This is done by defining ` __arm_za_disable` as a weak symbol with the assumption that if libunwind is being linked against SME-aware code, the ABI routines will be provided (by libgcc or compiler-rt).
|
Does any libunwind reviewer have an opinion on this approach? If not, we plan to land this soon. An alternative approach would be to re-implement (Ping @llvm/reviewers-libunwind) |
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.
This is mostly a style review - I don't really know what za is?
libunwind/src/Registers.hpp
Outdated
| } | ||
|
|
||
| void Registers_arm64::jumpto() { | ||
| #if !defined(__APPLE__) |
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 better would be gating on a feature specific flag - it makes it easier to understand whether something is written the way it because the feature is not supported, or because the implementation of that feature is different - in essence someone new comes along and says "there's an 'if !', does the mean it is not available, or the abi is different?"
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.
This should work on macOS too (as the same routines should be present); however, I think weak symbols are handled differently there (looks like it's weak_import instead). I'm going to try that on the CI (as I don't have a Mac to hand right now).
| // calling __arm_za_disable. | ||
| // | ||
| // See: | ||
| // https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#exceptions |
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 believe we should link to something more stable like their tag versions, say https://github.com/ARM-software/abi-aa/blob/2025Q1/aapcs64/aapcs64.rst#exceptions
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'm not sure what that'd be here? I'm aware GCC versions their symbols, but this symbol could be provided by libgcc/ibgcc_s or compiler-rt, and I'm not sure what version information is generally available.
This is done by defining
__arm_za_disableas a weak symbol with the assumption that if libunwind is being linked against SME-aware code, the ABI routines will be provided (by libgcc or compiler-rt).