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

[Pedro Alves]

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