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

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

Hi Mark,

this whole discussion is difficult without seeing how you want to use it.
I am considering only the eu-stack case as you supplied patch for it.


On Sun, 22 Dec 2013 00:51:39 +0100, Mark Wielaard wrote:
> On Sat, 2013-12-21 at 22:33 +0100, Jan Kratochvil wrote:
> > [...]
> > > --- 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.

If I have PID=12000 and I do 'eu-stack -p 12001' (its thread TID) then for
elfutils in fact PID is 12001.  So if Dwfl_Thread_Callbacks.next_thread
returns 12001 first we can backtrace it and stop.


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

The question was whether you have any use case for dwfl_getthread.
eu-stack uses dwfl_getthread_frames() but not dwfl_getthread.
I cannot imagine when to use dwfl_getthread when there is
dwfl_getthread_frames() available.


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

If I want to do something with 'each thread in a process' I am fine with the
current already checked-in API.

I am proposing to associate new Dwfl only with the specific one (possibly
non-leader) thread TID (in such case elfutils will consider that TID as PID
although in reality process group PID is different).


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

In this use case you should be with the the current already checked-in API so
I find your example off-topic for this mail thread.


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

OK.


Thanks,
Jan