[PATCH] Unwinding CFI for x86_64 signal frame (__restore_rt)

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Unwinding CFI for x86_64 signal frame (__restore_rt)

Jan Kratochvil-2
Hi,

Frysk debugger in development would like to properly unwind signal frames
without any hacks as has been done in the gdb case. Provided patch sets proper
CFI unwind information for `__restore_rt'.

`__restore_rt' CFI is fixed only for x86_64 (tested on Fedora Core
kernel-xen-2.6.18-1.2849.fc6.x86_64) as on i386 (kernel-2.6.18-1.2747.el5.i686)
is in use VDSOed `__kernel_sigreturn' instead (with proper CFI already).
Still i386 should get fixed a similiar way but I do not have an easy testcase.

Currently gdb identifies signal frames using strcmp ("__restore_rt", ...)
(`amd64_linux_sigtramp_p') and gdb has also hardcoded arch-dependent unwind
info (register locations) for the signal frames.

I was told gcc is using `MD_FALLBACK_FRAME_STATE_FOR' (for exceptions
unwinding) but it is not suitable for ptrace(2)ing remote debuggers.

Testcase in glibc is not provided as the whole DWARF/CFI unwinding requires
framework IMO out of the glibc testcases' scope.


Regards,
Jan


Original Bug:
        https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=217087

Testcase provided in libunwind testsuite forked in the Frysk repository:
        CVSROOT=:pserver:[hidden email]:/cvs/frysk
        CVS Repository=frysk-imports/libunwind
        testcase: tests/run-ptrace-stepper
        Requires patch: http://sourceware.org/bugzilla/show_bug.cgi?id=3590

glibc-signal-unwind-cfi.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unwinding CFI for x86_64 signal frame (__restore_rt)

Jakub Jelinek
On Sat, Nov 25, 2006 at 07:06:16PM +0100, Jan Kratochvil wrote:

> +/* Define if CFI x86_64 rflags is available.  */
> +#undef HAVE_ASM_CFI_RFLAGS

I don't think this conditional is needed, we can use numbers instead
of register names and they will work with any unwinder.  The CFI_IF_RFLAGS
macros are just too ugly and e.g. cs etc. regnames have been added
together with rflags to gas.

> --- glibc-20061120T1000-orig/sysdeps/unix/sysv/linux/x86_64/sigaction.c 2006-10-28 08:44:03.000000000 +0200
> +++ glibc-20061120T1000/sysdeps/unix/sysv/linux/x86_64/sigaction.c 2006-11-25 16:59:17.000000000 +0100
> @@ -92,16 +92,49 @@ weak_alias (__libc_sigaction, sigaction)
>     If you ever feel the need to make any changes, please notify the
>     appropriate GDB maintainer.  */
>  
> +/* sizeof (struct rt_sigframe)  */
> +#define FRAME_SIZE 0x238
> +/* offsetof (struct rt_sigframe, uc.uc_mcontext)  */
> +#define SIGCONTEXT_BASE (-FRAME_SIZE + 0x28)

I think you should use here (and in the offsets of the various registers)
a *.sym file to precompute macros and use them.  There is already
sysdeps/unix/sysv/linux/x86_64/ucontext_i.sym, with oR* macros for the
registers, where oR8 is 0x28.

Another, more important thing, you are assuming the size of signal
frame is always 0x238 bytes.  But what guarantees that?  That's something
that can be hardcoded e.g. in vDSO signal description, because if you change
the kernel, you just adjust the CFI as well.  But not in glibc.  Say if
2.6.35 kernel needs to save some new fancy register in new hardware,
it will need extra space.  MD_FALLBACK_FRAME_STATE_FOR in gcc handles
this portably:
  new_cfa = sc->rsp;
  fs->regs.cfa_how = CFA_REG_OFFSET;
  /* Register 7 is rsp  */
  fs->regs.cfa_reg = 7;
  fs->regs.cfa_offset = new_cfa - (long) context->cfa;

  fs->regs.reg[0].how = REG_SAVED_OFFSET;
  fs->regs.reg[0].loc.offset = (long)&sc->rax - new_cfa;
...
So, new CFA is read from uc_mcontext.gregs[REG_RSP] and
all offsets adjusted.

This can be represented in DWARF3 with DW_CFA_def_cfa_expression
to define new CFA and DW_CFA_expression for all the registers
(where e.g. r8 could have DW_CFA_expression
.byte DW_OP_breg7; .sleb128 oR8), but current .cfi_escape
can't deal with that, because some of the constants are > 128
and you'd need to translate them to SLEB128 manually (well,
#if (oRDX >= 128 && oRDX < 256)
.cfi_escape oRDX, 1
#elif (oRDX < 128)
.cfi_escape oRDX
#else
#error ...
#endif
is possible, but way too unreadable).  I plan to change fix
that on the gas side soon, but for the time being I'd say
you'd have much more readable unwind info if you just coded
by hand a (commented) .eh_frame section, after all, .cfi_*
directives don't help you much here, because all you want
is cover the 2 instructions including 1 byte before them.
With an open coded .eh_frame for the sigreturn pad, you don't
need configure checks for .cfi_signal_frame, you just use
S at the end of the augmentation.

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unwinding CFI for x86_64 signal frame (__restore_rt)

Jan Kratochvil-2
Hi Jakub,

On Mon, 27 Nov 2006 12:43:39 +0100, Jakub Jelinek wrote:
...
> I plan to change fix that on the gas side soon,

you plan to support DW_OP_* as pseudoinstructions for gas(1)? That would be
great. Sure I could code it into the `.eh_frame' section by hand but that's
ugly and currently this bug is not a stopper, signal frame unwinding came just
as logical related bugfix.

If it is a part of your planned CFI GCC extension it could be soon.
I'll try to propose something if the implementation would take too long.




> > +/* Define if CFI x86_64 rflags is available.  */
> > +#undef HAVE_ASM_CFI_RFLAGS
>
> I don't think this conditional is needed, we can use numbers instead
> of register names and they will work with any unwinder.

As missing CFI is not a showstopper I chose the better code readability.
OK, you would like more the commented numbers.

...
> > +/* sizeof (struct rt_sigframe)  */
> > +#define FRAME_SIZE 0x238
...
> I think you should use here (and in the offsets of the various registers)
> a *.sym file to precompute macros and use them.

I see now, thanks for the suggestion.


> Another, more important thing, you are assuming the size of signal
> frame is always 0x238 bytes.
...
> This can be represented in DWARF3 with DW_CFA_def_cfa_expression
> to define new CFA and DW_CFA_expression for all the registers

OK, thanks for pointing me this way is right.
 * I believed these glibc parts are tightly bound to the kernel to accept
   such ABI compatibility interdependencies.
 * I missed gas(1) .cfi_* pseudoinstructions for the *_expression types.



Best Regards,
Jan
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unwinding CFI for x86_64 signal frame (__restore_rt)

Daniel Jacobowitz-2
In reply to this post by Jakub Jelinek
On Mon, Nov 27, 2006 at 12:43:39PM +0100, Jakub Jelinek wrote:
> is possible, but way too unreadable).  I plan to change fix
> that on the gas side soon, but for the time being I'd say
> you'd have much more readable unwind info if you just coded
> by hand a (commented) .eh_frame section, after all, .cfi_*
> directives don't help you much here, because all you want
> is cover the 2 instructions including 1 byte before them.
> With an open coded .eh_frame for the sigreturn pad, you don't
> need configure checks for .cfi_signal_frame, you just use
> S at the end of the augmentation.

FYI, I did this and was told not to do it that way.

  http://sourceware.org/ml/gdb/2006-07/msg00131.html

Followup discussion in the August archives too.  My plan was to use
macros to expand uleb128 in the few places it was needed for the
expressions.

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unwinding CFI for x86_64 signal frame (__restore_rt)

Jan Kratochvil-2
In reply to this post by Jan Kratochvil-2
Hi Jakub,

On Mon, 27 Nov 2006 23:06:58 +0100, Jakub Jelinek wrote:
...

> .cfi_startproc
> nop # some instructions
> ...
> .cfi_escape 0x10 # DW_CFA_expression
> .cfi_uleb128 3, 2f-1f # register 3, length
> .cfi_label 1 # Similar to 1:
> .cfi_escape 0x77 # DW_OP_breg8
> .cfi_sleb128 0x80 # offset
> .cfi_label 2 # Similar to 2:
> ...
> .cfi_endproc

it looks nice.  On top of it could be even easily built some higher-level
pseudoinstrs - like macros - to provide the (postfix notation) DW_OPs there:

        .cfi_startproc
        nop # some instructions
        .cfi_cfa_startexpr, rbx # register 3
        .cfi_op_breg r8, 0x80 # DW_OP_breg8, offset
        .cfi_cfa_endexpr
        ...
        .cfi_endproc



Best Regards,
Jan
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unwinding CFI for x86_64 signal frame (__restore_rt)

Jan Kratochvil-2
In reply to this post by Daniel Jacobowitz-2
Hi Jakub,

On Tue, 28 Nov 2006 11:47:56 +0100, Jakub Jelinek wrote:
> On Mon, Nov 27, 2006 at 04:46:22PM -0500, Daniel Jacobowitz wrote:
...
> >   http://sourceware.org/ml/gdb/2006-07/msg00131.html
...
> I actually think your patch above is (almost) fine, all you should change
> is 1) use oR* macros from ucontext_i.h 2) use .uleb128 resp. .sleb128
> directives as opposed to .byte when the argument is leb128, that way you
> can get rid e.g. of the do_expr_2 macro and can have just one, do_expr.
> 3) I'd put the nop before .align 16 and use
> " .long .LSTART_" #name "-1-.\n" /* PC-relative start address */ \
> for the start address, so that movq/syscall isn't unnecessarily unaligned.
>
> Really .cfi_* directives don't buy us anything here.

while I strongly disagree we still should code assembly instructions by hand
even in the 21st century the attached patch refactored the Daniel's one
following your guidelines.

It has been tested with the frysk's forked libunwind.


Thanks,
Jan

glibc-signal-frame-unwind2.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unwinding CFI for x86_64 signal frame (__restore_rt)

Jakub Jelinek
On Wed, Nov 29, 2006 at 12:04:10AM +0100, Jan Kratochvil wrote:
> 2006-11-28  Daniel Jacobowitz  <[hidden email]>
>    Jakub Jelinek  <[hidden email]>
>    Jan Kratochvil  <[hidden email]>
>
> * sysdeps/unix/sysv/linux/x86_64/sigaction.c (restore_rt): Add correct
> unwind information.
> * sysdeps/unix/sysv/linux/x86_64/Makefile: Prvide symbols for
> `restore_rt' even in the `signal' directory.
> * sysdeps/unix/sysv/linux/x86_64/ucontext_i.sym: Extend the regs list.

Looks good, only minor nits (below).

Not sure, but perhaps it would be more readable if the .eh_frame asm
wasn't in the macro, but just an extra asm () below the RESTORE.
There is just one RESTORE, you'd just add the nop and .LSTART_restore_rt
and .LEND_restore_rt symbols to the RESTORE macro, and you could get rid of
all the \'s at the end of the lines, #name uglification etc.
Perhaps the RESTORE macro could be dropped altogether, all we need
is something like CFI_STRINGIFY which would be defined unconditionally
and use it for the syscall number.
Guess Ulrich should decide how he wants it to look like.

> +asm \
> +  ( \
> +   /* `nop' for debuggers assuming `call' should not disalign the code.  */ \
> +   " nop\n" \
> +   ".align 16\n" \
> +   ".LSTART_" #name ":\n" \
> +   " nop\n" \

This nop is unneeded.

> +   " .type __" #name ",@function\n" \
> +   "__" #name ":\n" \
> +   " movq $" #syscall ", %rax\n" \
> +   " syscall\n" \
> +   ".LEND_" #name ":\n" \
> +   ".section .eh_frame,\"a\",@progbits\n" \
> +   ".LSTARTFRAME_" #name ":\n" \
> +   " .long .LENDCIE_" #name "-.LSTARTCIE_" #name "\n" \
> +   ".LSTARTCIE_" #name ":\n" \
> +   " .long 0\n" /* CIE ID */ \
> +   " .byte 1\n" /* Version number */ \
> +   " .string \"zRS\"\n" /* NUL-terminated augmentation string */ \
> +   " .uleb128 1\n" /* Code alignment factor */ \
> +   " .sleb128 -8\n" /* Data alignment factor */ \
> +   " .byte 0x10\n" /* Return address register column */ \
> +   " .uleb128 1\n" /* Augmentation value length */ \
> +   " .byte 0x1b\n" /* DW_EH_PE_pcrel|DW_EH_PE_sdata4. */ \
> +   " .byte 0\n" /* DW_CFA_nop */ \

Why is this DW_CFA_nop needed?  No insns are correct initial CIE
insns as well (and, .align 8 adds some anyway).

> +   " .align 8\n" \
> +   ".LENDCIE_" #name ":\n" \
> +   " .long .LENDFDE_" #name "-.LSTARTFDE_" #name "\n" /* FDE len */ \
> +   ".LSTARTFDE_" #name ":\n" \
> +   " .long .LSTARTFDE_" #name "-.LSTARTFRAME_" #name "\n" /* CIE */ \
> +   /* `LSTART_' is subtracted 1 as debuggers assume a `call' here.  */ \
> +   " .long (.LSTART_" #name "-1)-.\n" /* PC-relative start addr.  */ \
> +   " .long .LEND_" #name "-(.LSTART_" #name "-1)\n" \
> +   " .uleb128 0\n" /* Augmentation */ \
> +   do_cfa_expr \
> +   do_expr (8 /* r8 */, oR8) \
> +   do_expr (9 /* r9 */, oR9) \
> +   do_expr (10 /* r10 */, oR10) \
> +   do_expr (11 /* r11 */, oR11) \
> +   do_expr (12 /* r12 */, oR12) \
> +   do_expr (13 /* r13 */, oR13) \
> +   do_expr (14 /* r14 */, oR14) \
> +   do_expr (15 /* r15 */, oR15) \
> +   do_expr (5 /* rdi */, oRDI) \
> +   do_expr (4 /* rsi */, oRSI) \
> +   do_expr (6 /* rbp */, oRBP) \
> +   do_expr (3 /* rbx */, oRBX) \
> +   do_expr (1 /* rdx */, oRDX) \
> +   do_expr (0 /* rax */, oRAX) \
> +   do_expr (2 /* rcx */, oRCX) \
> +   do_expr (7 /* rsp */, oRSP) \
> +   do_expr (16 /* rip */, oRIP) \
> +   /* Older gas(1) does not support `rflags'.  */ \

This comment is not needed when .eh_frame is hand written.

> +   do_expr (49 /* rflags */, oEFL) \
> +   /* `cs'/`ds'/`fs' are unaligned and a different size.  */ \
> +   /* gas: Error: register save offset not a multiple of 8  */ \
> +   " .align 4\n" \

You want .align 8 here.

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unwinding CFI for x86_64 signal frame (__restore_rt)

Jan Kratochvil-2
Hi Jakub,

On Wed, 29 Nov 2006 00:44:09 +0100, Jakub Jelinek wrote:
...
> Not sure, but perhaps it would be more readable if the .eh_frame asm
> wasn't in the macro, but just an extra asm () below the RESTORE.
> There is just one RESTORE, you'd just add the nop and .LSTART_restore_rt
> and .LEND_restore_rt symbols to the RESTORE macro, and you could get rid of
> all the \'s at the end of the lines, #name uglification etc.
> Perhaps the RESTORE macro could be dropped altogether, all we need
> is something like CFI_STRINGIFY which would be defined unconditionally
> and use it for the syscall number.
> Guess Ulrich should decide how he wants it to look like.

this idea was dropped as we discussed before.


Other suggested changes were integrated.  Minor .byte->.uleb128 length
fix+change, I hope it does not violate the intended coding style.

Tested with libunwind run-ptrace-stepper.


Best Regards,
Jan

glibc-signal-frame-unwind3.patch (6K) Download Attachment
glibc-signal-frame-unwind-2-to-3.metapatch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unwinding CFI for x86_64 signal frame (__restore_rt)

Ulrich Drepper
I've checked the patch in with one further little patch.

--
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unwinding CFI for x86_64 signal frame (__restore_rt)

Jan Kratochvil-2
Hi,

the patch caused regression on exceptions unwinding (e.g. `nptl/tst-cancelx6').
libgcc_s does not expect register numbers >= 17 and `rflags' used there # 49.

It was found by Jakub on just `make -k check', very sorry I did not check it.


Shouldn't libgcc be extended for the full x86_64 ABI register set of 67 regs?
[ http://www.x86-64.org/documentation/abi.pdf ]
But I expect the number is reduced regarding the usefulness vs. performance.


Jan


Proposed patch extending register set for libunwind, similiar for gcc?
        http://sourceware.org/bugzilla/show_bug.cgi?id=3590

glibc-unwind-libgcc-compat.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unwinding CFI for x86_64 signal frame (__restore_rt)

Ulrich Drepper
Applied.

--
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖