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

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

On Mon, 23 Dec 2013 13:54:19 +0100, Mark Wielaard wrote:
> On Sun, 2013-12-22 at 16:51 +0100, Jan Kratochvil wrote:
> > 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.
> 
> This just shows a bug in linux-pid-attach. It doesn't properly report
> the PID (Tgid). It should only report 12001 as PID if it really is the
> thread group leader of the given TID. I'll fix that.

OK, I agree, then the situation is different.  It looks a bit unusual as it is
not the behavior of GDB.


> Updated patch attached.

Fine with that with the comments below.


[...]
> --- a/libdwfl/dwfl_frame.c
> +++ b/libdwfl/dwfl_frame.c
> @@ -273,6 +273,93 @@ dwfl_getthreads (Dwfl *dwfl, int (*callback) (Dwfl_Thread *thread, void *arg),
>  }
>  INTDEF(dwfl_getthreads)
>  
> +struct one_arg
> +{
> +  pid_t tid;
> +  bool seen;
> +  int (*callback) (Dwfl_Thread *thread, void *arg);
> +  void *arg;
> +};
> +
> +static int
> +get_one_thread_cb (Dwfl_Thread *thread, void *arg)
> +{
> +  struct one_arg *oa = (struct one_arg *) arg;
> +  if (! oa->seen && INTUSE(dwfl_thread_tid) (thread) == oa->tid)
> +    {
> +      oa->seen = true;
> +      return oa->callback (thread, oa->arg);

Maybe it was intentionall to keep the code simple - but if oa->callback
returns DWARF_CB_OK then it will needlessly continue iterating the threads
despite it can never find that TID again.


> +    }
> +
> +  return DWARF_CB_OK;
> +}
> +
> +/* Note not currently exported, will be when there are more Dwfl_Thread
> +   properties to query.  Use dwfl_getthread_frames for now directly.  */
> +static int
> +dwfl_getthread (Dwfl *dwfl, pid_t tid,

It is no longer public so I think it should not have dwfl_* prefix.


> +		int (*callback) (Dwfl_Thread *thread, void *arg),
> +		void *arg)
> +{
> +  Dwfl_Process *process = dwfl->process;
> +  if (process == NULL)

As the function is no longer public this can be rather an assert.


> +    {
> +      __libdwfl_seterrno (dwfl->process_attach_error);
> +      return -1;
> +    }
> +
> +  if (process->callbacks->get_thread != NULL)
> +    {
> +      Dwfl_Thread thread;
> +      thread.process = process;
> +      thread.unwound = NULL;
> +      thread.callbacks_arg = NULL;
> +
> +      if (process->callbacks->get_thread (dwfl, tid, process->callbacks_arg,
> +					  &thread.callbacks_arg))
> +	{
> +	  int err;
> +	  thread.tid = tid;
> +	  err = callback (&thread, arg);
> +	  thread_free_all_states (&thread);
> +	  return err;
> +	}
> +
> +      return -1;
> +    }
> +
> +   struct one_arg oa = { .tid = tid, .callback = callback,
> +			 .arg = arg, .seen = false };
> +   int err = INTUSE(dwfl_getthreads) (dwfl, get_one_thread_cb, &oa);
> +   if (err == DWARF_CB_OK && ! oa.seen)

__libdwfl_seterrno is not called.

> +     return -1;
> +
> +   return err;
> +}
> +
> +struct one_thread
> +{
> +  int (*callback) (Dwfl_Frame *frame, void *arg);
> +  void *arg;
> +};
> +
> +static int
> +get_one_thread_frames_cb (Dwfl_Thread *thread, void *arg)
> +{
> +  struct one_thread *ot = (struct one_thread *) arg;
> +  return INTUSE(dwfl_thread_getframes) (thread, ot->callback, ot->arg);
> +}
> +
> +int
> +dwfl_getthread_frames (Dwfl *dwfl, pid_t tid,
> +		       int (*callback) (Dwfl_Frame *frame, void *arg),
> +		       void *arg)
> +{
> +  struct one_thread ot = { .callback = callback, .arg = arg };
> +  return dwfl_getthread (dwfl, tid, get_one_thread_frames_cb, &ot);
> +}
> +INTDEF(dwfl_getthread_frames)
> +
>  int
>  dwfl_thread_getframes (Dwfl_Thread *thread,
>  		       int (*callback) (Dwfl_Frame *state, void *arg),
> diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
> index 007089b..f1c0052 100644
> --- 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

s/may/it will/.  Setting errno has to be mandatory, not voluntary.


> +     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);
> +
>    /* Called during unwinding to access memory (stack) state.  Returns true for
>       successfully read *RESULT or false and sets dwfl_errno () on failure.
>       This method may be NULL - in such case dwfl_thread_getframes will return
> @@ -762,6 +773,16 @@ int dwfl_thread_getframes (Dwfl_Thread *thread,
>  			   void *arg)
>    __nonnull_attribute__ (1, 2);
>  
> +/* 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,
> +			   int (*callback) (Dwfl_Frame *thread, void *arg),
> +			   void *arg)
> +  __nonnull_attribute__ (1, 3);
> +
>  /* 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
[...]


Thanks,
Jan