This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [patch v6 2/5] x86* unwinder: backends/
- From: Mark Wielaard <mjw at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Thu, 17 Oct 2013 15:02:41 +0200
- Subject: Re: [patch v6 2/5] x86* unwinder: backends/
On Fri, 2013-10-11 at 22:47 +0200, Jan Kratochvil wrote:
> backends/
> 2013-06-23 Jan Kratochvil <jan.kratochvil@redhat.com>
> Mark Wielaard <mjw@redhat.com>
>
> * Makefile.am (i386_SRCS): Add i386_initreg.c.
> (x86_64_SRCS): Add x86_64_initreg.c.
> * i386_initreg.c: New file.
> * i386_init.c (i386_init): Initialize frame_nregs and
> set_initial_registers_tid.
> * x86_64_initreg.c: New file.
> * x86_64_init.c (x86_64_init): Initialize frame_nregs and
> set_initial_registers_tid.
Looks ok, just some small (comment) nitpicks below.
> --- a/backends/i386_init.c
> +++ b/backends/i386_init.c
> + /* gcc/config/ #define DWARF_FRAME_REGISTERS. For i386 it is 17, why? */
> + eh->frame_nregs = 9;
> + HOOK (eh, set_initial_registers_tid);
9 looks like the right number.
Did you figure out why gcc defines it differently?
It looks like confusing with the full x86_64 register, which obviously
has 8 more general purpose registers, that aren't available to i386
processes.
> +++ b/backends/i386_initreg.c
> @@ -0,0 +1,80 @@
> +/* Fetch process data from STATE->base->pid or STATE->base->core.
Comment doesn't really describe what the file does.
> +# else /* (__i386__ || __x86_64__) && (!__i386__ && !__x86_64__) */
> +# error
Please make that error say something:
#error Impossible, unreachable, arch should be i386 or x86_64.
> +# endif /* (__i386__ || __x86_64__) && (!__i386__ && !__x86_64__) */
> + return setfunc (0, 9, dwarf_regs, arg);
> +#endif /* __i386__ || __x86_64__ */
> + return true;
> +}
That last return statement also isn't reachable (there is a return false
at the top, if that isn't reached then the return setfunc will be
reached. So that should be either return false, an unreachable assert or
just left out.
> +++ b/backends/x86_64_initreg.c
> @@ -0,0 +1,73 @@
> +/* Fetch live process Dwfl_Frame from PID.
It isn't using Dwfl_Frame anymore.