Re: [patch v6 1/5] x86* unwinder: libebl/

[Mark Wielaard <mjw at redhat dot com>]

On Fri, 2013-10-11 at 22:46 +0200, Jan Kratochvil wrote:
> libebl/
> 2013-06-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 	    Mark Wielaard  <mjw@redhat.com>
> 
> 	* Makefile.am (gen_SOURCES): Add eblinitreg.c.
> 	* ebl-hooks.h (set_initial_registers_tid): New entry.
> 	* eblinitreg.c: New file.
> 	* libebl.h (ebl_tid_registers_t): New definition.
> 	(ebl_set_initial_registers_tid, ebl_frame_nregs): New declarations.
> 	* libeblP.h (struct ebl): New entry frame_nregs.

I think this looks OK. Just some small comments about the comments.

> +/* Fetch process data from live TID into THREAD->unwound.  */
> +bool EBLHOOK(set_initial_registers_tid) (pid_t tid,
> +					 ebl_tid_registers_t *setfunc,
> +					 void *arg);

Maybe s/into THREAD->unwound/and call SETFUNC one or more times/ ?
And add something like: "Should only be called when EBL_FRAME_NREGS > 0,
otherwise the backend doesn't support getting the initial registers."

> +/* Callback to fetch process data from live TID.  */
> +typedef bool (ebl_tid_registers_t) (const int firstreg,
> +				    unsigned nregs,
> +				    const Dwarf_Word *regs,
> +				    void *arg)
> +  __nonnull_attribute__ (3);
> +
> +extern bool ebl_set_initial_registers_tid (Ebl *ebl,
> +					   pid_t tid,
> +					   ebl_tid_registers_t *setfunc,
> +					   void *arg)
> +  __nonnull_attribute__ (1, 3);

Actually, the above comment should be added here in libebl.h.

> +/* Number of registers to allocate
> +   for STATE of ebl_set_initial_registers_tid.  */
> +extern size_t ebl_frame_nregs (Ebl *ebl)
> +  __nonnull_attribute__ (1);

This comment should mention the relationship with the
ebl_set_initial_registers_tid callback function. If zero, this backend
doesn't support fetching the initial registers of a tid. Otherwise it
returns the largest number of DWARF frame registers provided to the
callback. (Or something similar, I am not sure I worded that right for
the none-x86 case.)

> +  /* Number of Dwfl_Frame->regs entries to allocate.  */
> +  size_t frame_nregs;

If this could be reworded without referring to Dwfl_Frame, that would be
ideal.

But all the above is just about the comments, I think the actual code is
good to go now.

Cheers,

Mark