Fix signal trampoline detection/unwinding on recent FreeBSD/i386 and FreeBSD/amd64

  This patch fixes two issues with signal frame unwinding on recent FreeBSD/x86:

First, FreeBSD moved the signal trampoline code into a global shared page
exported by the kernel a few releases ago.  To try to make this easier to
detect, FreeBSD added a new per-process sysctl node that exports the starting
and ending address of the signal trampoline code.  This sysctl works on all
platforms that FreeBSD supports no matter where the signal trampoline lives,
but I have only updated i386 and amd64 in this patch.

Second, amd64fbsd_sigcontext_addr was using frame_unwind_register_unsigned to
fetch the stack pointer which resulted in infinite recursion.  I've changed it
to use get_frame_register() to match the sigcontext_addr methods in the
i386-bsd and amd64-linux targets instead.

Changelog:

2015-xx-xx  John Baldwin  <jhb@FreeBSD.org>

	* amd64fbsd-nat.c (_initialize_amd64fbsd_nat): Use the KERN_PROC_SIGTRAMP
	  sysctl to locate the signal trampoline.
	* i386fbsd-nat.c (_initialize_i386fbsd_nat): Likewise.
	* amd64fbsd-tdep.c (amd64fbsd_sigcontext_addr): Fix infinite recursion.
  

On Wednesday, February 04, 2015 10:47:07 AM John Baldwin wrote:
> This patch fixes two issues with signal frame unwinding on recent
> FreeBSD/x86:
> 
> First, FreeBSD moved the signal trampoline code into a global shared page
> exported by the kernel a few releases ago.  To try to make this easier to
> detect, FreeBSD added a new per-process sysctl node that exports the
> starting and ending address of the signal trampoline code.  This sysctl
> works on all platforms that FreeBSD supports no matter where the signal
> trampoline lives, but I have only updated i386 and amd64 in this patch.
> 
> Second, amd64fbsd_sigcontext_addr was using frame_unwind_register_unsigned
> to fetch the stack pointer which resulted in infinite recursion.  I've
> changed it to use get_frame_register() to match the sigcontext_addr methods
> in the i386-bsd and amd64-linux targets instead.

Does anyone have any comments on this patch or is there anything I need to fix 
in it?

> Changelog:
> 
> 2015-xx-xx  John Baldwin  <jhb@FreeBSD.org>
> 
> 	* amd64fbsd-nat.c (_initialize_amd64fbsd_nat): Use the KERN_PROC_SIGTRAMP
> 	  sysctl to locate the signal trampoline.
> 	* i386fbsd-nat.c (_initialize_i386fbsd_nat): Likewise.
> 	* amd64fbsd-tdep.c (amd64fbsd_sigcontext_addr): Fix infinite recursion.
> 
> diff --git a/gdb/amd64fbsd-nat.c b/gdb/amd64fbsd-nat.c
> index 1c396e2..6227681 100644
> --- a/gdb/amd64fbsd-nat.c
> +++ b/gdb/amd64fbsd-nat.c
> @@ -26,6 +26,7 @@
>  #include <sys/types.h>
>  #include <sys/ptrace.h>
>  #include <sys/sysctl.h>
> +#include <sys/user.h>
>  #include <machine/reg.h>
> 
>  #include "fbsd-nat.h"
> @@ -244,6 +245,29 @@ Please report this to <bug-gdb@gnu.org>."),
> 
>    SC_RBP_OFFSET = offset;
> 
> +#ifdef KERN_PROC_SIGTRAMP
> +  /* Newer versions of FreeBSD provide a kern.proc.sigtramp.<pid>
> +     sysctl that returns the location of the signal trampoline.
> +     Note that this fetches the address for the current (gdb) process.
> +     This will be correct for other 64-bit processes, but the signal
> +     trampoline location is not properly set for 32-bit processes. */
> +  {
> +    int mib[4];
> +    struct kinfo_sigtramp kst;
> +    size_t len;
> +
> +    mib[0] = CTL_KERN;
> +    mib[1] = KERN_PROC;
> +    mib[2] = KERN_PROC_SIGTRAMP;
> +    mib[3] = getpid();
> +    len = sizeof (kst);
> +    if (sysctl (mib, 4, &kst, &len, NULL, 0) == 0)
> +      {
> +	amd64fbsd_sigtramp_start_addr = (uintptr_t)kst.ksigtramp_start;
> +	amd64fbsd_sigtramp_end_addr = (uintptr_t)kst.ksigtramp_end;
> +      }
> +  }
> +#else
>    /* FreeBSD provides a kern.ps_strings 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
> @@ -264,4 +288,5 @@ Please report this to <bug-gdb@gnu.org>."),
>  	amd64fbsd_sigtramp_end_addr = ps_strings;
>        }
>    }
> +#endif
>  }
> diff --git a/gdb/amd64fbsd-tdep.c b/gdb/amd64fbsd-tdep.c
> index 2d49cdf..abb0cab 100644
> --- a/gdb/amd64fbsd-tdep.c
> +++ b/gdb/amd64fbsd-tdep.c
> @@ -37,12 +37,16 @@
>  static CORE_ADDR
>  amd64fbsd_sigcontext_addr (struct frame_info *this_frame)
>  {
> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>    CORE_ADDR sp;
> +  gdb_byte buf[8];
> 
>    /* The `struct sigcontext' (which really is an `ucontext_t' on
>       FreeBSD/amd64) lives at a fixed offset in the signal frame.  See
>       <machine/sigframe.h>.  */
> -  sp = frame_unwind_register_unsigned (this_frame, AMD64_RSP_REGNUM);
> +  get_frame_register (this_frame, AMD64_RSP_REGNUM, buf);
> +  sp = extract_unsigned_integer (buf, 8, byte_order);
>    return sp + 16;
>  }
>  
> diff --git a/gdb/i386fbsd-nat.c b/gdb/i386fbsd-nat.c
> index f4951d1..5293f0f 100644
> --- a/gdb/i386fbsd-nat.c
> +++ b/gdb/i386fbsd-nat.c
> @@ -25,6 +25,7 @@
>  #include <sys/types.h>
>  #include <sys/ptrace.h>
>  #include <sys/sysctl.h>
> +#include <sys/user.h>
> 
>  #include "fbsd-nat.h"
>  #include "i386-tdep.h"
> @@ -148,13 +149,34 @@ _initialize_i386fbsd_nat (void)
>    /* Support debugging kernel virtual memory images.  */
>    bsd_kvm_add_target (i386fbsd_supply_pcb);
> 
> +#ifdef KERN_PROC_SIGTRAMP
> +  /* Newer versions of FreeBSD provide a kern.proc.sigtramp.<pid>
> +     sysctl that returns the location of the signal trampoline.
> +     Note that this fetches the address for the current (gdb) process,
> +     but should be correct for other processes. */
> +  {
> +    int mib[4];
> +    struct kinfo_sigtramp kst;
> +    size_t len;
> +
> +    mib[0] = CTL_KERN;
> +    mib[1] = KERN_PROC;
> +    mib[2] = KERN_PROC_SIGTRAMP;
> +    mib[3] = getpid();
> +    len = sizeof (kst);
> +    if (sysctl (mib, 4, &kst, &len, NULL, 0) == 0)
> +      {
> +	i386fbsd_sigtramp_start_addr = (uintptr_t)kst.ksigtramp_start;
> +	i386fbsd_sigtramp_end_addr = (uintptr_t)kst.ksigtramp_end;
> +      }
> +  }
> +#elif defined(KERN_PS_STRINGS)
>    /* FreeBSD provides a kern.ps_strings 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.  */
> -#ifdef KERN_PS_STRINGS
>    {
>      int mib[2];
>      u_long ps_strings;
  

On 02/10/2015 02:50 PM, John Baldwin wrote:
> On Wednesday, February 04, 2015 10:47:07 AM John Baldwin wrote:
>> This patch fixes two issues with signal frame unwinding on recent
>> FreeBSD/x86:
>>
>> First, FreeBSD moved the signal trampoline code into a global shared page
>> exported by the kernel a few releases ago.  To try to make this easier to
>> detect, FreeBSD added a new per-process sysctl node that exports the
>> starting and ending address of the signal trampoline code.  This sysctl
>> works on all platforms that FreeBSD supports no matter where the signal
>> trampoline lives, but I have only updated i386 and amd64 in this patch.
>>
>> Second, amd64fbsd_sigcontext_addr was using frame_unwind_register_unsigned
>> to fetch the stack pointer which resulted in infinite recursion.  I've
>> changed it to use get_frame_register() to match the sigcontext_addr methods
>> in the i386-bsd and amd64-linux targets instead.
> 
> Does anyone have any comments on this patch or is there anything I need to fix 
> in it?
> 
>> Changelog:
>>
>> 2015-xx-xx  John Baldwin  <jhb@FreeBSD.org>
>>
>> 	* amd64fbsd-nat.c (_initialize_amd64fbsd_nat): Use the KERN_PROC_SIGTRAMP
>> 	  sysctl to locate the signal trampoline.

"sysctl" should be aligned to the tab as well (under the star).

>> 	* i386fbsd-nat.c (_initialize_i386fbsd_nat): Likewise.
>> 	* amd64fbsd-tdep.c (amd64fbsd_sigcontext_addr): Fix infinite recursion.

"Fix infinite recursion." is what goes in the commit log.  Here say:

	* amd64fbsd-tdep.c (amd64fbsd_sigcontext_addr): Use get_frame_register.

>>
>> diff --git a/gdb/amd64fbsd-nat.c b/gdb/amd64fbsd-nat.c
>> index 1c396e2..6227681 100644
>> --- a/gdb/amd64fbsd-nat.c
>> +++ b/gdb/amd64fbsd-nat.c
>> @@ -26,6 +26,7 @@
>>  #include <sys/types.h>
>>  #include <sys/ptrace.h>
>>  #include <sys/sysctl.h>
>> +#include <sys/user.h>

Mention this new inclusion in the CL:

	* amd64fbsd-nat.c: Include sys/user.h.
	(_initialize_amd64fbsd_nat): Use the KERN_PROC_SIGTRAMP
	sysctl to locate the signal trampoline.


>>  #include <machine/reg.h>
>>
>>  #include "fbsd-nat.h"
>> @@ -244,6 +245,29 @@ Please report this to <bug-gdb@gnu.org>."),
>>
>>    SC_RBP_OFFSET = offset;
>>
>> +#ifdef KERN_PROC_SIGTRAMP
>> +  /* Newer versions of FreeBSD provide a kern.proc.sigtramp.<pid>

"Newer" will get old soon.  Can you replace that with some kind of
version number/name?

>> +     sysctl that returns the location of the signal trampoline.
>> +     Note that this fetches the address for the current (gdb) process.
>> +     This will be correct for other 64-bit processes, but the signal
>> +     trampoline location is not properly set for 32-bit processes. */

Double-space after periods:  "processes.  */"

I'm not sure I understand what does "but the signal trampoline
location is not properly set for 32-bit processes" means.  You mean
it's not properly set because GDB is 64-bit; or it's not properly set
in the kernel; or something else?

>> +  {
>> +    int mib[4];
>> +    struct kinfo_sigtramp kst;
>> +    size_t len;
>> +
>> +    mib[0] = CTL_KERN;
>> +    mib[1] = KERN_PROC;
>> +    mib[2] = KERN_PROC_SIGTRAMP;
>> +    mib[3] = getpid();

Space before parens:

   mib[3] = getpid ();


>> +    len = sizeof (kst);
>> +    if (sysctl (mib, 4, &kst, &len, NULL, 0) == 0)
>> +      {
>> +	amd64fbsd_sigtramp_start_addr = (uintptr_t)kst.ksigtramp_start;
>> +	amd64fbsd_sigtramp_end_addr = (uintptr_t)kst.ksigtramp_end;

In casts too:

	amd64fbsd_sigtramp_start_addr = (uintptr_t) kst.ksigtramp_start;
	amd64fbsd_sigtramp_end_addr = (uintptr_t) kst.ksigtramp_end;

>> +      }
>> +  }
>> +#else

Did you consider making GDB fallback to the #else block at runtime, for the
case GDB that was built against newer FreeBSD headers but is actually
running on older FreeBSD ?

>>    /* FreeBSD provides a kern.ps_strings 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
>> @@ -264,4 +288,5 @@ Please report this to <bug-gdb@gnu.org>."),
>>  	amd64fbsd_sigtramp_end_addr = ps_strings;
>>        }
>>    }
>> +#endif
>>  }

Otherwise looks good to me too.

Thanks,
Pedro Alves