Re: [patch v5] unwinder: The unwinder (x86* only)

[Mark Wielaard <mjw at redhat dot com>]

On Fri, 2013-10-11 at 13:24 +0200, Mark Wielaard wrote:
> I can try to rewrite it a bit so that the types aren't intertwined
> either. Just make a ebl_state_registers_t callback based on a given
> generic void *arg instead of a Dwfl_Thread *. Only the libdwfl code
> should deal with the specific Dwfl types.

So I was thinking something like the attached (it compiles, might not
actually work).

diff --git a/backends/i386_initreg.c b/backends/i386_initreg.c
index 9d904a4..b776e01 100644
--- a/backends/i386_initreg.c
+++ b/backends/i386_initreg.c
@@ -40,8 +40,9 @@
 #include "libebl_CPU.h"
 
 bool
-i386_set_initial_registers_tid (struct Dwfl_Thread *thread, pid_t tid,
-				dwfl_thread_state_registers_t *setfunc)
+i386_set_initial_registers_tid (pid_t tid,
+				ebl_tid_registers_t *setfunc,
+				void *arg)
 {
 #if !defined __i386__ && !defined __x86_64__
   return false;
@@ -73,7 +74,7 @@ i386_set_initial_registers_tid (struct Dwfl_Thread *thread, pid_t tid,
 # else /* (__i386__ || __x86_64__) && (!__i386__ && !__x86_64__) */
 #  error
 # endif /* (__i386__ || __x86_64__) && (!__i386__ && !__x86_64__) */
-  return setfunc (thread, 0, 9, dwarf_regs);
+  return setfunc (0, 9, dwarf_regs, arg);
 #endif /* __i386__ || __x86_64__ */
   return true;
 }
diff --git a/backends/x86_64_initreg.c b/backends/x86_64_initreg.c
index 9f7c119..1cc9b9f 100644
--- a/backends/x86_64_initreg.c
+++ b/backends/x86_64_initreg.c
@@ -40,8 +40,9 @@
 #include "libebl_CPU.h"
 
 bool
-x86_64_set_initial_registers_tid (struct Dwfl_Thread *thread, pid_t tid,
-				  dwfl_thread_state_registers_t *setfunc)
+x86_64_set_initial_registers_tid (pid_t tid,
+				  ebl_tid_registers_t *setfunc,
+				  void *arg)
 {
 #ifndef __x86_64__
   return false;
@@ -67,6 +68,6 @@ x86_64_set_initial_registers_tid (struct Dwfl_Thread *thread, pid_t tid,
   dwarf_regs[14] = user_regs.r14;
   dwarf_regs[15] = user_regs.r15;
   dwarf_regs[16] = user_regs.rip;
-  return setfunc (thread, 0, 17, dwarf_regs);
+  return setfunc (0, 17, dwarf_regs, arg);
 #endif /* __x86_64__ */
 }
diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index 58878f9..2997e45 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -175,7 +175,7 @@ process_alloc (Dwfl *dwfl)
 }
 
 bool
-dwfl_attach_state (Dwfl *dwfl, Ebl *ebl, pid_t pid,
+dwfl_attach_state (Dwfl *dwfl, pid_t pid,
 		   const Dwfl_Thread_Callbacks *thread_callbacks, void *arg)
 {
   if (thread_callbacks == NULL || thread_callbacks->next_thread == NULL
@@ -189,22 +189,20 @@ dwfl_attach_state (Dwfl *dwfl, Ebl *ebl, pid_t pid,
       __libdwfl_seterrno (DWFL_E_ATTACH_STATE_CONFLICT);
       return false;
     }
+  Ebl *ebl = NULL;
+  for (Dwfl_Module *mod = dwfl->modulelist; mod != NULL; mod = mod->next)
+    {
+      Dwfl_Error error = __libdwfl_module_getebl (mod);
+      if (error != DWFL_E_NOERROR)
+	continue;
+      ebl = mod->ebl;
+      break;
+    }
   if (ebl == NULL)
     {
-      for (Dwfl_Module *mod = dwfl->modulelist; mod != NULL; mod = mod->next)
-	{
-	  Dwfl_Error error = __libdwfl_module_getebl (mod);
-	  if (error != DWFL_E_NOERROR)
-	    continue;
-	  ebl = mod->ebl;
-	  break;
-	}
-      if (ebl == NULL)
-	{
-	  /* Not identified EBL from any of the modules.  */
-	  __libdwfl_seterrno (DWFL_E_PROCESS_NO_ARCH);
-	  return false;
-	}
+      /* Not identified EBL from any of the modules.  */
+      __libdwfl_seterrno (DWFL_E_PROCESS_NO_ARCH);
+      return false;
     }
   process_alloc (dwfl);
   Dwfl_Process *process = dwfl->process;
diff --git a/libdwfl/dwfl_frame_core.c b/libdwfl/dwfl_frame_core.c
index f9f8a41..2ea6533 100644
--- a/libdwfl/dwfl_frame_core.c
+++ b/libdwfl/dwfl_frame_core.c
@@ -359,7 +359,7 @@ __libdwfl_attach_state_for_core (Dwfl *dwfl, Elf *core)
   core_arg->note_data = note_data;
   core_arg->thread_note_offset = 0;
   core_arg->ebl = ebl;
-  if (! INTUSE(dwfl_attach_state) (dwfl, ebl, pid, &core_thread_callbacks, core_arg))
+  if (! INTUSE(dwfl_attach_state) (dwfl, pid, &core_thread_callbacks, core_arg))
     {
       free (core_arg);
       ebl_closebackend (ebl);
diff --git a/libdwfl/dwfl_frame_pid.c b/libdwfl/dwfl_frame_pid.c
index e91e82f..e4cd000 100644
--- a/libdwfl/dwfl_frame_pid.c
+++ b/libdwfl/dwfl_frame_pid.c
@@ -134,6 +134,16 @@ pid_next_thread (Dwfl *dwfl __attribute__ ((unused)),
 }
 
 static bool
+pid_thread_state_registers_cb (const int firstreg,
+			       unsigned nregs,
+			       const Dwarf_Word *regs,
+			       void *arg)
+{
+  Dwfl_Thread *thread = (Dwfl_Thread *) arg;
+  return INTUSE(dwfl_thread_state_registers) (thread, firstreg, nregs, regs);
+}
+
+static bool
 pid_set_initial_registers (Dwfl_Thread *thread, void *thread_arg)
 {
   struct pid_arg *pid_arg = thread_arg;
@@ -151,8 +161,8 @@ pid_set_initial_registers (Dwfl_Thread *thread, void *thread_arg)
   pid_arg->tids_attached[pid_arg->tids_attached_used++] = tid;
   Dwfl_Process *process = thread->process;
   Ebl *ebl = process->ebl;
-  return ebl_set_initial_registers_tid (ebl, thread, tid,
-					INTUSE(dwfl_thread_state_registers));
+  return ebl_set_initial_registers_tid (ebl, tid,
+					pid_thread_state_registers_cb, thread);
 }
 
 static void
@@ -212,7 +222,7 @@ __libdwfl_attach_state_for_pid (Dwfl *dwfl, pid_t pid)
   pid_arg->tids_attached_size = 0;
   pid_arg->tids_attached_used = 0;
   pid_arg->tids_attached = NULL;
-  if (! INTUSE(dwfl_attach_state) (dwfl, NULL, pid, &pid_thread_callbacks, pid_arg))
+  if (! INTUSE(dwfl_attach_state) (dwfl, pid, &pid_thread_callbacks, pid_arg))
     {
       free (pid_arg);
       return false;
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index 511f172..aeb605e 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -30,7 +30,6 @@
 #define _LIBDWFL_H	1
 
 #include "libdw.h"
-#include "libebl.h"
 #include <stdio.h>
 
 /* Handle for a session using the library.  */
@@ -620,16 +619,15 @@ typedef struct
     __nonnull_attribute__ (1);
 } Dwfl_Thread_Callbacks;
 
-/* PID is the process id associated with the DWFL state.  Architecture of DWFL
-   modules is specified by EBL.  If EBL is NULL the function will detect it
-   from arbitrary Dwfl_Module of DWFL.  DWFL_ARG is the callback backend state.
-   DWFL_ARG will be provided to the callbacks.  *THREAD_CALLBACKS function
-   pointers must remain valid during lifetime of DWFL.  Function returns true
-   on success, false otherwise.  */
-bool dwfl_attach_state (Dwfl *dwfl, Ebl *ebl, pid_t pid,
+/* PID is the process id associated with the DWFL state.  DWFL_ARG is
+   the callback backend state.  DWFL_ARG will be provided to the
+   callbacks.  *THREAD_CALLBACKS function pointers must remain valid
+   during lifetime of DWFL.  Function returns true on success, false
+   otherwise.  */
+bool dwfl_attach_state (Dwfl *dwfl, pid_t pid,
                         const Dwfl_Thread_Callbacks *thread_callbacks,
 			void *dwfl_arg)
-  __nonnull_attribute__ (1, 4);
+  __nonnull_attribute__ (1, 3);
 
 /* Return PID for the process associated with DWFL.  Function returns -1 if
    dwfl_attach_state was not called for DWFL.  */
@@ -655,7 +653,14 @@ Dwfl_Thread *dwfl_frame_thread (Dwfl_Frame *state)
 bool dwfl_thread_state_registers (Dwfl_Thread *thread, const int firstreg,
                                   unsigned nregs, const Dwarf_Word *regs)
   __nonnull_attribute__ (1, 4);
+  /*
 dwfl_thread_state_registers_t dwfl_thread_state_registers;
+typedef bool (dwfl_thread_state_registers_t) (struct Dwfl_Thread *thread,
+					      const int firstreg,
+					      unsigned nregs,
+					      const Dwarf_Word *regs)
+  __nonnull_attribute__ (1, 4);
+  */
 
 /* Called by Dwfl_Thread_Callbacks.set_initial_registers implementation.
    If PC is not contained among DWARF registers passed by
diff --git a/libebl/ebl-hooks.h b/libebl/ebl-hooks.h
index 05ca18b..32ca37c 100644
--- a/libebl/ebl-hooks.h
+++ b/libebl/ebl-hooks.h
@@ -156,8 +156,9 @@ int EBLHOOK(disasm) (const uint8_t **startp, const uint8_t *end,
 int EBLHOOK(abi_cfi) (Ebl *ebl, Dwarf_CIE *abi_info);
 
 /* Fetch process data from live TID into THREAD->unwound.  */
-bool EBLHOOK(set_initial_registers_tid) (struct Dwfl_Thread *thread, pid_t tid,
-					dwfl_thread_state_registers_t *setfunc);
+bool EBLHOOK(set_initial_registers_tid) (pid_t tid,
+					 ebl_tid_registers_t *setfunc,
+					 void *arg);
 
 /* Destructor for ELF backend handle.  */
 void EBLHOOK(destr) (struct ebl *);
diff --git a/libebl/eblinitreg.c b/libebl/eblinitreg.c
index 4aa9d9b..8909c50 100644
--- a/libebl/eblinitreg.c
+++ b/libebl/eblinitreg.c
@@ -34,13 +34,14 @@
 #include <assert.h>
 
 bool
-ebl_set_initial_registers_tid (Ebl *ebl, struct Dwfl_Thread *thread, pid_t tid,
-			       dwfl_thread_state_registers_t *setfunc)
+ebl_set_initial_registers_tid (Ebl *ebl, pid_t tid,
+			       ebl_tid_registers_t *setfunc,
+			       void *arg)
 {
   /* Otherwise caller could not allocate THREAD frame of proper size.
      If set_initial_registers_tid is unsupported then FRAME_NREGS is zero.  */
   assert (ebl->set_initial_registers_tid != NULL);
-  return ebl->set_initial_registers_tid (thread, tid, setfunc);
+  return ebl->set_initial_registers_tid (tid, setfunc, arg);
 }
 
 size_t
diff --git a/libebl/libebl.h b/libebl/libebl.h
index e3185b4..f7b13f9 100644
--- a/libebl/libebl.h
+++ b/libebl/libebl.h
@@ -383,17 +383,18 @@ extern int ebl_auxv_info (Ebl *ebl, GElf_Xword a_type,
 			  const char **name, const char **format)
   __nonnull_attribute__ (1, 3, 4);
 
-/* Fetch process data from live TID into THREAD->unwound.  */
-struct Dwfl_Thread;
-typedef bool (dwfl_thread_state_registers_t) (struct Dwfl_Thread *thread,
-					      const int firstreg,
-					      unsigned nregs,
-					      const Dwarf_Word *regs)
-  __nonnull_attribute__ (1, 4);
-extern bool ebl_set_initial_registers_tid (Ebl *ebl, struct Dwfl_Thread *thread,
+/* Callback to fetch process data from live TID.  */
+typedef bool (ebl_tid_registers_t) (const int firstreg,
+				    unsigned nregs,
+				    const Dwarf_Word *regs,
+				    void *arg)
+  __nonnull_attribute__ (3);
+
+extern bool ebl_set_initial_registers_tid (Ebl *ebl,
 					   pid_t tid,
-					 dwfl_thread_state_registers_t *setfunc)
-  __nonnull_attribute__ (1);
+					   ebl_tid_registers_t *setfunc,
+					   void *arg)
+       __nonnull_attribute__ (1, 3);
 
 /* Number of registers to allocate
    for STATE of ebl_set_initial_registers_tid.  */
diff --git a/tests/backtrace-data.c b/tests/backtrace-data.c
index 8f3a405..7f79ecd 100644
--- a/tests/backtrace-data.c
+++ b/tests/backtrace-data.c
@@ -272,7 +272,7 @@ main (int argc __attribute__ ((unused)), char **argv __attribute__ ((unused)))
   assert (l == 0);
   report_module (dwfl, child, user_regs.rip);
 
-  bool ok = dwfl_attach_state (dwfl, NULL, child, &callbacks, NULL);
+  bool ok = dwfl_attach_state (dwfl, child, &callbacks, NULL);
   assert (ok);
 
   /* Multiple threads are not handled here.  */