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