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

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

On Wed, 23 Oct 2013 14:18:10 +0200, Mark Wielaard wrote:
> On Tue, 2013-10-22 at 22:32 +0200, Jan Kratochvil wrote:
> > 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.
> 
> I like it. It seems to simplify the code.

That it makes the unwinder suited only for "tracers" but not for debuggers.
The original goal was that you can stop the inferior and now you can traverse
across threads and across frames.  With the current design you cannot.

OTOH for core files - ABRT - it does not matter (cores do not move).
For perf it is questionable, it should minimally affect the inferior,
OTOH currently the Dwfl virtual address map is generated when still the
inferior is running so some of the backtraces may fail.  But still it is
probably more suitable for perf the current non-stopping way.  And one can
guess there will never be a real interactive debugger written a top of
elfutils unwinder.


> > @@ -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)
> > [...]
> > +      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;
> > +	}
> 
> thread_free doesn't set dwfl_errno, so should be save to call without
> saving/restoring it.

But nobody guarantees thread_free cannot modify errno in future.  Such bugs
are difficult to catch, in fact bugs in the error paths are never caught.
Do you still insist on removing saved_errno?

One could rather remove thread_alloc and make Dwfl_Thread just a local
variable in dwfl_getthreads.  Which I did in the attachment.


> I think you can now also simplify the Dwfl_Thread_Callbacks next_thread.
> It doesn't really seem to need the Dwfl_Thread that is being constructed
> since it doesn't seem contain anything that helps with constructing the
> next one. So I would change it to something like:
> 
>   /* Called to iterate through threads.  Returns next TID (thread ID) on
>      success, a negative number on failure and zero if there are no more
>      threads.  dwfl_errno () should be set if negative number has been
>      returned.  *THREAD_ARGP will be NULL on the first call and may be
>      optionally set by the implementation.  On next calls **THREAD_ARGP
>      will point to the *THREAD_ARGP value set last time.  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 must not be NULL.  */
>   pid_t (*next_thread) (Dwfl *dwfl, void *dwfl_arg, void **thread_argp)
>     __nonnull_attribute__ (1, 2);

Here should be only __nonnull_attribute__ (1) now.


> That way the callback can keep any state it wants between next_thread
> calls through *THREAD_ARGP if it wants.
> 
> Does that make sense?

Yes, done.


Thanks,
Jan

diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index 7309cc1..2810c6a 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -100,33 +100,6 @@ state_alloc (Dwfl_Thread *thread)
   return state;
 }
 
-/* Free and unlink THREAD from the internal lists.  */
-static void
-thread_free (Dwfl_Thread *thread)
-{
-  Dwfl_Process *process = thread->process;
-  assert (process->thread == thread);
-  while (thread->unwound)
-    state_free (thread->unwound);
-  process->thread = NULL;
-  free (thread);
-}
-
-/* Allocate new Dwfl_Thread and link it to PROCESS.  */
-static Dwfl_Thread *
-thread_alloc (Dwfl_Process *process)
-{
-  assert (process->thread == NULL);
-  Dwfl_Thread *thread = malloc (sizeof (*thread));
-  if (thread == NULL)
-    return NULL;
-  thread->process = process;
-  thread->unwound = NULL;
-  thread->tid = 0;
-  process->thread = thread;
-  return thread;
-}
-
 void
 internal_function
 __libdwfl_process_free (Dwfl_Process *process)
@@ -134,8 +107,6 @@ __libdwfl_process_free (Dwfl_Process *process)
   Dwfl *dwfl = process->dwfl;
   if (process->callbacks->detach != NULL)
     process->callbacks->detach (dwfl, process->callbacks_arg);
-  if (process->thread)
-    thread_free (process->thread);
   assert (dwfl->process == process);
   dwfl->process = NULL;
   if (process->ebl_close)
@@ -151,7 +122,6 @@ process_alloc (Dwfl *dwfl)
   if (process == NULL)
     return;
   process->dwfl = dwfl;
-  process->thread = NULL;
   dwfl->process = process;
 }
 
@@ -256,34 +226,35 @@ dwfl_getthreads (Dwfl *dwfl, int (*callback) (Dwfl_Thread *thread, void *arg),
       __libdwfl_seterrno (DWFL_E_NO_ATTACH_STATE);
       return -1;
     }
-  Dwfl_Thread *nthread = thread_alloc (process);
-  if (nthread == NULL)
-    {
-      __libdwfl_seterrno (DWFL_E_NOMEM);
-      return -1;
-    }
+  Dwfl_Thread thread;
+  thread.process = process;
+  thread.unwound = NULL;
   for (;;)
     {
-      nthread->tid = process->callbacks->next_thread (dwfl, nthread,
-						      process->callbacks_arg,
-						      &nthread->callbacks_arg);
-      if (nthread->tid < 0)
+      assert (thread.unwound == NULL);
+      thread.tid = process->callbacks->next_thread (dwfl,
+						    process->callbacks_arg,
+						    &thread.callbacks_arg);
+      if (thread.tid < 0)
 	{
 	  Dwfl_Error saved_errno = dwfl_errno ();
-	  thread_free (nthread);
+	  while (thread.unwound)
+	    state_free (thread.unwound);
 	  __libdwfl_seterrno (saved_errno);
 	  return -1;
 	}
-      if (nthread->tid == 0)
+      if (thread.tid == 0)
 	{
-	  thread_free (nthread);
+	  while (thread.unwound)
+	    state_free (thread.unwound);
 	  __libdwfl_seterrno (DWFL_E_NOERROR);
 	  return 0;
 	}
-      int err = callback (nthread, arg);
+      int err = callback (&thread, arg);
       if (err != DWARF_CB_OK)
 	{
-	  thread_free (nthread);
+	  while (thread.unwound)
+	    state_free (thread.unwound);
 	  return err;
 	}
     }
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index d45420f..4aa1f0a 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -582,14 +582,12 @@ typedef struct
   /* Called to iterate through threads.  Returns next TID (thread ID) on
      success, a negative number on failure and zero if there are no more
      threads.  dwfl_errno () should be set if negative number has been
-     returned.  NTHREAD is the new thread being created.  *THREAD_ARGP may be
-     optionally set by the implementation, 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 must not be NULL.  */ 
-  pid_t (*next_thread) (Dwfl *dwfl, Dwfl_Thread *nthread, void *dwfl_arg,
-			void **thread_argp)
-    __nonnull_attribute__ (1, 2);
+     returned.  *THREAD_ARGP may be optionally set by the implementation,
+     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 must not be NULL.  */ 
+  pid_t (*next_thread) (Dwfl *dwfl, 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.
diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
index d554f87..8b686bb 100644
--- a/libdwfl/libdwflP.h
+++ b/libdwfl/libdwflP.h
@@ -218,7 +218,6 @@ struct Dwfl_Process
   void *callbacks_arg;
   struct ebl *ebl;
   bool ebl_close:1;
-  Dwfl_Thread *thread;
 };
 
 /* See its typedef in libdwfl.h.  */
diff --git a/libdwfl/linux-core-attach.c b/libdwfl/linux-core-attach.c
index 567d891..8e4c297 100644
--- a/libdwfl/linux-core-attach.c
+++ b/libdwfl/linux-core-attach.c
@@ -96,8 +96,7 @@ core_memory_read (Dwfl *dwfl, Dwarf_Addr addr, Dwarf_Word *result,
 }
 
 static pid_t
-core_next_thread (Dwfl *dwfl __attribute__ ((unused)),
-		  Dwfl_Thread *nthread __attribute__ ((unused)), void *dwfl_arg,
+core_next_thread (Dwfl *dwfl __attribute__ ((unused)), void *dwfl_arg,
 		  void **thread_argp)
 {
   struct core_arg *core_arg = dwfl_arg;
diff --git a/libdwfl/linux-pid-attach.c b/libdwfl/linux-pid-attach.c
index 2892052..7f43844 100644
--- a/libdwfl/linux-pid-attach.c
+++ b/libdwfl/linux-pid-attach.c
@@ -115,8 +115,7 @@ pid_memory_read (Dwfl *dwfl, Dwarf_Addr addr, Dwarf_Word *result, void *arg)
 }
 
 static pid_t
-pid_next_thread (Dwfl *dwfl __attribute__ ((unused)),
-		 Dwfl_Thread *nthread __attribute__ ((unused)), void *dwfl_arg,
+pid_next_thread (Dwfl *dwfl __attribute__ ((unused)), void *dwfl_arg,
 		 void **thread_argp)
 {
   struct pid_arg *pid_arg = dwfl_arg;
diff --git a/tests/backtrace-data.c b/tests/backtrace-data.c
index 1d1020c..da8f613 100644
--- a/tests/backtrace-data.c
+++ b/tests/backtrace-data.c
@@ -140,8 +140,7 @@ report_module (Dwfl *dwfl, pid_t child, Dwarf_Addr addr)
 }
 
 static pid_t
-next_thread (Dwfl *dwfl, Dwfl_Thread *nthread __attribute__ ((unused)),
-	     void *dwfl_arg __attribute__ ((unused)),
+next_thread (Dwfl *dwfl, void *dwfl_arg __attribute__ ((unused)),
 	     void **thread_argp __attribute__ ((unused)))
 {
   return dwfl_pid (dwfl);