Re: [patch v7 3/5] x86* unwinder: libdwfl/

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

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);