Re: gdb has outdated knowledge of signal trampolines

2013-11-26 Thread Konstantin Belousov 
On Mon, Nov 25, 2013 at 07:35:27PM +0200, Konstantin Belousov wrote:
 Could you update your gdb patch to use the KERN_PROC_SIGTRAMP from
 the patch below ? If this works out, I will add initialization of
 sv_szsigcode for ABIs which do not use shared page.

Below is the complete patch.  With it applied, I get
(gdb) bt
#0  sighandler (signo=1, info=0x7fffd2b0, context=Unhandled dwarf 
expression opcode 0xf3
) at siginfo.c:34
#1  signal handler called
#2  0x00080088849a in sigsuspend () from /lib/libc.so.7
#3  0x0040093a in main (argc=Unhandled dwarf expression opcode 0xf3
) at siginfo.c:54

diff --git a/contrib/gdb/gdb/amd64fbsd-nat.c b/contrib/gdb/gdb/amd64fbsd-nat.c
index f083734..dacd4a3 100644
--- a/contrib/gdb/gdb/amd64fbsd-nat.c
+++ b/contrib/gdb/gdb/amd64fbsd-nat.c
@@ -29,6 +29,7 @@
 #include sys/types.h
 #include sys/ptrace.h
 #include sys/sysctl.h
+#include sys/user.h
 #include machine/reg.h
 
 #ifdef HAVE_SYS_PROCFS_H
@@ -212,24 +213,23 @@ Please report this to bug-...@gnu.org.,
 
   SC_RBP_OFFSET = offset;
 
-  /* FreeBSD provides a kern.ps_strings sysctl that we can use to
+  /* FreeBSD provides a kern.proc.sigtramp sysctl that we can use to
  locate the sigtramp.  That way we can still recognize a sigtramp
- if its location is changed in a new kernel.  Of course this is
- still based on the assumption that the sigtramp is placed
- directly under the location where the program arguments and
- environment can be found.  */
+ if its location is changed in a new kernel. */
   {
-int mib[2];
-long ps_strings;
+int mib[4];
+struct kinfo_sigtramp kst;
 size_t len;
 
 mib[0] = CTL_KERN;
-mib[1] = KERN_PS_STRINGS;
-len = sizeof (ps_strings);
-if (sysctl (mib, 2, ps_strings, len, NULL, 0) == 0)
+mib[1] = KERN_PROC;
+mib[2] = KERN_PROC_SIGTRAMP;
+mib[3] = getpid();
+len = sizeof (kst);
+if (sysctl (mib, sizeof(mib) / sizeof(mib[0]), kst, len, NULL, 0) == 0)
   {
-   amd64fbsd_sigtramp_start_addr = ps_strings - 32;
-   amd64fbsd_sigtramp_end_addr = ps_strings;
+   amd64fbsd_sigtramp_start_addr = kst.ksigtramp_start;
+   amd64fbsd_sigtramp_end_addr = kst.ksigtramp_end;
   }
   }
 }
diff --git a/sys/amd64/include/pcb.h b/sys/amd64/include/pcb.h
index c106edc..80aff86 100644
--- a/sys/amd64/include/pcb.h
+++ b/sys/amd64/include/pcb.h
@@ -43,6 +43,7 @@
 #include machine/fpu.h
 #include machine/segments.h
 
+#ifdef __amd64__
 struct pcb {
register_t  pcb_r15;
register_t  pcb_r14;
@@ -105,6 +106,7 @@ struct pcb {
 
uint64_tpcb_pad[3];
 };
+#endif
 
 #ifdef _KERNEL
 struct trapframe;
diff --git a/sys/amd64/include/segments.h b/sys/amd64/include/segments.h
index d9f4280..6bcadc7 100644
--- a/sys/amd64/include/segments.h
+++ b/sys/amd64/include/segments.h
@@ -82,8 +82,8 @@ structsoft_segment_descriptor {
  * region descriptors, used to load gdt/idt tables before segments yet exist.
  */
 struct region_descriptor {
-   unsigned long rd_limit:16;  /* segment extent */
-   unsigned long rd_base:64 __packed;  /* base address  */
+   uint64_t rd_limit:16;   /* segment extent */
+   uint64_t rd_base:64 __packed;   /* base address  */
 } __packed;
 
 #ifdef _KERNEL
diff --git a/sys/compat/freebsd32/freebsd32.h b/sys/compat/freebsd32/freebsd32.h
index 8376e95..94f886e 100644
--- a/sys/compat/freebsd32/freebsd32.h
+++ b/sys/compat/freebsd32/freebsd32.h
@@ -362,6 +362,12 @@ struct kinfo_proc32 {
int ki_tdflags;
 };
 
+struct kinfo_sigtramp32 {
+   uint32_t ksigtramp_start;
+   uint32_t ksigtramp_end;
+   uint32_t ksigtramp_spare[4];
+};
+
 struct kld32_file_stat_1 {
int version;/* set to sizeof(struct kld_file_stat_1) */
charname[MAXPATHLEN];
diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c
index 9968e76..2e6bc32 100644
--- a/sys/kern/kern_proc.c
+++ b/sys/kern/kern_proc.c
@@ -2632,6 +2632,60 @@ errout:
return (error);
 }
 
+static int
+sysctl_kern_proc_sigtramp(SYSCTL_HANDLER_ARGS)
+{
+   int *name = (int *)arg1;
+   u_int namelen = arg2;
+   struct proc *p;
+   struct kinfo_sigtramp kst;
+   const struct sysentvec *sv;
+   int error;
+#ifdef COMPAT_FREEBSD32
+   struct kinfo_sigtramp32 kst32;
+#endif
+
+   if (namelen != 1)
+   return (EINVAL);
+
+   error = pget((pid_t)name[0], PGET_CANDEBUG, p);
+   if (error != 0)
+   return (error);
+   sv = p-p_sysent;
+#ifdef COMPAT_FREEBSD32
+   if ((req-flags  SCTL_MASK32) != 0) {
+   bzero(kst32, sizeof(kst32));
+   if (SV_PROC_FLAG(p, SV_ILP32)) {
+   if (sv-sv_sigcode_base != 0) {
+   kst32.ksigtramp_start = sv-sv_sigcode_base;
+   kst32.ksigtramp_end = sv-sv_sigcode_base +
+   *sv-sv_szsigcode;
+

gdb has outdated knowledge of signal trampolines

2013-11-25 Thread Andriy Gapon 

It seems that placement of signal trampolines was changed a while ago.  Possibly
with the introduction of the shared page, but I am not sure.
Unfortunately, neither the gdb in base nor the ports gdb were updated to account
for the new location.

And thus, for example:
(kgdb) bt
#0  thr_kill () at thr_kill.S:3
#1  0x0008032c89a7 in nsProfileLock::FatalSignalHandler (signo=6,
info=optimized out, context=0x7b197630)
at
/usr/obj/ports/usr/ports/www/firefox/work/mozilla-release/obj-x86_64-unknown-freebsd11.0/toolkit/profile/nsProfileLock.cpp:180
#2  0x000800f90596 in handle_signal (actp=optimized out, sig=6,
info=0x7b1979a0, ucp=0x7b197630) at 
/usr/src/lib/libthr/thread/thr_sig.c:237
#3  0x000800f9013f in thr_sighandler (sig=6, info=0x0, _ucp=0x7b197630)
at /usr/src/lib/libthr/thread/thr_sig.c:182
#4  0x7003 in ?? ()
#5  0x000800f90010 in ?? () at /usr/src/lib/libthr/thread/thr_sig.c:566 from
/lib/libthr.so.3
#6  0x in ?? ()

Obviously, the gdb is confused after the frame that has 0x7003.

I looked only at amd64 code, but I believe that other platforms (all of them?)
are affected as well.

The following proof of concept patch for the base gdb seems to fix the case of
native debugging on amd64 (target case was not tested).

diff --git a/contrib/gdb/gdb/amd64fbsd-nat.c b/contrib/gdb/gdb/amd64fbsd-nat.c
index f083734..d49dc45 100644
--- a/contrib/gdb/gdb/amd64fbsd-nat.c
+++ b/contrib/gdb/gdb/amd64fbsd-nat.c
@@ -212,24 +212,23 @@ Please report this to bug-...@gnu.org.,

   SC_RBP_OFFSET = offset;

-  /* FreeBSD provides a kern.ps_strings sysctl that we can use to
+  /* FreeBSD provides a kern.usrstack sysctl that we can use to
  locate the sigtramp.  That way we can still recognize a sigtramp
  if its location is changed in a new kernel.  Of course this is
  still based on the assumption that the sigtramp is placed
- directly under the location where the program arguments and
- environment can be found.  */
+ directly at usrstack.  */
   {
 int mib[2];
-long ps_strings;
+long usrstack;
 size_t len;

 mib[0] = CTL_KERN;
-mib[1] = KERN_PS_STRINGS;
-len = sizeof (ps_strings);
-if (sysctl (mib, 2, ps_strings, len, NULL, 0) == 0)
+mib[1] = KERN_USRSTACK;
+len = sizeof (usrstack);
+if (sysctl (mib, 2, usrstack, len, NULL, 0) == 0)
   {
-   amd64fbsd_sigtramp_start_addr = ps_strings - 32;
-   amd64fbsd_sigtramp_end_addr = ps_strings;
+   amd64fbsd_sigtramp_start_addr = usrstack;
+   amd64fbsd_sigtramp_end_addr = usrstack + 0x20;
   }
   }
 }
diff --git a/contrib/gdb/gdb/amd64fbsd-tdep.c b/contrib/gdb/gdb/amd64fbsd-tdep.c
index e4e02ab..87c1484 100644
--- a/contrib/gdb/gdb/amd64fbsd-tdep.c
+++ b/contrib/gdb/gdb/amd64fbsd-tdep.c
@@ -86,8 +86,8 @@ static int amd64fbsd_r_reg_offset[] =
 };

 /* Location of the signal trampoline.  */
-CORE_ADDR amd64fbsd_sigtramp_start_addr = 0x7fc0;
-CORE_ADDR amd64fbsd_sigtramp_end_addr = 0x7fe0;
+CORE_ADDR amd64fbsd_sigtramp_start_addr = 0x7000;
+CORE_ADDR amd64fbsd_sigtramp_end_addr = 0x7020;

 /* From machine/signal.h.  */
 int amd64fbsd_sc_reg_offset[] =

-- 
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org

Re: gdb has outdated knowledge of signal trampolines

2013-11-25 Thread Konstantin Belousov 
On Mon, Nov 25, 2013 at 12:13:53PM +0200, Andriy Gapon wrote:
 
 It seems that placement of signal trampolines was changed a while ago.  
 Possibly
 with the introduction of the shared page, but I am not sure.
 Unfortunately, neither the gdb in base nor the ports gdb were updated to 
 account
 for the new location.
 
 And thus, for example:
 (kgdb) bt
 #0  thr_kill () at thr_kill.S:3
 #1  0x0008032c89a7 in nsProfileLock::FatalSignalHandler (signo=6,
 info=optimized out, context=0x7b197630)
 at
 /usr/obj/ports/usr/ports/www/firefox/work/mozilla-release/obj-x86_64-unknown-freebsd11.0/toolkit/profile/nsProfileLock.cpp:180
 #2  0x000800f90596 in handle_signal (actp=optimized out, sig=6,
 info=0x7b1979a0, ucp=0x7b197630) at 
 /usr/src/lib/libthr/thread/thr_sig.c:237
 #3  0x000800f9013f in thr_sighandler (sig=6, info=0x0, 
 _ucp=0x7b197630)
 at /usr/src/lib/libthr/thread/thr_sig.c:182
 #4  0x7003 in ?? ()
 #5  0x000800f90010 in ?? () at /usr/src/lib/libthr/thread/thr_sig.c:566 
 from
 /lib/libthr.so.3
 #6  0x in ?? ()
 
 Obviously, the gdb is confused after the frame that has 0x7003.
 
 I looked only at amd64 code, but I believe that other platforms (all of them?)
 are affected as well.
 
 The following proof of concept patch for the base gdb seems to fix the case of
 native debugging on amd64 (target case was not tested).
 
 diff --git a/contrib/gdb/gdb/amd64fbsd-nat.c b/contrib/gdb/gdb/amd64fbsd-nat.c
 index f083734..d49dc45 100644
 --- a/contrib/gdb/gdb/amd64fbsd-nat.c
 +++ b/contrib/gdb/gdb/amd64fbsd-nat.c
 @@ -212,24 +212,23 @@ Please report this to bug-...@gnu.org.,
 
SC_RBP_OFFSET = offset;
 
 -  /* FreeBSD provides a kern.ps_strings sysctl that we can use to
 +  /* FreeBSD provides a kern.usrstack sysctl that we can use to
   locate the sigtramp.  That way we can still recognize a sigtramp
   if its location is changed in a new kernel.  Of course this is
   still based on the assumption that the sigtramp is placed
 - directly under the location where the program arguments and
 - environment can be found.  */
 + directly at usrstack.  */
{
  int mib[2];
 -long ps_strings;
 +long usrstack;
  size_t len;
 
  mib[0] = CTL_KERN;
 -mib[1] = KERN_PS_STRINGS;
 -len = sizeof (ps_strings);
 -if (sysctl (mib, 2, ps_strings, len, NULL, 0) == 0)
 +mib[1] = KERN_USRSTACK;
 +len = sizeof (usrstack);
 +if (sysctl (mib, 2, usrstack, len, NULL, 0) == 0)
{
 - amd64fbsd_sigtramp_start_addr = ps_strings - 32;
 - amd64fbsd_sigtramp_end_addr = ps_strings;
 + amd64fbsd_sigtramp_start_addr = usrstack;
 + amd64fbsd_sigtramp_end_addr = usrstack + 0x20;
}
}
  }
 diff --git a/contrib/gdb/gdb/amd64fbsd-tdep.c 
 b/contrib/gdb/gdb/amd64fbsd-tdep.c
 index e4e02ab..87c1484 100644
 --- a/contrib/gdb/gdb/amd64fbsd-tdep.c
 +++ b/contrib/gdb/gdb/amd64fbsd-tdep.c
 @@ -86,8 +86,8 @@ static int amd64fbsd_r_reg_offset[] =
  };
 
  /* Location of the signal trampoline.  */
 -CORE_ADDR amd64fbsd_sigtramp_start_addr = 0x7fc0;
 -CORE_ADDR amd64fbsd_sigtramp_end_addr = 0x7fe0;
 +CORE_ADDR amd64fbsd_sigtramp_start_addr = 0x7000;
 +CORE_ADDR amd64fbsd_sigtramp_end_addr = 0x7020;
 
  /* From machine/signal.h.  */
  int amd64fbsd_sc_reg_offset[] =
 

Yes, your observation is correct, but the patch could be improved.
Specifically, the location of the signal trampoline code which you
hard-coded into the patch is not guaranteed to be stable, and in fact
somewhat depends on the order of ABI sysvecs registration.  The size
of the signal trampoline is not fixed as well.

Real solution is to start provide vdso for signal trampolines, which
is probably the only real reason to have vdso at all. But this is
labor-intensive and I am not convinced that the ABI changes are worth it
right now.

Instead, I propose the following sysctl helper which should provide the
same 'hackish' way to get the trampoline location, which would work
both for 64bit and 32bit, for later on i386 and amd64. For core files,
this is as bad as before since we cannot guarantee stability of the
trampoline location.

Could you update your gdb patch to use the KERN_PROC_SIGTRAMP from
the patch below ? If this works out, I will add initialization of
sv_szsigcode for ABIs which do not use shared page.

Thank you for looking into this.

 sys/compat/freebsd32/freebsd32.h |  6 +
 sys/kern/kern_proc.c | 58 
 sys/sys/sysctl.h |  1 +
 sys/sys/user.h   |  6 +
 4 files changed, 71 insertions(+)

diff --git a/sys/compat/freebsd32/freebsd32.h b/sys/compat/freebsd32/freebsd32.h
index 8376e95..94f886e 100644
--- a/sys/compat/freebsd32/freebsd32.h
+++ b/sys/compat/freebsd32/freebsd32.h
@@ -362,6 +362,12 @@ struct kinfo_proc32 {
int ki_tdflags;
 };
 
+struct kinfo_sigtramp32 {
+   uint32_t