<div dir="auto"><div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Feb 10, 2022, 2:54 AM Rémi Bernon <<a href="mailto:rbernon@codeweavers.com" target="_blank" rel="noreferrer">rbernon@codeweavers.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 2/9/22 17:00, Jinoh Kang wrote:<br>
> On 2/8/22 04:05, Rémi Bernon wrote:<br>
>> Making sure stack pointer points to previous syscall / exit frame before<br>
>> entering a syscall, and restoring the PE frame information on return.<br>
>><br>
>> Wine-Bug: <a href="https://bugs.winehq.org/show_bug.cgi?id=52213" rel="noreferrer noreferrer noreferrer" target="_blank">https://bugs.winehq.org/show_bug.cgi?id=52213</a><br>
>> Signed-off-by: Rémi Bernon <<a href="mailto:rbernon@codeweavers.com" rel="noreferrer noreferrer" target="_blank">rbernon@codeweavers.com</a>><br>
>> ---<br>
>> dlls/ntdll/unix/signal_i386.c | 9 +++++++++<br>
>> dlls/ntdll/unix/signal_x86_64.c | 9 +++++++++<br>
>> 2 files changed, 18 insertions(+)<br>
>><br>
>> diff --git a/dlls/ntdll/unix/signal_i386.c b/dlls/ntdll/unix/signal_i386.c<br>
>> index d98a3b1d4bb..2f6e2fd4153 100644<br>
>> --- a/dlls/ntdll/unix/signal_i386.c<br>
>> +++ b/dlls/ntdll/unix/signal_i386.c<br>
>> @@ -2492,6 +2492,9 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher,<br>
>> "movl %esi,0x30(%ecx)\n\t"<br>
>> "movl %ebp,0x34(%ecx)\n\t"<br>
>> "leal 0x34(%ecx),%ebp\n\t"<br>
>> + __ASM_CFI(".cfi_def_cfa %ebp,0\n\t")<br>
> <br>
> This changes the value of CFA. By definition, the actual value of CFA (not the CFA register) may never change within the context of a subroutine activation [1].<br>
> If we desire to switch CFA to a different frame anyway (with EIP overriden), we must end the current FDE with ".cfi_endproc" and start another FDE with ".cfi_startproc simple".<br>
> See [2] and [3] for how glibc achieves this.<br>
> <br>
>> + __ASM_CFI(".cfi_rel_offset %eip,-0x2c\n\t")<br>
> <br>
> This is the system call return address, which would be in a PE module.<br>
> I don't think this is very useful, since we will later switch to the exit frame anyway.<br>
> <br>
<br>
Yes, I added this here only because there's the same thing after the <br>
stack change and call, right after we restore the stack pointer. The <br>
latter is needed I think, as I understand the CFI are valid from their <br>
location up to the next cfi_endproc, and we want to restore the PE <br>
unwinding info as soon as we're off the unix stack.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I think what we want to do here is to mark the end of the call stack with ".cfi_undefined %eip". If we do want to return to the PE side, we may as well restore all the registers; a partial (not full) unwind would just make debugging things harder.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I guess then this should instead be done in the same way as glibc does <br>
for it to be correct, with cfi_endproc + cfi_startproc inserted on the <br>
boundaries where we're switching stacks.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Sounds great.</div><div dir="auto"><br></div><div dir="auto">Also, please keep in mind that glibc places its CFI pivot points after SP switch only because the old stack is no longer valid (clone) or needed (setcontext); as long as the CFA expression resolves to the same address value and the current frame is valid, it's ok to leave the CFI state unchanged.<br></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
>> + __ASM_CFI(".cfi_rel_offset %esp,-0x28\n\t")<br>
> <br>
> This makes GDB unhappy: "previous frame inner to this frame (corrupt stack)?" [4].<br>
> This is caused by jumping from one stack (the thread stack) to another (the kernel/syscall stack). This usually never happens unless we're calling __morestack or handling a signal.<br>
> The solution would be to have .cfi_signal_frame at the start of FDE; not sure if it is worth it...<br>
> <br>
<br>
Yes, GDB isn't happy about the syscall frame being above the PE stack, <br>
it then believes they are on the same stack but that the syscall frame <br>
frame is inner. For some reason, it now manages to unwind in my local <br>
setup, though I may just have hacked out the heuristic in a local GDB <br>
build because it was annoying.<br>
<br>
In any case if we manage to link the unix frames together it should then <br>
happily unwind the syscall frames up to the exit frame and skip all the <br>
PE ones. It is a bit unfortunate for debugging purposes, but there's not <br>
much to do about it, and this will fix this error.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">For debugging purposes I think the user can find some way to inject dynamic unwind info, so that shouldn't be a concern.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
>> /* Legends of Runeterra hooks the first system call return instruction, and<br>
>> * depends on us returning to it. Adjust the return address accordingly. */<br>
>> "subq $0xb,0x70(%rcx)\n\t"<br>
>> @@ -3206,6 +3209,9 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher,<br>
>> #endif<br>
>> "leaq 0x28(%rsp),%rsi\n\t" /* first argument */<br>
>> "movq %rcx,%rsp\n\t"<br>
>> + __ASM_CFI(".cfi_def_cfa %rbp,0\n\t")<br>
>> + __ASM_CFI(".cfi_rel_offset %rip,0x20\n\t") /* frame->unwind_rip */<br>
>> + __ASM_CFI(".cfi_rel_offset %rsp,0x08\n\t") /* frame->prev_frame */<br>
>> "movq 0x00(%rcx),%rax\n\t"<br>
>> "movq 0x18(%rcx),%rdx\n\t"<br>
>> "movl %eax,%ebx\n\t"<br>
>> @@ -3231,6 +3237,9 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher,<br>
>> "movq (%rbx),%r10\n\t" /* table->ServiceTable */<br>
>> "callq *(%r10,%rax,8)\n\t"<br>
>> "leaq -0x98(%rbp),%rcx\n"<br>
>> + __ASM_CFI(".cfi_def_cfa %rcx,0\n\t")<br>
>> + __ASM_CFI(".cfi_rel_offset %rip,0x70\n\t")<br>
>> + __ASM_CFI(".cfi_rel_offset %rsp,0x88\n\t")<br>
>> "2:\tmovl 0x94(%rcx),%edx\n\t" /* frame->restore_flags */<br>
>> #ifdef __linux__<br>
>> "testl $12,%r14d\n\t" /* SYSCALL_HAVE_PTHREAD_TEB | SYSCALL_HAVE_WRFSGSBASE */<br>
> <br>
> Also, it might be a good idea to apply this to the ARM side as well (although I'm not sure).<br>
> <br>
<br>
Yeah, I didn't do it because I have no idea which registers are needed <br>
there.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I suppose we can start with SP and LR.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> [1] DWARF Debugging Information Format Version 5, page 172, line 4 (§6.4 Call Frame Information), <a href="https://dwarfstd.org/doc/DWARF5.pdf" rel="noreferrer noreferrer noreferrer" target="_blank">https://dwarfstd.org/doc/DWARF5.pdf</a> ("(By definition, the CFA value does not change.)")<br>
> [2] <a href="https://elixir.bootlin.com/glibc/glibc-2.35.9000/source/sysdeps/unix/sysv/linux/x86_64/clone.S#L78" rel="noreferrer noreferrer noreferrer" target="_blank">https://elixir.bootlin.com/glibc/glibc-2.35.9000/source/sysdeps/unix/sysv/linux/x86_64/clone.S#L78</a><br>
> [3] <a href="https://elixir.bootlin.com/glibc/glibc-2.35.9000/source/sysdeps/unix/sysv/linux/x86_64/setcontext.S#L187" rel="noreferrer noreferrer noreferrer" target="_blank">https://elixir.bootlin.com/glibc/glibc-2.35.9000/source/sysdeps/unix/sysv/linux/x86_64/setcontext.S#L187</a><br>
> <br>
<br>
Thanks for the help!<br>
-- <br>
Rémi Bernon <<a href="mailto:rbernon@codeweavers.com" rel="noreferrer noreferrer" target="_blank">rbernon@codeweavers.com</a>><br>
</blockquote></div></div></div>