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
