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

[Mark Wielaard <mjw at redhat dot com>]

On Sat, 2013-06-15 at 18:27 +0200, Jan Kratochvil wrote:
> On Thu, 13 Jun 2013 22:47:10 +0200, Mark Wielaard wrote:
> > On Thu, 2013-06-13 at 19:26 +0200, Jan Kratochvil wrote:
> > > > /* 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?
> 
> What if one does two dwfl_attach_state to the same DWFL?  Maybe the second one
> will override the first one.  Although it could rather fail and we would also
> need to provide dwfl_detach_state.  But for now I put there 'void'.

Good point. Maybe in the future we want to support "reattaching" (maybe
switching from fully detached state to fully attached/frozen state).
Even if we don't support that now. Lets add a return value just in case,
even if for now we always succeed when not yet set and always fail when
trying to reset.

> > > 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.
> 
> ppc has LR register which is somehow both 108 and 65.
> 	http://en.wikipedia.org/wiki/Link_register
> 
> But this is not PC, PC IMO does not have any DWARF register.  DWARF never
> needs to address PC itself, DWARF only needs to address "return address" and
> that is LR.  But during the "activation" we need to set both PC and LR, those
> are two different registers.
> 
> Maybe one of 65 and 108 should have been LR and the other one PC.  But in
> reality there are some bugs and both 65 and 108 are used interchangeable for
> LR, therefore I aliased 108 to 65 (ppc_frame_dwarf_to_regno).

ah, forgot about PPC again (or link registers in general). I was
thinking of x86 where the Return Address is a "fake" DWARF register, but
can in practice be used as the actual IP register. You are right, we
need to handle PC explicitly.

> I have removed some duplicities - like if DWFL is passed then we do not need
> to also pass PID, because we have pid_t dwfl_pid (Dwfl *dwfl);

Yeah, that was more "convenience", but clutters the interface.

> typedef struct
> {
>   /* Called to iterate through threads.  Returns next TID (tthread ID) on
>      success, a negative number on failure and zero if there are no more
>      threads.  For successful results the implementation must call
>      dwfl_thread_state on NTHREAD.  This method must not be NULL.  */
>   pid_t (*next_thread) (Dwfl *dwfl, Dwfl_Thread *nthread, void *arg);

I think this should get Dwfl_Thread *pthread also as argument,
indicating the previous thread (NULL on first call).

>   /* Called by dwfl_end.  All thread_detach method calls have been already
>      done.  This method may be NULL.  */
>   void (*detach) (Dwfl *dwfl, void *arg);
> 
>   /* Called during unwinding to access memory (stack) state.  Returns true for
>      successfully read *RESULT or false and sets dwfl_err () on failure.  This
>      method must not be NULL.  */
>   bool (*memory_read) (Dwfl *dwfl, Dwarf_Addr addr, Dwarf_Word *result,
>                        void *arg);

If NULL, then unwinding is not supported. Which might not be fatal,
someone could just be interested in the initial state/PC of the thread
and don't care if they cannot unwind (fully).

>   /* 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 and possibly also dwfl_thread_state_register_pc, 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.  This method must not be NULL.  */ 
>   bool (*set_initial_registers) (Dwfl_Thread *thread, void *arg);
> 
>   /* Called when unwinding is done.  No callback will be called after
>      this method has been called.  Iff set_initial_registers was called for
>      a TID thread_detach will be called before the detach method above.
>      This method may be NULL.  */
>   void (*thread_detach) (Dwfl_Thread *thread, void *arg);
> } Dwfl_Thread_Callbacks;
> 
> /* PID is the process id associated with the DWFL state, ARG is the
>    callback backend state.  ARG will be provided to the callbacks.  */
> void dwfl_attach_state (Dwfl *dwfl, pid_t pid,
>                         Dwfl_Thread_Callbacks *thread_callbacks, void *arg);

Lets do add a return value to indicate success or failure as discussed
above.

> /* Return PID for the process associated with DWFL.  Function returns -1 if
>    dwfl_attach_state was not called for DWFL.  */
> pid_t dwfl_pid (Dwfl *dwfl);
> 
> /* Return DWFL from which THREAD was created using dwfl_next_thread.  */
> Dwfl *dwfl_thread_dwfl (Dwfl_Thread *thread);
> 
> /* Return positive TID (thread ID) for THREAD.  This function never fails.  */
> pid_t dwfl_thread_tid (Dwfl_Thread *thread);
> 
> /* Called by Dwfl_Thread_Callbacks.set_initial_registers implementation.
>    For every known continuous block of registers <FIRSTREG..FIRSTREG+NREGS)
>    (inclusive..exclusive) set their content to REGS (array of NREGS items).  */
> void dwfl_thread_state_registers (Dwfl_Thread *thread, const int firstreg,
>                                   unsigned nregs, const Dwarf_Word *regs);
> 
> /* Called by Dwfl_Thread_Callbacks.set_initial_registers implementation.
>    If PC is not contained among DWARF registers passed by
>    dwfl_thread_state_registers on the target architecture pass the PC value
>    here.  */
> void dwfl_thread_state_register_pc (Dwfl_Thread *thread, Dwarf_Word pc);
> 
> /* Gets the next known thread, if any.  To get the initial thread
>    provide NULL as previous thread PREV_THREAD.  */
> Dwfl_Thread *dwfl_next_thread (Dwfl *dwfl, Dwfl_Thread *prev_thread);
> 
> /* 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_thread_getframes (Dwfl_Thread *thread,
>                            int (*callback) (Dwfl_Frame *, void *));

Needs an extra (possibly NULL) void *arg argument that will be passed to
the callback.

> /* Return *PC (program counter) for thread-specific frame STATE.
>    Set *ISACTIVATION according to DWARF frame "activation" definition.
>    Typically you need to substract 1 from *PC if *ACTIVATION is false to safely
>    find function of the caller.  ACTIVATION may be NULL.  PC must not be NULL.
>    Function returns false if it failed to find *PC.  */
> bool Dwfl_Frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool *isactivation);

lower case dwfl_frame_pc I presume.

Thanks,

Mark