[rfc] Handle broken CFI for signal trampolines in libc on amd64-linux

[rfc] Handle broken CFI for signal trampolines in libc on amd64-linux

[Ulrich Weigand]

Hello,

on my amd64-linux system a large number of signal-related test cases fail.
This happens because GDB doesn't properly recognize when it steps into a
signal handler.  The problem is that on amd64-linux, the signal trampoline
is not provided on the stack by the kernel, but is instead a regular library
function provided by libc, called __restore_rt.

Now there is code in amd64-linux-tdep.c to recognize that function, and
install a SIGFRAMP_FRAME for its call frame.  Unfortunately, that code
does never trigger, as the frame sniffer in question is on the sniffer
stack *after* the generic DWARF-2 sniffer.  As __restore_rt is also a
regular library function with CFI provided, the DWARF-2 sniffer will
recognize it and claim it as a NORMAL_FRAME.

Unfortunately even if I use the dwarf_signal_frame_p hook to have the
DWARF-2 sniffer recognize a SIGTRAMP_FRAME, it still doesn't work as
the __restore_rt CFI (at least in my glibc) only describes the unwind
effects as if it were a regular function, without taking into account
the restoring of registers by the sigreturn system call.

To fix this, I'd suggest to change amd64_init_abi to *prepend* the
hard-coded signal trampoline sniffer instead of appending it, so that
it takes precedence over the generic DWARF-2 sniffer.  This fixes all
signal-related test cases for me on amd64-linux.

As the trampoline sniffers shouldn't really have any false positives
and the hard-coded signal frame structure is unchangable ABI, there
doesn't seem to be any drawback to that approach.  I'd appreciate any
suggestions for a better fix ...

If there's no objection, I'm planning on committing this in a week.

Tested on amd-linux, fixes 67 signal-related FAILs.

Bye,
Ulrich



ChangeLog:

	* amd64-tdep.c (amd64_init_abi) Prepend the signal trampoline
	frame sniffer instead of appending it.

diff -urNp gdb-orig/gdb/amd64-tdep.c gdb-head/gdb/amd64-tdep.c
--- gdb-orig/gdb/amd64-tdep.c	2008-11-20 05:18:56.000000000 +0100
+++ gdb-head/gdb/amd64-tdep.c	2008-11-20 23:35:55.000000000 +0100
@@ -1359,7 +1359,13 @@ amd64_init_abi (struct gdbarch_info info
 
   set_gdbarch_dummy_id (gdbarch, amd64_dummy_id);
 
-  frame_unwind_append_unwinder (gdbarch, &amd64_sigtramp_frame_unwind);
+  /* On some amd64 systems, the signal trampoline is provided in a
+     library with DWARF-2 CFI, but without properly describing the
+     full unwind effects of the sigreturn call.  If the trampoline
+     is recognized by the DWARF-2 sniffer in that case, we get incorrect
+     results.  Therefore we *prepend* our hard-coded trampoline sniffer
+     here to ensure it takes precedence over the DWARF-2 sniffer.  */
+  frame_unwind_prepend_unwinder (gdbarch, &amd64_sigtramp_frame_unwind);
   frame_unwind_append_unwinder (gdbarch, &amd64_frame_unwind);
   frame_base_set_default (gdbarch, &amd64_frame_base);
 
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Handle broken CFI for signal trampolines in libc on amd64-linux

[Daniel Jacobowitz]

On Fri, Nov 21, 2008 at 12:38:11AM +0100, Ulrich Weigand wrote:
> Unfortunately even if I use the dwarf_signal_frame_p hook to have the
> DWARF-2 sniffer recognize a SIGTRAMP_FRAME, it still doesn't work as
> the __restore_rt CFI (at least in my glibc) only describes the unwind
> effects as if it were a regular function, without taking into account
> the restoring of registers by the sigreturn system call.

What version of glibc is this?  It was fixed two years ago this month:

2006-12-01  Jan Kratochvil  <jan.kratochvil@redhat.com>

        * sysdeps/unix/sysv/linux/x86_64/sigaction.c: Fix
	compatibility with
        libgcc not supporting `rflags' unwinding (register # >= 17).

2006-11-29  Daniel Jacobowitz  <dan@codesourcery.com>
            Jakub Jelinek  <jakub@redhat.com>
            Jan Kratochvil  <jan.kratochvil@redhat.com>

        * sysdeps/unix/sysv/linux/x86_64/sigaction.c (restore_rt): Add
	correct
        unwind information.
        * sysdeps/unix/sysv/linux/x86_64/Makefile: Provide symbols for
        'restore_rt' even in the 'signal' directory.
        * sysdeps/unix/sysv/linux/x86_64/ucontext_i.sym: Extend the
	regs list.



-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Handle broken CFI for signal trampolines in libc on amd64-linux

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Fri, Nov 21, 2008 at 12:38:11AM +0100, Ulrich Weigand wrote:
> > Unfortunately even if I use the dwarf_signal_frame_p hook to have the
> > DWARF-2 sniffer recognize a SIGTRAMP_FRAME, it still doesn't work as
> > the __restore_rt CFI (at least in my glibc) only describes the unwind
> > effects as if it were a regular function, without taking into account
> > the restoring of registers by the sigreturn system call.
> 
> What version of glibc is this?  It was fixed two years ago this month:

Well, it's the system glibc of my openSUSE 10.2 install (based on
a 2006-10-12 glibc-2.5 snapshot), which is indeed somewhat over two
years old.  I guess I could update to a more recent distro one of
these days ...

Anyway, while it is certainly good that this is fixed, I'm still
wondering why we should rely on that when we have a hard-coded
sigtramp detector that should be working just fine under any 
circumstances.

Bye,
Ulrich


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Handle broken CFI for signal trampolines in libc on amd64-linux

[Daniel Jacobowitz]

On Fri, Nov 21, 2008 at 02:33:29AM +0100, Ulrich Weigand wrote:
> Anyway, while it is certainly good that this is fixed, I'm still
> wondering why we should rely on that when we have a hard-coded
> sigtramp detector that should be working just fine under any 
> circumstances.

I think that one reason was the extra work of the signal handler
sniffer.  The amd64 one doesn't do much for named functions, though,
and functions with CFI are likely to be named.  I suggest asking
Mark Kettenis's opinion.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Handle broken CFI for signal trampolines in libc on amd64-linux

[Mark Kettenis]

>  On Fri, Nov 21, 2008 at 02:33:29AM +0100, Ulrich Weigand wrote:
> > Anyway, while it is certainly good that this is fixed, I'm still
> > wondering why we should rely on that when we have a hard-coded
> > sigtramp detector that should be working just fine under any
> > circumstances.
>
>  I think that one reason was the extra work of the signal handler
>  sniffer.  The amd64 one doesn't do much for named functions, though,
>  and functions with CFI are likely to be named.  I suggest asking
>  Mark Kettenis's opinion.

My memory is a bit hazy on this, but I think the idea was that the signal
frame unwinder would only be used for older versions of linux/glibc that
don't provide the necessary CFI, and that newer versions would provide
correct CFI which would give the kernel/glibc people complete freedom on
how to implement signal frames.  As such, I'm inclined to say "no" to your
diff.

Re: [rfc] Handle broken CFI for signal trampolines in libc on amd64-linux

[Ulrich Weigand]

Mark Kettenis wrote:
> >  On Fri, Nov 21, 2008 at 02:33:29AM +0100, Ulrich Weigand wrote:
> > > Anyway, while it is certainly good that this is fixed, I'm still
> > > wondering why we should rely on that when we have a hard-coded
> > > sigtramp detector that should be working just fine under any
> > > circumstances.
> >
> >  I think that one reason was the extra work of the signal handler
> >  sniffer.  The amd64 one doesn't do much for named functions, though,
> >  and functions with CFI are likely to be named.  I suggest asking
> >  Mark Kettenis's opinion.
> 
> My memory is a bit hazy on this, but I think the idea was that the signal
> frame unwinder would only be used for older versions of linux/glibc that
> don't provide the necessary CFI, and that newer versions would provide
> correct CFI which would give the kernel/glibc people complete freedom on
> how to implement signal frames.  As such, I'm inclined to say "no" to your
> diff.

OK, fair enough.  As the bug is really in my copy of glibc anyway, I'll
withdraw my patch.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com