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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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?
This would also remove the dwfl_thread_state function.
Dwfl_Thread_Callbacks->memory_read is a PID-related function, not TID-related.



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


> 
> /* 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?
>                    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);


> 
> 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.


> 
>   /* 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.


> 
> 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. NREGS is number of valid registers in REGS and REGS_SET.  REGS_SET is
a bitfield specifying which REGS are valid.

extern Dwfl_Frame_State *dwfl_frame_state_data (Dwfl *dwfl, bool pc_set,
                                                Dwarf_Addr pc, unsigned nregs,
                                                const uint64_t *regs_set,
                                                const Dwarf_Addr *regs,
                                                dwfl_frame_memory_read_t
                                                                   *memory_read,
                                                void *memory_read_user_data);



> Then (the existing?) dwfl_linux_proc_report and dwfl_core_file_report
> could setup the above callbacks. Or they could be setup by someone by
> hand if they can provide registers and memory accessors in some other
> way.

Great idea.


> 
> For the user/usage side we could then have:
> 
> /* Gets the next known thread, if any.  To get the initial thread
>    provide NULL as previous thread.  */
> Dwfl_Thread *dwfl_next_thread (Dwfl *dwfl, Dwfl_Thread *pthread);
> 
> /* 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".

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

> 		   void *arg);
> 
> bool dwfl_frame_state_pc (Dwfl_Frame_State *state, Dwarf_Addr *pc,
> 		          bool *isactivation)


Thanks,
Jan