[PATCH v2 0/1] MR4928: ntdll: Remove libunwind support for ARM.
Building for ARM with libunwind available has been broken since 89f3c59739e6a879b6f362dfd29d41347590449d, due to references to raise_func_trampoline that were left behind. In Linux builds, libunwind isn't practically needed since a27b202a4ddc314e3e856c10f2e5d010c4a88ee0 (which implemented an internal EHABI unwinder). That unwinder currently only supports Linux, due to relying on dl_iterate_phdr, but if necessary, we could also try to detect support for dl_iterate_phdr in configure for other OSes. -- v2: ntdll: Remove libunwind support for ARM. https://gitlab.winehq.org/wine/wine/-/merge_requests/4928
From: Martin Storsjö <martin(a)martin.st> Building for ARM with libunwind available has been broken since 89f3c59739e6a879b6f362dfd29d41347590449d, due to references to raise_func_trampoline that were left behind. In Linux builds, libunwind isn't practically needed since a27b202a4ddc314e3e856c10f2e5d010c4a88ee0 (which implemented an internal EHABI unwinder). That unwinder currently only supports Linux, due to relying on dl_iterate_phdr, but if necessary, we could also try to detect support for dl_iterate_phdr in configure for other OSes. Signed-off-by: Martin Storsjö <martin(a)martin.st> --- dlls/ntdll/unix/signal_arm.c | 116 ++++------------------------------- 1 file changed, 12 insertions(+), 104 deletions(-) diff --git a/dlls/ntdll/unix/signal_arm.c b/dlls/ntdll/unix/signal_arm.c index 5115fa7ec3a..68c8f0c0486 100644 --- a/dlls/ntdll/unix/signal_arm.c +++ b/dlls/ntdll/unix/signal_arm.c @@ -51,10 +51,6 @@ #ifdef HAVE_SYS_UCONTEXT_H # include <sys/ucontext.h> #endif -#ifdef HAVE_LIBUNWIND -# define UNW_LOCAL_ONLY -# include <libunwind.h> -#endif #ifdef HAVE_LINK_H # include <link.h> #endif @@ -260,8 +256,6 @@ static BOOL is_inside_syscall( ucontext_t *sigcontext ) (char *)SP_sig(sigcontext) <= (char *)arm_thread_data()->syscall_frame); } -extern void raise_func_trampoline( EXCEPTION_RECORD *rec, CONTEXT *context, void *dispatcher ); - struct exidx_entry { uint32_t addr; @@ -559,11 +553,6 @@ static NTSTATUS ehabi_virtual_unwind( UINT ip, DWORD *frame, CONTEXT *context, if (!set_pc) context->Pc = context->Lr; - /* There's no need to check for raise_func_trampoline and manually restore - * Lr separately from Pc like with libunwind; the EHABI unwind info - * describes how both of them are restored separately, and as long as - * the unwind info restored Pc, it doesn't have to be set from Lr. */ - TRACE( "next function pc=%08lx\n", context->Pc ); TRACE(" r0=%08lx r1=%08lx r2=%08lx r3=%08lx\n", context->R0, context->R1, context->R2, context->R3 ); @@ -672,94 +661,6 @@ static const struct exidx_entry *find_exidx_entry( void *ip ) } #endif -#ifdef HAVE_LIBUNWIND -static NTSTATUS libunwind_virtual_unwind( DWORD ip, DWORD *frame, CONTEXT *context, - PEXCEPTION_ROUTINE *handler, void **handler_data ) -{ - unw_context_t unw_context; - unw_cursor_t cursor; - unw_proc_info_t info; - int rc, i; - - for (i = 0; i <= 12; i++) - unw_context.regs[i] = (&context->R0)[i]; - unw_context.regs[13] = context->Sp; - unw_context.regs[14] = context->Lr; - unw_context.regs[15] = context->Pc; - rc = unw_init_local( &cursor, &unw_context ); - - if (rc != UNW_ESUCCESS) - { - WARN( "setup failed: %d\n", rc ); - return STATUS_INVALID_DISPOSITION; - } - rc = unw_get_proc_info( &cursor, &info ); - if (UNW_ENOINFO < 0) rc = -rc; /* LLVM libunwind has negative error codes */ - if (rc != UNW_ESUCCESS && rc != -UNW_ENOINFO) - { - WARN( "failed to get info: %d\n", rc ); - return STATUS_INVALID_DISPOSITION; - } - if (rc == -UNW_ENOINFO || ip < info.start_ip || ip > info.end_ip) - { - NTSTATUS status = context->Pc != context->Lr ? - STATUS_SUCCESS : STATUS_INVALID_DISPOSITION; - TRACE( "no info found for %x ip %x-%x, %s\n", - ip, info.start_ip, info.end_ip, status == STATUS_SUCCESS ? - "assuming leaf function" : "error, stuck" ); - *handler = NULL; - *frame = context->Sp; - context->Pc = context->Lr; - context->ContextFlags |= CONTEXT_UNWOUND_TO_CALL; - return status; - } - - TRACE( "ip %#x function %#lx-%#lx personality %#lx lsda %#lx fde %#lx\n", - ip, (unsigned long)info.start_ip, (unsigned long)info.end_ip, (unsigned long)info.handler, - (unsigned long)info.lsda, (unsigned long)info.unwind_info ); - - rc = unw_step( &cursor ); - if (rc < 0) - { - WARN( "failed to unwind: %d %d\n", rc, UNW_ENOINFO ); - return STATUS_INVALID_DISPOSITION; - } - - *handler = (void *)info.handler; - *handler_data = (void *)info.lsda; - *frame = context->Sp; - - for (i = 0; i <= 12; i++) - unw_get_reg( &cursor, UNW_ARM_R0 + i, (unw_word_t *)&(&context->R0)[i] ); - unw_get_reg( &cursor, UNW_ARM_R13, (unw_word_t *)&context->Sp ); - unw_get_reg( &cursor, UNW_ARM_R14, (unw_word_t *)&context->Lr ); - unw_get_reg( &cursor, UNW_REG_IP, (unw_word_t *)&context->Pc ); - context->ContextFlags |= CONTEXT_UNWOUND_TO_CALL; - - if ((info.start_ip & ~(unw_word_t)1) == - ((unw_word_t)raise_func_trampoline & ~(unw_word_t)1)) { - /* raise_func_trampoline stores the original Lr at the bottom of the - * stack. The unwinder normally can't restore both Pc and Lr to - * individual values, thus do that manually here. - * (The function we unwind to might be a leaf function that hasn't - * backed up its own original Lr value on the stack.) */ - const DWORD *orig_lr = (const DWORD *) *frame; - context->Lr = *orig_lr; - } - - TRACE( "next function pc=%08lx%s\n", context->Pc, rc ? "" : " (last frame)" ); - TRACE(" r0=%08lx r1=%08lx r2=%08lx r3=%08lx\n", - context->R0, context->R1, context->R2, context->R3 ); - TRACE(" r4=%08lx r5=%08lx r6=%08lx r7=%08lx\n", - context->R4, context->R5, context->R6, context->R7 ); - TRACE(" r8=%08lx r9=%08lx r10=%08lx r11=%08lx\n", - context->R8, context->R9, context->R10, context->R11 ); - TRACE(" r12=%08lx sp=%08lx lr=%08lx pc=%08lx\n", - context->R12, context->Sp, context->Lr, context->Pc ); - return STATUS_SUCCESS; -} -#endif - /*********************************************************************** * unwind_builtin_dll */ @@ -771,16 +672,23 @@ NTSTATUS unwind_builtin_dll( void *args ) DWORD ip = context->Pc - (dispatch->ControlPcIsUnwound ? 2 : 0); #ifdef linux const struct exidx_entry *entry = find_exidx_entry( (void *)ip ); + NTSTATUS status; if (entry) return ehabi_virtual_unwind( ip, &dispatch->EstablisherFrame, context, entry, &dispatch->LanguageHandler, &dispatch->HandlerData ); -#endif -#ifdef HAVE_LIBUNWIND - return libunwind_virtual_unwind( ip, &dispatch->EstablisherFrame, context, - &dispatch->LanguageHandler, &dispatch->HandlerData ); + + status = context->Pc != context->Lr ? + STATUS_SUCCESS : STATUS_INVALID_DISPOSITION; + TRACE( "no info found for %lx, %s\n", ip, status == STATUS_SUCCESS ? + "assuming leaf function" : "error, stuck" ); + dispatch->LanguageHandler = NULL; + dispatch->EstablisherFrame = context->Sp; + context->Pc = context->Lr; + context->ContextFlags |= CONTEXT_UNWOUND_TO_CALL; + return status; #else - ERR("libunwind not available, unable to unwind\n"); + ERR("ARM EHABI unwinding available, unable to unwind\n"); return STATUS_INVALID_DISPOSITION; #endif } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4928
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=142328 Your paranoid android. === debian11b (64 bit WoW report) === mf: Unhandled exception: divide by zero in 64-bit code (0x0000000042e22c).
On Wed Jan 24 13:18:57 2024 +0000, Martin Storsjö wrote:
Yep - I guess that non-PE builds for ARM won't be necessary in the long run. As long as there's no huge extra burden to keep that going (I guess that you'll still support non-PE builds for x86 for some time still, due to downstream/packager/user inertia?), I think we probably should keep it alive for ARM as well for another little while (another major version?). If something crops up sooner, where it takes unreasonable amounts of work to keep that config working for ARM, I don't mind axing it sooner though. Actually, nevermind, we apparently don't support non-PE builds for aarch64 at all since 11486a7b48aa43421ada5a557d892feabf27b372, so there's probably no need to keep support it for ARM either, as it's admittedly a very fringe architecture here.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4928#note_58832
On Wed Jan 24 13:26:22 2024 +0000, Martin Storsjö wrote:
Actually, nevermind, we apparently don't support non-PE builds for aarch64 at all since 11486a7b48aa43421ada5a557d892feabf27b372, so there's probably no need to keep support it for ARM either, as it's admittedly a very fringe architecture here. Yes, aarch64 is nice and simple now, it would be nice to do the same, so that the asm side of ARM is less painful to change. Especially since as you've noticed, I can't test it properly <g>
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4928#note_58837
participants (3)
-
Alexandre Julliard (＠julliard) -
Martin Storsjö
-
Marvin