Re: [patch 4/4 v2] unwinder: The unwinder (x86* only)

[Mark Wielaard <mjw at redhat dot com>]

On Thu, 2013-06-13 at 19:26 +0200, Jan Kratochvil wrote:
> On Thu, 13 Jun 2013 15:38:33 +0200, Mark Wielaard wrote:
> > typedef struct Dwfl_Thread;
>   typedef struct Dwfl_Thread Dwfl_Thread;
> > 
> > typedef struct
> > {
> >   /* Called to iterate through threads.  Starting with previous *ptid
> >      set to -1.  Returns next thread id on success, a negative number on
> >      failure and zero if there are no more threads.  Should call
> >      dwfl_thread_state () on nthread if thread can be unwound with
> >      initial register and memory accessors.  */
> >   pid_t (*next_thread) (Dwfl *dwfl, pid_t pid, pid_t *ptid,
> >                         Dwfl_Thread *nthread, void *arg);
> > 
> >   /* Called by dwfl_end ().  */
> >   void (*detach) (Dwfl *dwfl, pid_t pid, void *arg);
> > 
> > } Dwfl_State_Callbacks;
> 
> Couldn't it be merged together with Dwfl_Thread_Callbacks?

Yes they probably could/should. I imagined you might want to have
totally different thread callbacks per thread, but that is kind of silly
I guess. As long as the callbacks do get the thread identifier it should
be flexible enough.

> This would also remove the dwfl_thread_state function.
> Dwfl_Thread_Callbacks->memory_read is a PID-related function, not TID-related.

Yes, but I imagined a callback could depend on the requested tid (to
make sure it is attached) or have stored memory hunks (stacks) of the
different threads separately, so might like to know for which tid this
memory request is.

> Maybe rather Dwfl_Frame_Callbacks, I no longer remember much why I called it
> dwfl_frame_state and not just dwfl_frame.

I also like Dwfl_Thread_Callbacks (if we have just one callback struct.
But any name will do.

> > /* pid is the process id associated with the Dwfl state, arg is the
> >    callback backend state.  Both will be provided to the callbacks.  */
> > dwfl_attach_state (Dwfl *dwfl, pid_t pid,
> void?

Yes, I cannot imagine any return value making sense, it just sets some
state on the Dwfl, which cannot ever fail. Can it?

> >                    Dwfl_State_Callbacks *state, void *arg);
>                      Dwfl_State_Callbacks *state_callbacks, void *arg);
> 
> > 
> > /* probably needed... */
> > Dwfl *dwfl_thread_dwfl (Dwfl_Thread *thread);
> 
> > pid_t dwfl_thread_pid (Dwfl_Thread *thread);
> 
> PID or TID?  You have declared dwfl_thread_tid below. But if it should be PID
> then it should be:
> pid_t dwfl_pid (Dwfl *dwfl);

ah, yes, it should be dwfl_pid and dwfl_thread_tid.
With dwfl_pid () returning -1 if dwfl_attach_state has not been called.

> > typedef struct
> > {
> >   /* Called on initial unwind to get the initial register state of the
> >      first frame.  Should call dwfl_thread_state_registers (), possibly
> >      multiple times for different ranges, to fill in initial (DWARF)
> >      register values.  After this call, till at least thread_detach is
> >      called, the thread is assumed to be frozen, so that it is safe to
> >      unwind.  Returns true on success or false and sets dwfl_err() on
> >      failure.  */
> >   bool (*set_initial_registers) (Dwfl_Thread *thread, pid_t tid,
> >                                  void *arg);
> 
> I think it should integrate dwfl_thread_state_registers, all the
> dwfl_thread_state_registers parameters should be here as return-references.

I thought you might want to call dwfl_thread_state_registers multiple
times from this callback. See below.

> >   /* Called during unwinding to access memory (stack) state. */
> >   bool (*memory_read) (Dwfl_Thread *thread, pid_t tid,
> >                        Dwarf_Addr addr, Dwarf_Word *result,
> >                        void *arg);
> > 
> >   /* Called when unwinding is done. No callback will be called after
> >      this has been called.  */
> >   void (*thread_detach) (Dwfl_Thread *thread, pid_t tid, void *arg);
> > } Dwfl_Thread_Callbacks;
> > 
> 
> > dwfl_thread_state (Dwfl_Thread *thread, Dwfl_Thread_Callbacks *state);
> 
> Proposing to be removed above.

Yes if we merge them into one.

> > pid_t dwfl_thread_tid (Dwfl_Thread *thread);
> > 
> 
> > dwfl_thread_state_registers (Dwfl_Thread *thread,
> void?
> >                              const int firstreg,
> >                              unsigned nregs,
> >                              const Dwarf_Word *regs);
> 
> I would delete this function, see above.  wrt its parameters:
> 
> This is apparently derived from dwfl_frame_state_data.  But I do not see what
> is firstreg, where is pc/pc_set and where is regs_set.
> 
> PC_SET is true if PC contains a valid value.  On some archs PC is not present
> in REGS.

ah, I thought it was always defined as a dwarf register number, even if
it was just a "fake" return register. But I might misremember.

>  NREGS is number of valid registers in REGS and REGS_SET.  REGS_SET is
> a bitfield specifying which REGS are valid.

Yes, I should have explained. When you reminded me of the messy powerpc
DWARF register numbering I thought it would be more flexible if you
could call dwfl_thread_state_registers multiple times for valid ranges
of (DWARF) registers. So instead of having a valid registers bitfield
mask you would just call it only for ranges of registers that are valid
(firstregnr..firstregnr+nregs). Then you don't need to have pass around
very large (sparse) arrays.

> > /* Iterate through the frames for a thread.  Returns zero if all frames
> >    have been processed by the callback, returns -1 on error, or the
> >    value of the callback when not DWARF_CB_OK.  Keeps calling the
> >    callback with the next frame while the callback returns
> >    DWARF_CB_OK, till there are no more frames.  On start will call the
> >    set_initial_registers callback and on return will call the
> >    detach_thread callback of the Dwfl_Thread.  */
> > int dwfl_getdwarf (Dwfl_Thread *thread,
>       dwfl_getframes?
> I do not understand the name "getdwarf".

Grin, I also don't understand it. I thought I had called it
dwfl_thread_getframes.

> > 		   int (*callback) (Dwfl_Frame *, void *),
>                                     Dwfl_Frame_State *
> Although maybe we could rename it to Dwfl_Frame anyway.

I like the shorter name (although it was a typo).

Cheers,

Mark