This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [patch v7 3/5] x86* unwinder: libdwfl/
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Tue, 22 Oct 2013 22:32:51 +0200
- Subject: Re: [patch v7 3/5] x86* unwinder: libdwfl/
On Mon, 21 Oct 2013 21:19:40 +0200, Jan Kratochvil wrote:
> On Sat, 19 Oct 2013 03:05:23 +0200, Mark Wielaard wrote:
> > Again mostly nitpicks. The only real issue I would like you to
> > think/comment on is around the dwfl_thread_next comments.
>
> Maybe you want threads to be iterated through callbacks?
Implemented it in the attached diff and checked into jankratochvil/unwindx86.
Thanks,
Jan
diff --git a/libdw/libdw.map b/libdw/libdw.map
index 0130ca0..1777585 100644
--- a/libdw/libdw.map
+++ b/libdw/libdw.map
@@ -266,7 +266,7 @@ ELFUTILS_0.156 {
dwfl_frame_thread;
dwfl_thread_state_registers;
dwfl_thread_state_register_pc;
- dwfl_next_thread;
+ dwfl_getthreads;
dwfl_thread_getframes;
dwfl_frame_pc;
} ELFUTILS_0.149;
diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index 258af8a..7309cc1 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -100,44 +100,30 @@ state_alloc (Dwfl_Thread *thread)
return state;
}
-/* Free and unlink THREAD from the internal lists. PREV_THREAD must be NULL if
- THREAD was the first one or PREV_THREAD must be the preceding thread. */
+/* Free and unlink THREAD from the internal lists. */
static void
-thread_free (Dwfl_Thread *thread, Dwfl_Thread *prev_thread)
+thread_free (Dwfl_Thread *thread)
{
Dwfl_Process *process = thread->process;
- assert (prev_thread == NULL || prev_thread->process == process);
- assert (prev_thread != NULL || process->first_thread == thread);
- assert (prev_thread == NULL || prev_thread->next == thread);
+ assert (process->thread == thread);
while (thread->unwound)
state_free (thread->unwound);
- if (prev_thread == NULL)
- process->first_thread = thread->next;
- else
- prev_thread->next = thread->next;
+ process->thread = NULL;
free (thread);
}
-/* Allocate new Dwfl_Thread and link it to PROCESS. PREV_THREAD must be NULL
- if this is the first thread for PROCESS, otherwise PREV_THREAD must be the
- last thread of PROCESS. */
+/* Allocate new Dwfl_Thread and link it to PROCESS. */
static Dwfl_Thread *
-thread_alloc (Dwfl_Process *process, Dwfl_Thread *prev_thread)
+thread_alloc (Dwfl_Process *process)
{
- assert (prev_thread == NULL || prev_thread->process == process);
- assert (prev_thread != NULL || process->first_thread == NULL);
- assert (prev_thread == NULL || prev_thread->next == NULL);
+ assert (process->thread == NULL);
Dwfl_Thread *thread = malloc (sizeof (*thread));
if (thread == NULL)
return NULL;
thread->process = process;
thread->unwound = NULL;
thread->tid = 0;
- thread->next = NULL;
- if (prev_thread == NULL)
- process->first_thread = thread;
- else
- prev_thread->next = thread;
+ process->thread = thread;
return thread;
}
@@ -148,8 +134,8 @@ __libdwfl_process_free (Dwfl_Process *process)
Dwfl *dwfl = process->dwfl;
if (process->callbacks->detach != NULL)
process->callbacks->detach (dwfl, process->callbacks_arg);
- while (process->first_thread)
- thread_free (process->first_thread, NULL);
+ if (process->thread)
+ thread_free (process->thread);
assert (dwfl->process == process);
dwfl->process = NULL;
if (process->ebl_close)
@@ -165,7 +151,7 @@ process_alloc (Dwfl *dwfl)
if (process == NULL)
return;
process->dwfl = dwfl;
- process->first_thread = NULL;
+ process->thread = NULL;
dwfl->process = process;
}
@@ -260,47 +246,50 @@ dwfl_frame_thread (Dwfl_Frame *state)
}
INTDEF(dwfl_frame_thread)
-Dwfl_Thread *
-dwfl_next_thread (Dwfl *dwfl, Dwfl_Thread *prev_thread)
+int
+dwfl_getthreads (Dwfl *dwfl, int (*callback) (Dwfl_Thread *thread, void *arg),
+ void *arg)
{
- if (prev_thread != NULL && prev_thread->process->dwfl != dwfl)
- {
- __libdwfl_seterrno (DWFL_E_INVALID_ARGUMENT);
- return NULL;
- }
- if (prev_thread != NULL && prev_thread->next != NULL)
- return prev_thread->next;
Dwfl_Process *process = dwfl->process;
if (process == NULL)
{
__libdwfl_seterrno (DWFL_E_NO_ATTACH_STATE);
- return NULL;
+ return -1;
}
- Dwfl_Thread *nthread = thread_alloc (process, prev_thread);
+ Dwfl_Thread *nthread = thread_alloc (process);
if (nthread == NULL)
{
__libdwfl_seterrno (DWFL_E_NOMEM);
- return NULL;
- }
- nthread->tid = process->callbacks->next_thread (dwfl, nthread,
- process->callbacks_arg,
- &nthread->callbacks_arg);
- if (nthread->tid < 0)
- {
- Dwfl_Error saved_errno = dwfl_errno ();
- thread_free (nthread, prev_thread);
- __libdwfl_seterrno (saved_errno);
- return NULL;
+ return -1;
}
- if (nthread->tid == 0)
+ for (;;)
{
- thread_free (nthread, prev_thread);
- __libdwfl_seterrno (DWFL_E_NOERROR);
- return NULL;
+ nthread->tid = process->callbacks->next_thread (dwfl, nthread,
+ process->callbacks_arg,
+ &nthread->callbacks_arg);
+ if (nthread->tid < 0)
+ {
+ Dwfl_Error saved_errno = dwfl_errno ();
+ thread_free (nthread);
+ __libdwfl_seterrno (saved_errno);
+ return -1;
+ }
+ if (nthread->tid == 0)
+ {
+ thread_free (nthread);
+ __libdwfl_seterrno (DWFL_E_NOERROR);
+ return 0;
+ }
+ int err = callback (nthread, arg);
+ if (err != DWARF_CB_OK)
+ {
+ thread_free (nthread);
+ return err;
+ }
}
- return nthread;
+ /* NOTREACHED */
}
-INTDEF(dwfl_next_thread)
+INTDEF(dwfl_getthreads)
int
dwfl_thread_getframes (Dwfl_Thread *thread,
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index 11f22dc..d45420f 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -640,7 +640,7 @@ bool dwfl_attach_state (Dwfl *dwfl, int machine, pid_t pid,
pid_t dwfl_pid (Dwfl *dwfl)
__nonnull_attribute__ (1);
-/* Return DWFL from which THREAD was created using dwfl_next_thread. */
+/* Return DWFL from which THREAD was created using dwfl_getthreads. */
Dwfl *dwfl_thread_dwfl (Dwfl_Thread *thread)
__nonnull_attribute__ (1);
@@ -667,12 +667,15 @@ bool dwfl_thread_state_registers (Dwfl_Thread *thread, const int firstreg,
void dwfl_thread_state_register_pc (Dwfl_Thread *thread, Dwarf_Word pc)
__nonnull_attribute__ (1);
-/* Gets the next known thread, if any. To get the initial thread
- provide NULL as previous thread PREV_THREAD. On error function returns NULL
- and sets dwfl_errno (). When no more threads are found function returns
- NULL and dwfl_errno () is set to 0 - dwfl_errmsg (0) returns NULL then. */
-Dwfl_Thread *dwfl_next_thread (Dwfl *dwfl, Dwfl_Thread *prev_thread)
- __nonnull_attribute__ (1);
+/* Iterate through the threads for a process. Returns zero if all threads 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 set dwfl_errno ().
+ Keeps calling the callback with the next thread while the callback returns
+ DWARF_CB_OK, till there are no more threads. */
+int dwfl_getthreads (Dwfl *dwfl,
+ int (*callback) (Dwfl_Thread *thread, void *arg),
+ void *arg)
+ __nonnull_attribute__ (1, 2);
/* Iterate through the frames for a thread. Returns zero if all frames
have been processed by the callback, returns -1 on error, or the
@@ -686,8 +689,8 @@ Dwfl_Thread *dwfl_next_thread (Dwfl *dwfl, Dwfl_Thread *prev_thread)
callback and on return will call the detach_thread callback of the
Dwfl_Thread. */
int dwfl_thread_getframes (Dwfl_Thread *thread,
- int (*callback) (Dwfl_Frame *state, void *arg),
- void *arg)
+ int (*callback) (Dwfl_Frame *state, void *arg),
+ void *arg)
__nonnull_attribute__ (1, 2);
/* Return *PC (program counter) for thread-specific frame STATE.
diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
index 92d51d7..d554f87 100644
--- a/libdwfl/libdwflP.h
+++ b/libdwfl/libdwflP.h
@@ -218,7 +218,7 @@ struct Dwfl_Process
void *callbacks_arg;
struct ebl *ebl;
bool ebl_close:1;
- Dwfl_Thread *first_thread;
+ Dwfl_Thread *thread;
};
/* See its typedef in libdwfl.h. */
@@ -226,9 +226,7 @@ struct Dwfl_Process
struct Dwfl_Thread
{
Dwfl_Process *process;
- Dwfl_Thread *next;
pid_t tid;
- bool thread_detach_needed : 1;
/* The current frame being unwound. Initially it is the bottom frame.
Later the processed frames get freed and this pointer is updated. */
Dwfl_Frame *unwound;
@@ -700,7 +698,7 @@ INTDECL (dwfl_thread_tid)
INTDECL (dwfl_frame_thread)
INTDECL (dwfl_thread_state_registers)
INTDECL (dwfl_thread_state_register_pc)
-INTDECL (dwfl_next_thread)
+INTDECL (dwfl_getthreads)
INTDECL (dwfl_thread_getframes)
INTDECL (dwfl_frame_pc)
diff --git a/src/stack.c b/src/stack.c
index ed6e791..cea5e5b 100644
--- a/src/stack.c
+++ b/src/stack.c
@@ -85,6 +85,25 @@ frame_callback (Dwfl_Frame *state, void *arg)
return DWARF_CB_OK;
}
+static int
+thread_callback (Dwfl_Thread *thread, void *thread_arg __attribute__ ((unused)))
+{
+ printf ("TID %ld:\n", (long) dwfl_thread_tid (thread));
+ unsigned frameno = 0;
+ switch (dwfl_thread_getframes (thread, frame_callback, &frameno))
+ {
+ case 0:
+ case 1:
+ break;
+ case -1:
+ error (0, 0, "dwfl_thread_getframes: %s", dwfl_errmsg (-1));
+ break;
+ default:
+ abort ();
+ }
+ return DWARF_CB_OK;
+}
+
static void
dump (Dwfl *dwfl, pid_t pid, const char *corefile)
{
@@ -94,30 +113,15 @@ dump (Dwfl *dwfl, pid_t pid, const char *corefile)
report_corefile (dwfl, corefile);
else
abort ();
- Dwfl_Thread *thread = NULL;
- for (;;)
+ switch (dwfl_getthreads (dwfl, thread_callback, NULL))
{
- thread = dwfl_next_thread (dwfl, thread);
- if (thread == NULL)
- {
- const char *msg = dwfl_errmsg (0);
- if (msg == NULL)
- break;
- error (2, 0, "dwfl_next_thread: %s", msg);
- }
- printf ("TID %ld:\n", (long) dwfl_thread_tid (thread));
- unsigned frameno = 0;
- switch (dwfl_thread_getframes (thread, frame_callback, &frameno))
- {
- case 0:
- case 1:
- break;
- case -1:
- error (0, 0, "dwfl_thread_getframes: %s", dwfl_errmsg (-1));
- break;
- default:
- abort ();
- }
+ case 0:
+ break;
+ case -1:
+ error (0, 0, "dwfl_getthreads: %s", dwfl_errmsg (-1));
+ break;
+ default:
+ abort ();
}
dwfl_end (dwfl);
}
diff --git a/tests/backtrace-data.c b/tests/backtrace-data.c
index 4679bb6..1d1020c 100644
--- a/tests/backtrace-data.c
+++ b/tests/backtrace-data.c
@@ -221,6 +221,22 @@ frame_callback (Dwfl_Frame *state, void *arg)
return DWARF_CB_OK;
}
+static int
+thread_callback (Dwfl_Thread *thread, void *thread_arg __attribute__ ((unused)))
+{
+ unsigned frameno = 0;
+ switch (dwfl_thread_getframes (thread, frame_callback, &frameno))
+ {
+ case 0:
+ break;
+ case -1:
+ error (1, 0, "dwfl_thread_getframes: %s", dwfl_errmsg (-1));
+ default:
+ abort ();
+ }
+ return DWARF_CB_OK;
+}
+
int
main (int argc __attribute__ ((unused)), char **argv __attribute__ ((unused)))
{
@@ -276,19 +292,8 @@ main (int argc __attribute__ ((unused)), char **argv __attribute__ ((unused)))
assert (ok);
/* Multiple threads are not handled here. */
- Dwfl_Thread *thread = dwfl_next_thread (dwfl, NULL);
- assert (thread);
-
- unsigned frameno = 0;
- switch (dwfl_thread_getframes (thread, frame_callback, &frameno))
- {
- case 0:
- break;
- case -1:
- error (1, 0, "dwfl_thread_getframes: %s", dwfl_errmsg (-1));
- default:
- abort ();
- }
+ int err = dwfl_getthreads (dwfl, thread_callback, NULL);
+ assert (! err);
dwfl_end (dwfl);
kill (child, SIGKILL);
diff --git a/tests/backtrace.c b/tests/backtrace.c
index 243b51c..8dd7177 100644
--- a/tests/backtrace.c
+++ b/tests/backtrace.c
@@ -154,9 +154,9 @@ struct frame_callback
};
static int
-frame_callback (Dwfl_Frame *state, void *arg)
+frame_callback (Dwfl_Frame *state, void *frame_arg)
{
- struct frame_callback *data = arg;
+ struct frame_callback *data = frame_arg;
Dwarf_Addr pc;
bool isactivation;
if (! dwfl_frame_pc (state, &pc, &isactivation))
@@ -184,6 +184,36 @@ frame_callback (Dwfl_Frame *state, void *arg)
return DWARF_CB_OK;
}
+struct thread_callback
+{
+ callback_t *callback;
+ void *callback_data;
+};
+
+static int
+thread_callback (Dwfl_Thread *thread, void *thread_arg)
+{
+ struct thread_callback *data = thread_arg;
+ printf ("TID %ld:\n", (long) dwfl_thread_tid (thread));
+ struct frame_callback frame_callback_data;
+ frame_callback_data.frameno = 0;
+ frame_callback_data.callback = data->callback;
+ frame_callback_data.callback_data = data->callback_data;
+ switch (dwfl_thread_getframes (thread, frame_callback,
+ &frame_callback_data))
+ {
+ case 0:
+ break;
+ case 1:
+ return 1;
+ case -1:
+ error (0, 0, "dwfl_thread_getframes: %s", dwfl_errmsg (-1));
+ return 1;
+ default:
+ abort ();
+ }
+ return DWARF_CB_OK;
+}
static void
dump (pid_t pid, const char *corefile, callback_t *callback,
@@ -198,40 +228,24 @@ dump (pid_t pid, const char *corefile, callback_t *callback,
abort ();
ptrdiff_t ptrdiff = dwfl_getmodules (dwfl, dump_modules, NULL, 0);
assert (ptrdiff == 0);
- Dwfl_Thread *thread = NULL;
+ struct thread_callback thread_callback_data;
+ thread_callback_data.callback = callback;
+ thread_callback_data.callback_data = callback_data;
int err = 0;
- for (;;)
+ switch (dwfl_getthreads (dwfl, thread_callback, &thread_callback_data))
{
- thread = dwfl_next_thread (dwfl, thread);
- if (thread == NULL)
- {
- const char *msg = dwfl_errmsg (0);
- if (msg == NULL)
- break;
- error (2, 0, "dwfl_next_thread: %s", msg);
- }
- printf ("TID %ld:\n", (long) dwfl_thread_tid (thread));
- struct frame_callback frame_callback_data;
- frame_callback_data.frameno = 0;
- frame_callback_data.callback = callback;
- frame_callback_data.callback_data = callback_data;
- switch (dwfl_thread_getframes (thread, frame_callback,
- &frame_callback_data))
- {
- case 0:
- break;
- case 1:
- err = 1;
- break;
- case -1:
- error (0, 0, "dwfl_thread_getframes: %s", dwfl_errmsg (-1));
- err = 1;
- break;
- default:
- abort ();
- }
+ case 0:
+ break;
+ case 1:
+ err = 1;
+ break;
+ case -1:
+ error (0, 0, "dwfl_getthreads: %s", dwfl_errmsg (-1));
+ err = 1;
+ break;
+ default:
+ abort ();
}
- while (0);
if (callback)
callback (0, 0, 0, NULL, dwfl, callback_data);
dwfl_end (dwfl);