Re: [patch v7 3/5] x86* unwinder: libdwfl/

[Mark Wielaard <mjw at redhat dot com>]

On Tue, 2013-10-22 at 22:32 +0200, Jan Kratochvil wrote:
> On Mon, 21 Oct 2013 21:19:40 +0200, Jan Kratochvil wrote:
> > On Sat, 19 Oct 2013 03:05:23 +0200, Mark Wielaard wrote:
> > > Again mostly nitpicks. The only real issue I would like you to
> > > think/comment on is around the dwfl_thread_next comments.
> > 
> > Maybe you want threads to be iterated through callbacks?
> 
> Implemented it in the attached diff and checked into jankratochvil/unwindx86.

I like it. It seems to simplify the code.

> @@ -260,47 +246,50 @@ dwfl_frame_thread (Dwfl_Frame *state)
>  }
>  INTDEF(dwfl_frame_thread)
>  
> -Dwfl_Thread *
> -dwfl_next_thread (Dwfl *dwfl, Dwfl_Thread *prev_thread)
> +int
> +dwfl_getthreads (Dwfl *dwfl, int (*callback) (Dwfl_Thread *thread, void *arg),
> +		 void *arg)
> [...]
> +      nthread->tid = process->callbacks->next_thread (dwfl, nthread,
> +						      process->callbacks_arg,
> +						      &nthread->callbacks_arg);
> +      if (nthread->tid < 0)
> +	{
> +	  Dwfl_Error saved_errno = dwfl_errno ();
> +	  thread_free (nthread);
> +	  __libdwfl_seterrno (saved_errno);
> +	  return -1;
> +	}

thread_free doesn't set dwfl_errno, so should be save to call without
saving/restoring it.

I think you can now also simplify the Dwfl_Thread_Callbacks next_thread.
It doesn't really seem to need the Dwfl_Thread that is being constructed
since it doesn't seem contain anything that helps with constructing the
next one. So I would change it to something like:

  /* Called to iterate through threads.  Returns next TID (thread ID) on
     success, a negative number on failure and zero if there are no more
     threads.  dwfl_errno () should be set if negative number has been
     returned.  *THREAD_ARGP will be NULL on the first call and may be
     optionally set by the implementation.  On next calls **THREAD_ARGP
     will point to the *THREAD_ARGP value set last time.  THREAD_ARGP is
     never NULL.  *THREAD_ARGP will be passed to set_initial_registers
     or thread_detach callbacks together with Dwfl_Thread *thread.  
     This method must not be NULL.  */
  pid_t (*next_thread) (Dwfl *dwfl, void *dwfl_arg, void **thread_argp)
    __nonnull_attribute__ (1, 2);

That way the callback can keep any state it wants between next_thread
calls through *THREAD_ARGP if it wants.

Does that make sense?

Cheers,

Mark