Re: [PATCH] libdwfl: Add dwfl_getthread and dwfl_getthread_frames.

[Mark Wielaard <mjw at redhat dot com>]

On Sat, 2013-12-21 at 22:33 +0100, Jan Kratochvil wrote:
> On Fri, 20 Dec 2013 15:23:54 +0100, Mark Wielaard wrote:
> > While playing with the new unwinder interfaces I noticed that I was
> > writing the same extra wrapper callbacks every time.
> 
> Glad you agree the callbacks without lambda functions are a pain. :-)

:) They could be more useful with some syntactic helpers yes.
Or... with some utility functions like in this case.

> [...]
> > --- a/libdwfl/libdwfl.h
> > +++ b/libdwfl/libdwfl.h
> > @@ -662,6 +662,17 @@ typedef struct
> >    pid_t (*next_thread) (Dwfl *dwfl, void *dwfl_arg, void **thread_argp)
> >      __nonnull_attribute__ (1);
> >  
> > +  /* Called to get a specific thread.  Returns true if there is a
> > +     thread with the given thread id number, returns false if no such
> > +     thread exists and may set dwfl_errno in that case.  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 may be NULL and will then be
> > +     emulated using the next_thread callback. */
> > +  bool (*get_thread) (Dwfl *dwfl, pid_t tid, void *dwfl_arg,
> > +		      void **thread_argp)
> > +    __nonnull_attribute__ (1);
> 
> I find this backend extension is not needed.  One could just add to
> 	Dwfl_Thread_Callbacks.next_thread
> comment/requirement
> 	Thread group leader (the thread with TID == PID) is always reported
> 	first (current Linux kernel always keeps it existing at least as
> 	a zombie).

Yes, the new callback is not strictly needed, the implementation allows
it to be NULL (and then does a fallback search for the tid through the
existing next_thread callback mechanism). But it might provide a more
efficient way to access a specific tid of a process. I am not sure how
it would help to return the thread group leader first. That would only
help for TID == PID, but not in any other case.

> > +/* Like dwfl_getthreads, but only for one specific thread with the
> > +   given thead id number if it exists.  Returns zero on success,
> > +   returns -1 on error (and when no thread with the given thread id
> > +   number exists), or the value of the callback when not DWARF_CB_OK.
> > +   -1 returned on error will set dwfl_errno ().  */
> > +int dwfl_getthread (Dwfl *dwfl, pid_t tid,
> > +		    int (*callback) (Dwfl_Thread *thread, void *arg),
> > +		    void *arg)
> > +  __nonnull_attribute__ (1, 3);
> 
> I am not sure even this function needs to be exported as public.
> But this all belongs to your "callbacked" API.

No, both functions are not strictly necessary. But while writing code
using the current interface I noticed I always write some form of
dwfl_getthread to inspect a specific thread and/or dwfl_getthread_frames
to inspect the frames of a specific thread. That is why I added them to
the public API, so people don't have to write them themselves (and
because our implementation can optimize for that specific case that
seems to happen a lot).

> > +/* Like dwfl_thread_getframes, but specifying the thread by its unique
> > +   identifier number.  Returns zero if all frames have been processed
> > +   by the callback, returns -1 on error (and when no thread with
> > +   the given thread id number exists), or the value of the callback
> > +   when not DWARF_CB_OK.  -1 returned on error will set dwfl_errno ().  */
> > +int dwfl_getthread_frames (Dwfl *dwfl, pid_t tid,
> 
> I do not think TID is needed here.  When one wants to backtrace single TID one
> attaches to just that TID.  With the proposed
> Dwfl_Thread_Callbacks.next_thread change to report PID as first TID we just
> need to prevent calling Dwfl_Thread_Callbacks.next_thread for the second time.

So you propose to associate a new Dwfl or detach and attach a Dwfl for
each thread in a process? That seems somewhat counter intuitive to me.
And it seems unnecessary extra work on the user.

One use case I have is attaching to a process/Dwfl, calling
dwfl_getthreads on it to inspect the threads that are there, then for
one (or more) threads that are interesting (for example they were
waiting in the kernel) you want to see if they are still waiting there
and/or maybe inspect the frames of those particular tids. That seems
more natural with both an interface that allows you to walk all threads
and an interface to inspect one specific thread without having to change
or detach the Dwfl.

> > +/* Just checks that the thread id status file (currently) exists. */
> > +static bool
> > +pid_getthread (Dwfl *dwfl __attribute__ ((unused)), pid_t tid,
> > +	       void *dwfl_arg, void **thread_argp)
> > +{
> > +  char fbuf[36];
> > +  snprintf (fbuf, sizeof (fbuf), "/proc/%ld/status", (long) tid);
> > +  *thread_argp = dwfl_arg;
> > +  return access (fbuf, R_OK) == 0;
> 
> It should be enough to do:
> 	return kill (tid, 0) == 0;

Yes, that is easier, thanks.

> > @@ -260,6 +271,7 @@ pid_thread_detach (Dwfl_Thread *thread, void *thread_arg)
> >  static const Dwfl_Thread_Callbacks pid_thread_callbacks =
> >  {
> >    pid_next_thread,
> > +  pid_getthread,
> 
> It should have been called pid_get_thread as the method is called
> (*get_thread).
> 
> I see I missed some underscore inconsistencies in the current API, I would
> call it
> 	dwfl_getthreads -> dwfl_get_threads
> 	dwfl_thread_getframes -> dwfl_thread_get_frames
> 	dwfl_getthread_frames -> dwfl_get_thread_frames
> I can post a patch for it if you agree.

I think it is more consistent with the rest of the libdwfl interfaces to
not have an underscore after _get. See dwfl_getmodules,
dwfl_module_getelf, dwfl_module_getsymtab, etc.

Thanks,

Mark