[PATCH 0/21] MR4445: ntdll: Unwind the kernel stack on thread exit.

[Alexandre Julliard (＠julliard)]

Based on @rbernon's work in !1088, with additional ARM support and
some cleanups.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/ntdll/unix/signal_x86_64.c:
> #endif
>                     /* switch to kernel stack */
>                     "movq %rcx,%rsp\n"
> -
>               /* we're now on the kernel stack, stitch unwind info with previous frame */
> -
>               __ASM_CFI_CFA_IS_AT2(rsp, 0xb8, 0x01) /* frame->syscall_cfa */
> -
>               __ASM_CFI_REG_IS_AT2(rsp, rsp, 0xb8, 0x01)

```suggestion:-1+0
                   __ASM_CFI_CFA_IS_AT2(rsp, 0xa8, 0x01) /* frame->syscall_cfa */
                   __ASM_CFI_REG_IS_AT2(rsp, rsp, 0xa8, 0x01)
```

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/ntdll/unix/signal_i386.c:
> -
> -
>       user_mode_abort_thread
> - */
>
> +extern void DECLSPEC_NORETURN user_mode_abort_thread( NTSTATUS status, struct syscall_frame *frame );
> +__ASM_GLOBAL_FUNC( user_mode_abort_thread,
> -
>               "movl 8(%esp),%eax\n\t"     /* frame */
> -
>               "movl 0x38(%eax),%ebp\n\t"  /* frame->syscall_cfa */
> -
>               "movl 4(%esp),%eax\n\t "    /* status */
> -
>               "subl $8,%ebp\n\t"
> -
>               __ASM_CFI(".cfi_adjust_cfa_offset 4\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset %ebp,0\n\t")
> -
>               __ASM_CFI(".cfi_def_cfa_register %ebp\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset %ebx,-4\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset %esi,-8\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset %edi,-12\n\t")
> -
>               /* switch to kernel stack */
> -
>               "leal -16(%ebp),%esp\n\t"

I find it a bit awkward to rely on the current frame offsets to build the syscall frame CFI. I'd use absolute offsets instead, like in the dispatchers.

Also, as we use %esp to decide whether we are in or out of a syscall, I think it's better to connect the frames *after* %esp is updated. It probably does not matter so much here as it's aborting anyway, but I find it more consistent.

```suggestion:-7+0
                   /* switch to kernel stack */
                   "leal -16(%ebp),%esp\n\t"
                   /* we're now on the kernel stack, stitch unwind info with previous frame */
                   __ASM_CFI(".cfi_def_cfa %ebp,8\n\t")
                   __ASM_CFI(".cfi_val_offset %esp,0\n\t") /* <~~ Not completely sure about this one? */
                   __ASM_CFI(".cfi_offset %eip,+4\n\t")
                   __ASM_CFI(".cfi_offset %ebp,0\n\t")
                   __ASM_CFI(".cfi_offset %ebx,-4\n\t")
                   __ASM_CFI(".cfi_offset %esi,-8\n\t")
                   __ASM_CFI(".cfi_offset %edi,-12\n\t")
```

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/ntdll/unix/signal_x86_64.c:
> +/***********************************************************************
> -
> -
>       user_mode_abort_thread
> - */
>
> +extern void DECLSPEC_NORETURN user_mode_abort_thread( NTSTATUS status, struct syscall_frame *frame );
> +__ASM_GLOBAL_FUNC( user_mode_abort_thread,
> -
>               "movq 0xa8(%rsi),%rbp\n\t"  /* frame->syscall_cfa */
> -
>               "subq $0x10,%rbp\n\t"
> -
>               __ASM_CFI(".cfi_def_cfa_register %rbp\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset %rbx,-0x08\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset %r12,-0x10\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset %r13,-0x18\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset %r14,-0x20\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset %r15,-0x28\n\t")
> -
>               /* switch to kernel stack */
> -
>               "movq %rbp,%rsp\n\t"

Same comment here.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/ntdll/unix/signal_arm64.c:
> -
>               "sub x29, x1, #0xc0\n\t"
> -
>               __ASM_CFI(".cfi_def_cfa_register 29\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset 29,0x00\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset 30,0x08\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset 19,0x10\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset 20,0x18\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset 21,0x20\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset 22,0x28\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset 23,0x30\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset 24,0x38\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset 25,0x40\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset 26,0x48\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset 27,0x50\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset 28,0x58\n\t")
> -
>               /* switch to kernel stack */
> -
>               "mov sp, x29\n\t"

And here.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/ntdll/unix/signal_arm.c:
> */
>  static void quit_handler( int signal, siginfo_t *siginfo, void *sigcontext )
>  {
> - if (!is_inside_syscall( sigcontext )) user_mode_abort_thread( 0, arm_thread_data()->syscall_frame );

Out of curiosity, how does `pthread_exit` unwind work on ARM? Does it not need any unwind info somehow?