[PATCH] libdwfl: Add dwfl_getthread and dwfl_getthread_frames.

Jan Kratochvil jan.kratochvil at redhat.com
Sat Dec 21 21:33:37 UTC 2013


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


[...]
> --- 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).

Current libdwfl/linux-pid-attach.c sure needs an update for it.

It would then also change + simplify dwfl_getthread() implementation.


> +
>    /* 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
> @@ -748,6 +759,16 @@ int dwfl_getthreads (Dwfl *dwfl,
>  		     void *arg)
>    __nonnull_attribute__ (1, 2);
>  
> +/* 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.


> +
>  /* Iterate through the frames for a thread.  Returns zero if all frames
>     have been processed by the callback, returns -1 on error, or the value of
>     the callback when not DWARF_CB_OK.  -1 returned on error will
> @@ -762,6 +783,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,

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.


> +			   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
[...]
> --- a/libdwfl/linux-pid-attach.c
> +++ b/libdwfl/linux-pid-attach.c
> @@ -201,6 +201,17 @@ pid_next_thread (Dwfl *dwfl __attribute__ ((unused)), void *dwfl_arg,
>    return tid;
>  }
>  
> +/* 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;

BTW:
 * Zombie processes/threads return true (valid) in both cases.
 * If a thread group leader (TID==PID) exits it remains a zombie:
   gdb/testsuite/gdb.threads/leader-exit.c


> +}
> +
>  /* Implement the ebl_set_initial_registers_tid setfunc callback.  */
>  
>  static bool
> @@ -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.


>    pid_memory_read,
>    pid_set_initial_registers,
>    pid_detach,
[...]


Thanks,
Jan


More information about the elfutils-devel mailing list