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:
> > > +/* See its typedef in libdwfl.h.  */
> > > +
> > > +struct Dwfl_Frame
> > > +{
> > > +  Dwfl_Thread *thread;
> > > +  /* Previous (outer) frame.  */
> > > +  Dwfl_Frame *unwound;
> > 
> > Do we really need to keep all old frames around like this?
> 
> You have redesigned from scratch the API.  The original goal was to:
>  * Have no callbacks.
>  * Keep cached everything that's possible - never do anything twice.
> 
> I did not want to influence your decisions so I kept it as is.
> But I see now the change to the callbacks I wanted to avoid is not complete.

When thinking about it wrt the new "stateless" design with callbacks we have do
not have to keep the processed frames with the callback approach now.

Checked into the branch the diff below.


Thanks,
Jan


diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index 34b4727..258af8a 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -92,6 +92,7 @@ state_alloc (Dwfl_Thread *thread)
     return NULL;
   state->thread = thread;
   state->signal_frame = false;
+  state->initial_frame = true;
   state->pc_state = DWFL_FRAME_STATE_ERROR;
   memset (state->regs_set, 0, sizeof (state->regs_set));
   thread->unwound = state;
@@ -340,9 +341,10 @@ dwfl_thread_getframes (Dwfl_Thread *thread,
       return -1;
     }
 
-  Dwfl_Frame *state = thread->unwound;
+  Dwfl_Frame *state;
   do
     {
+      state = thread->unwound;
       int err = callback (state, arg);
       if (err != DWARF_CB_OK)
 	{
@@ -353,7 +355,9 @@ dwfl_thread_getframes (Dwfl_Thread *thread,
 	  return err;
 	}
       __libdwfl_frame_unwind (state);
-      state = state->unwound;
+      /* The old frame is no longer needed.  */
+      state_free (thread->unwound);
+      state = thread->unwound;
     }
   while (state && state->pc_state == DWFL_FRAME_STATE_PC_SET);
 
diff --git a/libdwfl/dwfl_frame_pc.c b/libdwfl/dwfl_frame_pc.c
index 99001dd..5462e4c 100644
--- a/libdwfl/dwfl_frame_pc.c
+++ b/libdwfl/dwfl_frame_pc.c
@@ -40,7 +40,7 @@ dwfl_frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool *isactivation)
   if (isactivation)
     {
       /* Bottom frame?  */
-      if (state == state->thread->unwound)
+      if (state->initial_frame)
 	*isactivation = true;
       /* *ISACTIVATION is logical union of whether current or previous frame
 	 state is SIGNAL_FRAME.  */
diff --git a/libdwfl/dwfl_frame_regs.c b/libdwfl/dwfl_frame_regs.c
index c1a08ba..8a11364 100644
--- a/libdwfl/dwfl_frame_regs.c
+++ b/libdwfl/dwfl_frame_regs.c
@@ -34,6 +34,7 @@ dwfl_thread_state_registers (Dwfl_Thread *thread, const int firstreg,
 {
   Dwfl_Frame *state = thread->unwound;
   assert (state && state->unwound == NULL);
+  assert (state->initial_frame);
   for (unsigned regno = firstreg; regno < firstreg + nregs; regno++)
     if (! dwfl_frame_reg_set (state, regno, regs[regno - firstreg]))
       {
@@ -49,6 +50,7 @@ dwfl_thread_state_register_pc (Dwfl_Thread *thread, Dwarf_Word pc)
 {
   Dwfl_Frame *state = thread->unwound;
   assert (state && state->unwound == NULL);
+  assert (state->initial_frame);
   state->pc = pc;
   state->pc_state = DWFL_FRAME_STATE_PC_SET;
 }
diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c
index 4e32958..7acdb59 100644
--- a/libdwfl/frame_unwind.c
+++ b/libdwfl/frame_unwind.c
@@ -333,6 +333,7 @@ handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, Dwarf_CFI *cfi)
   unwound->thread = thread;
   unwound->unwound = NULL;
   unwound->signal_frame = frame->fde->cie->signal_frame;
+  unwound->initial_frame = false;
   unwound->pc_state = DWFL_FRAME_STATE_ERROR;
   memset (unwound->regs_set, 0, sizeof (unwound->regs_set));
   for (unsigned regno = 0; regno < nregs; regno++)
@@ -407,7 +408,7 @@ __libdwfl_frame_unwind (Dwfl_Frame *state)
   assert (ok);
   /* Check whether this is the initial frame or a signal frame.
      Then we need to unwind from the original, unadjusted PC.  */
-  if (state != state->thread->unwound && ! state->signal_frame)
+  if (! state->initial_frame && ! state->signal_frame)
     pc--;
   Dwfl_Module *mod = INTUSE(dwfl_addrmodule) (state->thread->process->dwfl, pc);
   if (mod != NULL)
diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
index 0f18c66..92d51d7 100644
--- a/libdwfl/libdwflP.h
+++ b/libdwfl/libdwflP.h
@@ -229,7 +229,8 @@ struct Dwfl_Thread
   Dwfl_Thread *next;
   pid_t tid;
   bool thread_detach_needed : 1;
-  /* Bottom frame.  */
+  /* 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;
   void *callbacks_arg;
 };
@@ -242,6 +243,7 @@ struct Dwfl_Frame
   /* Previous (outer) frame.  */
   Dwfl_Frame *unwound;
   bool signal_frame : 1;
+  bool initial_frame : 1;
   enum
   {
     /* This structure is still being initialized or there was an error