On Fri, 2013-10-11 at 22:46 +0200, Jan Kratochvil wrote:
> libebl/
> 2013-06-23  Jan Kratochvil  <[email protected]>
>           Mark Wielaard  <[email protected]>
> 
>       * 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

Reply via email to