[PATCH 0/5] libdwfl: expand stack frame interface

[PATCH 0/5] libdwfl: expand stack frame interface

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Hello,

While using the libdwfl stack unwinding interface for my debugger [1], I
found that it works great for getting the program counter at each stack
frame, but it's hard to do anything beyond that. This is an attempt to
expand the functionality by adding two main features: attaching
to/detaching from threads at will, and evaluating DWARF expressions.
This functionality already exists within libdwfl, so it's just a matter
of defining the interface.

Patches 1 and 2 are cleanups in preparation for the rest of the series.
Patch 3 adds dwfl_attach_thread and dwfl_detach_thread so that it's
possible to attach to threads at will and save stack frames so that they
can be inspected later. Patch 4 adds dwfl_frame_module and
dwfl_frame_dwarf_frame to avoid redundant lookups internally and in
clients of libdwfl. Patch 5 adds dwfl_frame_eval_expr for evaluating
DWARF expressions in the context of a stack frame.

Please let me know what you think.

Thanks!

1: https://drgn.readthedocs.io/en/latest/

Omar Sandoval (5):
  libdwfl: don't bother freeing frames outside of dwfl_thread_getframes
  libdwfl: only use thread->unwound for initial frame
  libdwfl: add interface for attaching to/detaching from threads
  libdwfl: cache Dwfl_Module and Dwarf_Frame for Dwfl_Frame
  libdwfl: add interface for evaluating DWARF expressions in a frame

 libdw/ChangeLog        |   9 ++
 libdw/libdw.map        |   8 ++
 libdwfl/ChangeLog      |  31 +++++++
 libdwfl/dwfl_frame.c   | 190 ++++++++++++++++++++++++-----------------
 libdwfl/frame_unwind.c |  37 +++++++-
 libdwfl/libdwfl.h      |  28 ++++++
 libdwfl/libdwflP.h     |   9 +-
 7 files changed, 228 insertions(+), 84 deletions(-)

-- 
2.23.0

[PATCH 3/5] libdwfl: add interface for attaching to/detaching from threads

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

libdwfl has implementations of attaching to/detaching from threads and
unwinding stack traces. However, that functionality is only available
through the dwfl_thread_getframes interface, which isn't very flexible.
This adds two new functions, dwfl_attach_thread and dwfl_detach_thread,
which separate the thread stopping functionality out of
dwfl_thread_getframes. Additionally, it makes dwfl_thread_getframes
cache the stack trace for threads stopped this way. This makes it
possible to use the frames after dwfl_thread_getframes returns.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libdw/ChangeLog      |   6 ++
 libdw/libdw.map      |   5 ++
 libdwfl/ChangeLog    |   8 +++
 libdwfl/dwfl_frame.c | 136 ++++++++++++++++++++++++++++++-------------
 libdwfl/libdwfl.h    |  12 ++++
 libdwfl/libdwflP.h   |   3 +-
 6 files changed, 128 insertions(+), 42 deletions(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 498cf0b7..1de8e2fc 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,9 @@
+2019-10-07  Omar Sandoval  <osandov@fb.com>
+
+	* libdw.map (ELFUTILS_0.178): New section.
+	Add dwfl_attach_thread.
+	Add dwfl_detach_thread.
+
 2019-07-05  Omar Sandoval  <osandov@fb.com>
 
 	* Makefile.am (libdw_so_LIBS): Replace libebl.a with libebl_pic.a.
diff --git a/libdw/libdw.map b/libdw/libdw.map
index decac05c..f20ffc2f 100644
--- a/libdw/libdw.map
+++ b/libdw/libdw.map
@@ -370,3 +370,8 @@ ELFUTILS_0.177 {
     # presume that NULL is only returned on error (otherwise ELF_K_NONE).
     dwelf_elf_begin;
 } ELFUTILS_0.175;
+ELFUTILS_0.178 {
+  global:
+    dwfl_attach_thread;
+    dwfl_detach_thread;
+} ELFUTILS_0.177;
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 07a1e8df..b7eaedca 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -7,6 +7,14 @@
 	(thread_free_all_states): Remove function.
 	(free_states): Add function.
 	(dwfl_thread_getframes): Don't update thread->unwound while unwinding.
+	Use start_unwind.
+	Cache frames when attached with dwfl_attach_thread.
+	(start_unwind): Add function.
+	(attach_thread_cb): Add function.
+	(dwfl_attach_thread): Add function.
+	(dwfl_detach_thread): Add function.
+	* libdwfl.h (dwfl_attach_thread): Add definition.
+	(dwfl_detach_thread): Add definition.
 	* libdwflP.h (struct Dwfl_Thread): Update comment for unwound member.
 
 2019-08-12  Mark Wielaard  <mark@klomp.org>
diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index 5bbf850e..61fad8b9 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -103,6 +103,29 @@ state_alloc (Dwfl_Thread *thread)
   return state;
 }
 
+static Dwfl_Frame *
+start_unwind(Dwfl_Thread *thread)
+{
+  if (ebl_frame_nregs (thread->process->ebl) == 0)
+    {
+      __libdwfl_seterrno (DWFL_E_NO_UNWIND);
+      return NULL;
+    }
+  if (state_alloc (thread) == NULL)
+    {
+      __libdwfl_seterrno (DWFL_E_NOMEM);
+      return NULL;
+    }
+  if (! thread->process->callbacks->set_initial_registers (thread,
+							   thread->callbacks_arg))
+    {
+      free_states (thread->unwound);
+      thread->unwound = NULL;
+      return NULL;
+    }
+  return thread->unwound;
+}
+
 void
 internal_function
 __libdwfl_process_free (Dwfl_Process *process)
@@ -366,6 +389,45 @@ getthread (Dwfl *dwfl, pid_t tid,
    return err;
 }
 
+static int
+attach_thread_cb(Dwfl_Thread *thread, void *arg)
+{
+  Dwfl_Thread *copied = malloc (sizeof (*copied));
+  if (copied == NULL)
+    {
+      __libdwfl_seterrno (DWFL_E_NOMEM);
+      return DWARF_CB_ABORT;
+    }
+  *copied = *thread;
+  if (start_unwind (copied) == NULL)
+    {
+      free (copied);
+      return DWARF_CB_ABORT;
+    }
+  *(Dwfl_Thread **)arg = copied;
+  return DWARF_CB_OK;
+}
+
+Dwfl_Thread *
+dwfl_attach_thread(Dwfl *dwfl, pid_t tid)
+{
+  Dwfl_Thread *thread;
+  if (getthread (dwfl, tid, attach_thread_cb, &thread))
+    return NULL;
+  return thread;
+}
+
+void
+dwfl_detach_thread(Dwfl_Thread *thread)
+{
+  if (thread == NULL)
+    return;
+  if (thread->process->callbacks->thread_detach)
+    thread->process->callbacks->thread_detach (thread, thread->callbacks_arg);
+  free_states (thread->unwound);
+  free (thread);
+}
+
 struct one_thread
 {
   int (*callback) (Dwfl_Frame *frame, void *arg);
@@ -394,63 +456,55 @@ dwfl_thread_getframes (Dwfl_Thread *thread,
 		       int (*callback) (Dwfl_Frame *state, void *arg),
 		       void *arg)
 {
-  Ebl *ebl = thread->process->ebl;
-  if (ebl_frame_nregs (ebl) == 0)
-    {
-      __libdwfl_seterrno (DWFL_E_NO_UNWIND);
-      return -1;
-    }
-  if (state_alloc (thread) == NULL)
-    {
-      __libdwfl_seterrno (DWFL_E_NOMEM);
-      return -1;
-    }
   Dwfl_Process *process = thread->process;
-  if (! process->callbacks->set_initial_registers (thread,
-						   thread->callbacks_arg))
-    {
-      free_states (thread->unwound);
-      thread->unwound = NULL;
-      return -1;
-    }
+  int ret = -1;
+  bool cache = thread->unwound != NULL;
+  if (! cache && start_unwind (thread) == NULL)
+    return -1;
   Dwfl_Frame *state = thread->unwound;
-  thread->unwound = NULL;
+  if (! cache)
+    thread->unwound = NULL;
   if (! state_fetch_pc (state))
-    {
-      if (process->callbacks->thread_detach)
-	process->callbacks->thread_detach (thread, thread->callbacks_arg);
-      free_states (state);
-      return -1;
-    }
+    goto out;
   do
     {
       int err = callback (state, arg);
       if (err != DWARF_CB_OK)
 	{
-	  if (process->callbacks->thread_detach)
-	    process->callbacks->thread_detach (thread, thread->callbacks_arg);
-	  free_states (state);
-	  return err;
+	  ret = err;
+	  goto out;
+	}
+      if (state->unwound == NULL)
+	__libdwfl_frame_unwind (state);
+      else if (state->unwound->pc_state == DWFL_FRAME_STATE_ERROR)
+	{
+	  /* This frame was previously cached as an error.  We still return -1,
+	     but we don't know what the original error was.  */
+	  __libdwfl_seterrno (DWFL_E_NOERROR);
 	}
-      __libdwfl_frame_unwind (state);
       Dwfl_Frame *next = state->unwound;
-      /* The old frame is no longer needed.  */
-      free (state);
+      if (! cache)
+	{
+	  /* The old frame is no longer needed.  */
+	  free (state);
+	}
       state = next;
     }
   while (state && state->pc_state == DWFL_FRAME_STATE_PC_SET);
 
-  Dwfl_Error err = dwfl_errno ();
-  if (process->callbacks->thread_detach)
-    process->callbacks->thread_detach (thread, thread->callbacks_arg);
-  if (state == NULL || state->pc_state == DWFL_FRAME_STATE_ERROR)
+  if (state && state->pc_state == DWFL_FRAME_STATE_PC_UNDEFINED)
+    ret = 0;
+out:
+  if (! cache)
     {
+      if (process->callbacks->thread_detach)
+	{
+	  Dwfl_Error err = dwfl_errno ();
+	  process->callbacks->thread_detach (thread, thread->callbacks_arg);
+	  __libdwfl_seterrno (err);
+	}
       free_states (state);
-      __libdwfl_seterrno (err);
-      return -1;
     }
-  assert (state->pc_state == DWFL_FRAME_STATE_PC_UNDEFINED);
-  free_states (state);
-  return 0;
+  return ret;
 }
 INTDEF(dwfl_thread_getframes)
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index a0c1d357..a22afc78 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -775,6 +775,18 @@ int dwfl_getthreads (Dwfl *dwfl,
 		     void *arg)
   __nonnull_attribute__ (1, 2);
 
+/* Attach to a thread.  The returned thread must be detached and freed with
+   dwfl_detach_thread.  Returns NULL on error.  This calls the
+   set_initial_registers callback.  While a thread is attached,
+   dwfl_thread_getframes will cache the unwound frames for the thread.  They
+   remain valid until dwfl_detach_thread is called.  */
+Dwfl_Thread *dwfl_attach_thread(Dwfl *dwfl, pid_t tid)
+  __nonnull_attribute__ (1);
+
+/* Detach from a thread that was attached with dwfl_attach_thread and free it.
+   This calls the detach_thread callback.  */
+void dwfl_detach_thread(Dwfl_Thread *thread);
+
 /* Iterate through the frames for a thread.  Returns zero if all frames
    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
diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
index 6b2d4867..c80d2051 100644
--- a/libdwfl/libdwflP.h
+++ b/libdwfl/libdwflP.h
@@ -242,7 +242,8 @@ struct Dwfl_Thread
 {
   Dwfl_Process *process;
   pid_t tid;
-  /* Bottom (innermost) frame while we're initializing, NULL afterwards.  */
+  /* Bottom (innermost) frame.  If the stack trace is not cached, then this is
+     NULL except during initialization.  */
   Dwfl_Frame *unwound;
   void *callbacks_arg;
 };
-- 
2.23.0

Re: [PATCH 3/5] libdwfl: add interface for attaching to/detaching from threads

[Mark Wielaard]

Hi Omar,

On Mon, 2019-10-07 at 02:05 -0700, Omar Sandoval wrote:
> libdwfl has implementations of attaching to/detaching from threads
> and
> unwinding stack traces. However, that functionality is only available
> through the dwfl_thread_getframes interface, which isn't very flexible.
> This adds two new functions, dwfl_attach_thread and dwfl_detach_thread,
> which separate the thread stopping functionality out of
> dwfl_thread_getframes. Additionally, it makes dwfl_thread_getframes
> cache the stack trace for threads stopped this way. This makes it
> possible to use the frames after dwfl_thread_getframes returns.

The advantage of the dwfl_thread_getframes interface is that you cannot
forget to "detach", so no Dwfl_Frames get dangling and (if the process
is life) you don't disrupt it more than necessary. This new interface
seems very simple to get wrong causing leaks and locking up
processes/threads.

Also, if we would adopt dwfl_attach_thread then I think it should take
a Dwfl_Thread object not a pid/tid as argument.

Could you provide some examples where this new interface/style is
beneficial?

Thanks,

Mark

Re: [PATCH 3/5] libdwfl: add interface for attaching to/detaching from threads

[Omar Sandoval]

On Wed, Oct 30, 2019 at 01:47:52PM +0100, Mark Wielaard wrote:
> Hi Omar,
> 
> On Mon, 2019-10-07 at 02:05 -0700, Omar Sandoval wrote:
> > libdwfl has implementations of attaching to/detaching from threads
> > and
> > unwinding stack traces. However, that functionality is only available
> > through the dwfl_thread_getframes interface, which isn't very flexible.
> > This adds two new functions, dwfl_attach_thread and dwfl_detach_thread,
> > which separate the thread stopping functionality out of
> > dwfl_thread_getframes. Additionally, it makes dwfl_thread_getframes
> > cache the stack trace for threads stopped this way. This makes it
> > possible to use the frames after dwfl_thread_getframes returns.
> 
> The advantage of the dwfl_thread_getframes interface is that you cannot
> forget to "detach", so no Dwfl_Frames get dangling and (if the process
> is life) you don't disrupt it more than necessary. This new interface
> seems very simple to get wrong causing leaks and locking up
> processes/threads.

That is true, although I imagine that use cases which don't need the
additional flexibility would continue to use the simpler API.

> Also, if we would adopt dwfl_attach_thread then I think it should take
> a Dwfl_Thread object not a pid/tid as argument.

In that case, we'd probably want to expose the internal getthread
function with something like:

/* Find the thread with the given thread ID.  Returns zero if the
   thread was processed, -1 on error (including when no thread with the
   given thread ID exists), or the return value of the callback when not
   DWARF_CB_OK.  */
int dwfl_getthread (Dwfl *dwfl, pid_t tid,
		    int (*callback) (Dwfl_Thread *thread, void *arg),
		    void *arg)
  __nonnull_attribute__ (1, 3);

Then dwfl_attach_thread could be used with either dwfl_getthread or
dwfl_getthreads, which is nice. However, a crucial part of this feature
is being able to access the Dwfl_Thread outside of the callback. Since
the Dwfl_Thread is currently on the stack, I see a couple of options:

1. We keep Dwfl_Thread on the stack and make dwfl_attach_thread() return
   a copy (like it does in this patch).
2. We always allocate the Dwfl_Thread on the heap and free it before
   returning from dwfl_getthread(s) _unless_ dwfl_attach_thread() was
   called. If it was, it will be freed by dwfl_detach_thread() instead.

Option 2 sounds better to me. What do you think?

> Could you provide some examples where this new interface/style is
> beneficial?

dwfl_attach_thread could be used to implement something like `gdb -p
$pid`: attach to a thread, stop it, and poke at it further.
dwfl_detach_thread would be kind of like GDB's `continue` command.

I applied these patches to the version of elfutils that I bundle with
drgn; you can see how I use the interface here [1]. Basically, drgn has
an API to get the stack trace for a thread [2] and get the value of
registers (and in the future, local variables) at each stack frame [3].
When I get the stack trace, I use dwfl_attach_thread and
dwfl_thread_getframes. The user can then access the frames to their
heart's content. When they're done with it, I free everything with
dwfl_detach_thread. (The attach/detach is currently tied to the lifetime
of the stack trace because I don't yet support actually pausing threads,
but I plan to support that, hopefully using the same libdwfl
attach/detach API.)

Thanks, and sorry for the wall of text :)

1: https://github.com/osandov/drgn/blob/master/libdrgn/stack_trace.c
2: https://drgn.readthedocs.io/en/latest/api_reference.html#drgn.Program.stack_trace
3: https://drgn.readthedocs.io/en/latest/api_reference.html#stack-traces

Re: [PATCH 3/5] libdwfl: add interface for attaching to/detaching from threads

[Mark Wielaard]

On Wed, 2019-10-30 at 16:49 -0700, Omar Sandoval wrote:
> On Wed, Oct 30, 2019 at 01:47:52PM +0100, Mark Wielaard wrote:
> > Also, if we would adopt dwfl_attach_thread then I think it should take
> > a Dwfl_Thread object not a pid/tid as argument.
> 
> In that case, we'd probably want to expose the internal getthread
> function with something like:
> 
> /* Find the thread with the given thread ID.  Returns zero if the
>    thread was processed, -1 on error (including when no thread with the
>    given thread ID exists), or the return value of the callback when not
>    DWARF_CB_OK.  */
> int dwfl_getthread (Dwfl *dwfl, pid_t tid,
> 		    int (*callback) (Dwfl_Thread *thread, void *arg),
> 		    void *arg)
>   __nonnull_attribute__ (1, 3);
> 
> Then dwfl_attach_thread could be used with either dwfl_getthread or
> dwfl_getthreads, which is nice. However, a crucial part of this
> feature
> is being able to access the Dwfl_Thread outside of the callback. 
>
> Since
> the Dwfl_Thread is currently on the stack, I see a couple of options:
> 
> 1. We keep Dwfl_Thread on the stack and make dwfl_attach_thread()
> return
>    a copy (like it does in this patch).
> 2. We always allocate the Dwfl_Thread on the heap and free it before
>    returning from dwfl_getthread(s) _unless_ dwfl_attach_thread() was
>    called. If it was, it will be freed by dwfl_detach_thread()
> instead.
> 
> Option 2 sounds better to me. What do you think?

To be honest I am not sure I like either.

I think it is mainly because this isn't really about
attaching/detaching from a thread but stopping/resuming it. That is
part of what set_initial_registers does. So what
dwfl_attach_thread/dwfl_detach_thread really do is setting up the
Dwfl_Thread so it can be queried/used.

You are attached/detached on the process as a whole. Which is done with
the dwfl_linux_proc_attach call.

So basically I think what we want is an interface which you call
pauzing the thread.

Sorry for not having a clear vision of the perfect interface yet.

Cheers,

Mark

Re: [PATCH 3/5] libdwfl: add interface for attaching to/detaching from threads

[Omar Sandoval]

On Thu, Oct 31, 2019 at 05:21:17PM +0100, Mark Wielaard wrote:
> On Wed, 2019-10-30 at 16:49 -0700, Omar Sandoval wrote:
> > On Wed, Oct 30, 2019 at 01:47:52PM +0100, Mark Wielaard wrote:
> > > Also, if we would adopt dwfl_attach_thread then I think it should take
> > > a Dwfl_Thread object not a pid/tid as argument.
> > 
> > In that case, we'd probably want to expose the internal getthread
> > function with something like:
> > 
> > /* Find the thread with the given thread ID.  Returns zero if the
> >    thread was processed, -1 on error (including when no thread with the
> >    given thread ID exists), or the return value of the callback when not
> >    DWARF_CB_OK.  */
> > int dwfl_getthread (Dwfl *dwfl, pid_t tid,
> > 		    int (*callback) (Dwfl_Thread *thread, void *arg),
> > 		    void *arg)
> >   __nonnull_attribute__ (1, 3);
> > 
> > Then dwfl_attach_thread could be used with either dwfl_getthread or
> > dwfl_getthreads, which is nice. However, a crucial part of this
> > feature
> > is being able to access the Dwfl_Thread outside of the callback. 
> >
> > Since
> > the Dwfl_Thread is currently on the stack, I see a couple of options:
> > 
> > 1. We keep Dwfl_Thread on the stack and make dwfl_attach_thread()
> > return
> >    a copy (like it does in this patch).
> > 2. We always allocate the Dwfl_Thread on the heap and free it before
> >    returning from dwfl_getthread(s) _unless_ dwfl_attach_thread() was
> >    called. If it was, it will be freed by dwfl_detach_thread()
> > instead.
> > 
> > Option 2 sounds better to me. What do you think?
> 
> To be honest I am not sure I like either.
> 
> I think it is mainly because this isn't really about
> attaching/detaching from a thread but stopping/resuming it. That is
> part of what set_initial_registers does. So what
> dwfl_attach_thread/dwfl_detach_thread really do is setting up the
> Dwfl_Thread so it can be queried/used.
> 
> You are attached/detached on the process as a whole. Which is done with
> the dwfl_linux_proc_attach call.
> 
> So basically I think what we want is an interface which you call
> pauzing the thread.
> 
> Sorry for not having a clear vision of the perfect interface yet.

No problem, I'm sure we'll be able to come up with something better :)

I was hesitant to call this dwfl_stop_thread/dwfl_resume_thread because
you aren't necessarily stopping/resuming anything, for example when
you're working with a core dump. Plus, the detach terminology is
consistent with Dwfl_Thread_Callbacks::thread_detach.

But, stop/resume is more descriptive. Does that sound better to you? The
semantics could be that a Dwfl_Thread is valid after dwfl_getthread(s)
returns if and only if it is currently paused. The Dwfl_Thread is freed
on dwfl_resume_thread() or when dwfl_getthread(s) returns, whichever
comes last.

Thanks,
Omar

[PATCH 1/5] libdwfl: don't bother freeing frames outside of dwfl_thread_getframes

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

dwfl_thread_getframes always frees the state before returning, so
dwfl_getthreads and getthread don't need to do it.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libdwfl/ChangeLog    |  6 ++++++
 libdwfl/dwfl_frame.c | 18 +++---------------
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 04a39637..f4a3cad9 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,9 @@
+2019-10-07  Omar Sandoval  <osandov@fb.com>
+
+	* dwfl_frame.c (dwfl_getthreads): Get rid of unnecessary
+	thread_free_all_states calls.
+	(getthread): Ditto.
+
 2019-08-12  Mark Wielaard  <mark@klomp.org>
 
 	* gzip.c (open_stream): Return DWFL_E_ERRNO on bad file operation.
diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index 881f735a..20bdbd9b 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -279,24 +279,15 @@ dwfl_getthreads (Dwfl *dwfl, int (*callback) (Dwfl_Thread *thread, void *arg),
 						    process->callbacks_arg,
 						    &thread.callbacks_arg);
       if (thread.tid < 0)
-	{
-	  Dwfl_Error saved_errno = dwfl_errno ();
-	  thread_free_all_states (&thread);
-	  __libdwfl_seterrno (saved_errno);
-	  return -1;
-	}
+	return -1;
       if (thread.tid == 0)
 	{
-	  thread_free_all_states (&thread);
 	  __libdwfl_seterrno (DWFL_E_NOERROR);
 	  return 0;
 	}
       int err = callback (&thread, arg);
       if (err != DWARF_CB_OK)
-	{
-	  thread_free_all_states (&thread);
-	  return err;
-	}
+	return err;
       assert (thread.unwound == NULL);
     }
   /* NOTREACHED */
@@ -356,11 +347,8 @@ getthread (Dwfl *dwfl, pid_t tid,
       if (process->callbacks->get_thread (dwfl, tid, process->callbacks_arg,
 					  &thread.callbacks_arg))
 	{
-	  int err;
 	  thread.tid = tid;
-	  err = callback (&thread, arg);
-	  thread_free_all_states (&thread);
-	  return err;
+	  return callback (&thread, arg);
 	}
 
       return -1;
-- 
2.23.0

Re: [PATCH 1/5] libdwfl: don't bother freeing frames outside of dwfl_thread_getframes

[Mark Wielaard]

Hi Omar,

On Mon, 2019-10-07 at 02:05 -0700, Omar Sandoval wrote:
> dwfl_thread_getframes always frees the state before returning, so
> dwfl_getthreads and getthread don't need to do it.

I am not sure I follow. dwfl_getthreads can be used independently from
its (indirect) usage from dwfl_thread_getframes. So doesn't it need to
do its own cleanup?

Thanks,

Mark

Re: [PATCH 1/5] libdwfl: don't bother freeing frames outside of dwfl_thread_getframes

[Omar Sandoval]

On Tue, Oct 29, 2019 at 04:55:27PM +0100, Mark Wielaard wrote:
> Hi Omar,
> 
> On Mon, 2019-10-07 at 02:05 -0700, Omar Sandoval wrote:
> > dwfl_thread_getframes always frees the state before returning, so
> > dwfl_getthreads and getthread don't need to do it.
> 
> I am not sure I follow. dwfl_getthreads can be used independently from
> its (indirect) usage from dwfl_thread_getframes. So doesn't it need to
> do its own cleanup?

Hi, Mark,

Unless I missed something, the only place we allocate the state is from
dwfl_thread_getframes, and we always free it before returning from that
function. So if you're not using dwfl_thread_getframes, dwfl_getthreads
won't have anything to free, and if you are, dwfl_thread_getframes
already freed it. Or am I missing something?

Thanks,
Omar

Re: [PATCH 1/5] libdwfl: don't bother freeing frames outside of dwfl_thread_getframes

[Mark Wielaard]

On Tue, 2019-10-29 at 09:17 -0700, Omar Sandoval wrote:
> Unless I missed something, the only place we allocate the state is
> from dwfl_thread_getframes, and we always free it before returning from
> that function. So if you're not using dwfl_thread_getframes, dwfl_getthreads
> won't have anything to free, and if you are, dwfl_thread_getframes
> already freed it. Or am I missing something?

O, you are right. Sorry, I missed that. Now that I understand what is
going on it actually is odd that the other functions try to free the
state. They cannot know whether that is what their caller wants. So
yes, this makes sense. Added that extra sentence to the commit message
and pushed to master.

Thanks,

Mark

[PATCH 4/5] libdwfl: cache Dwfl_Module and Dwarf_Frame for Dwfl_Frame

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

The next change will need to have the Dwarf_Frame readily available, so
rather than finding it again every time, let's cache it for reuse. The
CFI frame can also be useful to clients of libdwfl, so add
dwfl_frame_dwarf_frame to get it. Similarly, the Dwfl_Module is also
frequently needed in conjunction with the frame, so cache it and add
dwfl_frame_module.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libdw/ChangeLog        |  2 ++
 libdw/libdw.map        |  2 ++
 libdwfl/ChangeLog      | 10 ++++++++++
 libdwfl/dwfl_frame.c   |  2 ++
 libdwfl/frame_unwind.c | 25 +++++++++++++++++++++----
 libdwfl/libdwfl.h      | 10 ++++++++++
 libdwfl/libdwflP.h     |  5 +++++
 7 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 1de8e2fc..1a712497 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -3,6 +3,8 @@
 	* libdw.map (ELFUTILS_0.178): New section.
 	Add dwfl_attach_thread.
 	Add dwfl_detach_thread.
+	Add dwfl_frame_module.
+	Add dwfl_frame_dwarf_frame.
 
 2019-07-05  Omar Sandoval  <osandov@fb.com>
 
diff --git a/libdw/libdw.map b/libdw/libdw.map
index f20ffc2f..c1469c4e 100644
--- a/libdw/libdw.map
+++ b/libdw/libdw.map
@@ -374,4 +374,6 @@ ELFUTILS_0.178 {
   global:
     dwfl_attach_thread;
     dwfl_detach_thread;
+    dwfl_frame_module;
+    dwfl_frame_dwarf_frame;
 } ELFUTILS_0.177;
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index b7eaedca..fdb517c9 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -13,9 +13,19 @@
 	(attach_thread_cb): Add function.
 	(dwfl_attach_thread): Add function.
 	(dwfl_detach_thread): Add function.
+	(state_alloc): Initialize state->mod and state->frame.
+	* frame_unwind.c (new_unwound): Initialize state->mod and
+	state->frame.
+	(handle_cfi): Cache state->frame and state->bias.
+	(__libdwfl_frame_unwind): Cache state->mod.
+	(dwfl_frame_module): Add function.
+	(dwfl_frame_dwarf_frame): Add function.
 	* libdwfl.h (dwfl_attach_thread): Add definition.
 	(dwfl_detach_thread): Add definition.
+	(dwfl_frame_module): Add definition.
+	(dwfl_frame_dwarf_frame): Add definition.
 	* libdwflP.h (struct Dwfl_Thread): Update comment for unwound member.
+	Add mod, frame, and bias.
 
 2019-08-12  Mark Wielaard  <mark@klomp.org>
 
diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index 61fad8b9..3f660f7e 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -94,6 +94,8 @@ state_alloc (Dwfl_Thread *thread)
   if (state == NULL)
     return NULL;
   state->thread = thread;
+  state->mod = NULL;
+  state->frame = NULL;
   state->signal_frame = false;
   state->initial_frame = true;
   state->pc_state = DWFL_FRAME_STATE_ERROR;
diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c
index d7dfa5a9..9738ca72 100644
--- a/libdwfl/frame_unwind.c
+++ b/libdwfl/frame_unwind.c
@@ -523,6 +523,8 @@ new_unwound (Dwfl_Frame *state)
   state->unwound = unwound;
   unwound->thread = thread;
   unwound->unwound = NULL;
+  unwound->mod = NULL;
+  unwound->frame = NULL;
   unwound->signal_frame = false;
   unwound->initial_frame = false;
   unwound->pc_state = DWFL_FRAME_STATE_ERROR;
@@ -544,6 +546,8 @@ handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, Dwarf_CFI *cfi, Dwarf_Addr bias)
       __libdwfl_seterrno (DWFL_E_LIBDW);
       return;
     }
+  state->frame = frame;
+  state->bias = bias;
 
   Dwfl_Frame *unwound = new_unwound (state);
   if (unwound == NULL)
@@ -724,20 +728,20 @@ __libdwfl_frame_unwind (Dwfl_Frame *state)
      Then we need to unwind from the original, unadjusted PC.  */
   if (! state->initial_frame && ! state->signal_frame)
     pc--;
-  Dwfl_Module *mod = INTUSE(dwfl_addrmodule) (state->thread->process->dwfl, pc);
-  if (mod == NULL)
+  state->mod = INTUSE(dwfl_addrmodule) (state->thread->process->dwfl, pc);
+  if (state->mod == NULL)
     __libdwfl_seterrno (DWFL_E_NO_DWARF);
   else
     {
       Dwarf_Addr bias;
-      Dwarf_CFI *cfi_eh = INTUSE(dwfl_module_eh_cfi) (mod, &bias);
+      Dwarf_CFI *cfi_eh = INTUSE(dwfl_module_eh_cfi) (state->mod, &bias);
       if (cfi_eh)
 	{
 	  handle_cfi (state, pc - bias, cfi_eh, bias);
 	  if (state->unwound)
 	    return;
 	}
-      Dwarf_CFI *cfi_dwarf = INTUSE(dwfl_module_dwarf_cfi) (mod, &bias);
+      Dwarf_CFI *cfi_dwarf = INTUSE(dwfl_module_dwarf_cfi) (state->mod, &bias);
       if (cfi_dwarf)
 	{
 	  handle_cfi (state, pc - bias, cfi_dwarf, bias);
@@ -770,3 +774,16 @@ __libdwfl_frame_unwind (Dwfl_Frame *state)
   assert (state->unwound->pc_state == DWFL_FRAME_STATE_PC_SET);
   state->unwound->signal_frame = signal_frame;
 }
+
+Dwfl_Module *
+dwfl_frame_module (Dwfl_Frame *state)
+{
+  return state->mod;
+}
+
+Dwarf_Frame *
+dwfl_frame_dwarf_frame (Dwfl_Frame *state, Dwarf_Addr *bias)
+{
+  *bias = state->bias;
+  return state->frame;
+}
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index a22afc78..b450816b 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -750,6 +750,16 @@ pid_t dwfl_thread_tid (Dwfl_Thread *thread)
 Dwfl_Thread *dwfl_frame_thread (Dwfl_Frame *state)
   __nonnull_attribute__ (1);
 
+/* Return module containing the PC for frame STATE.  Returns NULL if no module
+   contains the PC.  */
+Dwfl_Module *dwfl_frame_module (Dwfl_Frame *state)
+  __nonnull_attribute__ (1);
+
+/* Return CFI frame for frame STATE.  Returns NULL if no CFI frame was
+   found. */
+Dwarf_Frame *dwfl_frame_dwarf_frame (Dwfl_Frame *state, Dwarf_Addr *bias)
+  __nonnull_attribute__ (1, 2);
+
 /* Called by Dwfl_Thread_Callbacks.set_initial_registers implementation.
    For every known continuous block of registers <FIRSTREG..FIRSTREG+NREGS)
    (inclusive..exclusive) set their content to REGS (array of NREGS items).
diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
index c80d2051..ea417596 100644
--- a/libdwfl/libdwflP.h
+++ b/libdwfl/libdwflP.h
@@ -255,6 +255,11 @@ struct Dwfl_Frame
   Dwfl_Thread *thread;
   /* Previous (outer) frame.  */
   Dwfl_Frame *unwound;
+  /* Module containing pc. */
+  Dwfl_Module *mod;
+  /* CFI frame containing pc. */
+  Dwarf_Frame *frame;
+  Dwarf_Addr bias;
   bool signal_frame : 1;
   bool initial_frame : 1;
   enum
-- 
2.23.0

Re: [PATCH 4/5] libdwfl: cache Dwfl_Module and Dwarf_Frame for Dwfl_Frame

[Mark Wielaard]

Hi Omar,

On Mon, 2019-10-07 at 02:05 -0700, Omar Sandoval wrote:
> The next change will need to have the Dwarf_Frame readily available, so
> rather than finding it again every time, let's cache it for reuse. The
> CFI frame can also be useful to clients of libdwfl, so add
> dwfl_frame_dwarf_frame to get it. Similarly, the Dwfl_Module is also
> frequently needed in conjunction with the frame, so cache it and add
> dwfl_frame_module.

I can see how this is useful. But it seems the Dwarf_Frame is only
cached when __libdwfl_frame_unwind is called. Which I believe isn't
done for the initial frame. Also it isn't clear how to propagate any
errors when NULL is returned. Maybe dwfl_frame_dwarf_frame () should
check first to see if frame is NULL and then call lookup the CFI and
call dwarf_cfi_addrframe if not?

Thanks,

Mark

Re: [PATCH 4/5] libdwfl: cache Dwfl_Module and Dwarf_Frame for Dwfl_Frame

[Omar Sandoval]

On Wed, Oct 30, 2019 at 02:04:42PM +0100, Mark Wielaard wrote:
> Hi Omar,
> 
> On Mon, 2019-10-07 at 02:05 -0700, Omar Sandoval wrote:
> > The next change will need to have the Dwarf_Frame readily available, so
> > rather than finding it again every time, let's cache it for reuse. The
> > CFI frame can also be useful to clients of libdwfl, so add
> > dwfl_frame_dwarf_frame to get it. Similarly, the Dwfl_Module is also
> > frequently needed in conjunction with the frame, so cache it and add
> > dwfl_frame_module.
> 
> I can see how this is useful. But it seems the Dwarf_Frame is only
> cached when __libdwfl_frame_unwind is called. Which I believe isn't
> done for the initial frame. Also it isn't clear how to propagate any
> errors when NULL is returned. Maybe dwfl_frame_dwarf_frame () should
> check first to see if frame is NULL and then call lookup the CFI and
> call dwarf_cfi_addrframe if not?

Yes, that makes sense. Rather than doing the lookups in
__libdwfl_frame_unwind and handle_cfi, I can move the lookups to
dwfl_frame_module and dwfl_frame_dwarf_frame, have those functions cache
it internally, and make __libdwfl_frame_unwind and handle_cfi call those
functions.

Thanks,
Omar

Re: [PATCH 4/5] libdwfl: cache Dwfl_Module and Dwarf_Frame for Dwfl_Frame

[Mark Wielaard]

On Wed, 2019-10-30 at 16:55 -0700, Omar Sandoval wrote:
> On Wed, Oct 30, 2019 at 02:04:42PM +0100, Mark Wielaard wrote:
> > Hi Omar,
> > 
> > On Mon, 2019-10-07 at 02:05 -0700, Omar Sandoval wrote:
> > > The next change will need to have the Dwarf_Frame readily available, so
> > > rather than finding it again every time, let's cache it for reuse. The
> > > CFI frame can also be useful to clients of libdwfl, so add
> > > dwfl_frame_dwarf_frame to get it. Similarly, the Dwfl_Module is also
> > > frequently needed in conjunction with the frame, so cache it and add
> > > dwfl_frame_module.
> > 
> > I can see how this is useful. But it seems the Dwarf_Frame is only
> > cached when __libdwfl_frame_unwind is called. Which I believe isn't
> > done for the initial frame. Also it isn't clear how to propagate any
> > errors when NULL is returned. Maybe dwfl_frame_dwarf_frame () should
> > check first to see if frame is NULL and then call lookup the CFI and
> > call dwarf_cfi_addrframe if not?
> 
> Yes, that makes sense. Rather than doing the lookups in
> __libdwfl_frame_unwind and handle_cfi, I can move the lookups to
> dwfl_frame_module and dwfl_frame_dwarf_frame, have those functions cache
> it internally, and make __libdwfl_frame_unwind and handle_cfi call those
> functions.

dwfl_frame_dwarf_frame should also save any errors, so that when it is
called again it can set it again when returning NULL. Like what
dwfl_module_getdwarf does.

Thanks,

Mark

[PATCH 5/5] libdwfl: add interface for evaluating DWARF expressions in a frame

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

libdwfl can evaluate DWARF expressions in order to unwind the stack, but
this functionality isn't exposed to clients of the library. Now that the
pieces are in place, add dwfl_frame_eval_expr to provide this feature.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libdw/ChangeLog        |  1 +
 libdw/libdw.map        |  1 +
 libdwfl/ChangeLog      |  2 ++
 libdwfl/frame_unwind.c | 12 ++++++++++++
 libdwfl/libdwfl.h      |  6 ++++++
 5 files changed, 22 insertions(+)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 1a712497..765d51ec 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -5,6 +5,7 @@
 	Add dwfl_detach_thread.
 	Add dwfl_frame_module.
 	Add dwfl_frame_dwarf_frame.
+	Add dwfl_frame_eval_expr.
 
 2019-07-05  Omar Sandoval  <osandov@fb.com>
 
diff --git a/libdw/libdw.map b/libdw/libdw.map
index c1469c4e..d06a29ed 100644
--- a/libdw/libdw.map
+++ b/libdw/libdw.map
@@ -376,4 +376,5 @@ ELFUTILS_0.178 {
     dwfl_detach_thread;
     dwfl_frame_module;
     dwfl_frame_dwarf_frame;
+    dwfl_frame_eval_expr;
 } ELFUTILS_0.177;
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index fdb517c9..3689ddd9 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -20,10 +20,12 @@
 	(__libdwfl_frame_unwind): Cache state->mod.
 	(dwfl_frame_module): Add function.
 	(dwfl_frame_dwarf_frame): Add function.
+	(dwfl_frame_eval_expr): Add function.
 	* libdwfl.h (dwfl_attach_thread): Add definition.
 	(dwfl_detach_thread): Add definition.
 	(dwfl_frame_module): Add definition.
 	(dwfl_frame_dwarf_frame): Add definition.
+	(dwfl_frame_eval_expr): Add definition.
 	* libdwflP.h (struct Dwfl_Thread): Update comment for unwound member.
 	Add mod, frame, and bias.
 
diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c
index 9738ca72..4c2df837 100644
--- a/libdwfl/frame_unwind.c
+++ b/libdwfl/frame_unwind.c
@@ -787,3 +787,15 @@ dwfl_frame_dwarf_frame (Dwfl_Frame *state, Dwarf_Addr *bias)
   *bias = state->bias;
   return state->frame;
 }
+
+bool
+dwfl_frame_eval_expr (Dwfl_Frame *state, const Dwarf_Op *ops, size_t nops,
+		      Dwarf_Addr *result)
+{
+  if (state->frame == NULL)
+    {
+      __libdwfl_seterrno (DWFL_E_NO_DWARF);
+      return false;
+    }
+  return expr_eval (state, state->frame, ops, nops, result, state->bias);
+}
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index b450816b..90a3e103 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -829,6 +829,12 @@ int dwfl_getthread_frames (Dwfl *dwfl, pid_t tid,
 bool dwfl_frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool *isactivation)
   __nonnull_attribute__ (1, 2);
 
+/* Evaluate a DWARF expression in the context of a frame.  On success, returns
+   true and fills in *RESULT.  On error, returns false. */
+bool dwfl_frame_eval_expr (Dwfl_Frame *state, const Dwarf_Op *ops, size_t nops,
+			   Dwarf_Addr *result)
+  __nonnull_attribute__ (1, 2, 4);
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.23.0

Re: [PATCH 5/5] libdwfl: add interface for evaluating DWARF expressions in a frame

[Mark Wielaard]

Hi Omar,

On Mon, 2019-10-07 at 02:05 -0700, Omar Sandoval wrote:
> libdwfl can evaluate DWARF expressions in order to unwind the stack,
> but this functionality isn't exposed to clients of the library. Now that
> the pieces are in place, add dwfl_frame_eval_expr to provide this feature.

I think this is useful. But same issue as the previous patch that I am
not sure the error handling is correct for state->frame == NULL.

Also this could really use some examples and maybe a small testcase. 

That would show how to handle DWARF expressions to are simple location
descriptions (which calculate a location where a value can be found)
versus implicit location descriptions (e.g. DW_OP_value) versus
composite location descriptions (e.g. DW_OP_piece).

It might be that we don't care, because all we care about is whether or
not we can get a value, but it would validate the interface.

Having some examples/testcases would also show how/where to get the
DWARF expressions to use with this new function.

Thanks,

Mark

Re: [PATCH 5/5] libdwfl: add interface for evaluating DWARF expressions in a frame

[Omar Sandoval]

On Wed, Oct 30, 2019 at 02:23:06PM +0100, Mark Wielaard wrote:
> Hi Omar,
> 
> On Mon, 2019-10-07 at 02:05 -0700, Omar Sandoval wrote:
> > libdwfl can evaluate DWARF expressions in order to unwind the stack,
> > but this functionality isn't exposed to clients of the library. Now that
> > the pieces are in place, add dwfl_frame_eval_expr to provide this feature.
> 
> I think this is useful. But same issue as the previous patch that I am
> not sure the error handling is correct for state->frame == NULL.

The change I described for the previous patch should fix that.

> Also this could really use some examples and maybe a small testcase. 
> 
> That would show how to handle DWARF expressions to are simple location
> descriptions (which calculate a location where a value can be found)
> versus implicit location descriptions (e.g. DW_OP_value) versus
> composite location descriptions (e.g. DW_OP_piece).
> 
> It might be that we don't care, because all we care about is whether or
> not we can get a value, but it would validate the interface.
> 
> Having some examples/testcases would also show how/where to get the
> DWARF expressions to use with this new function.

Sounds good, I'll put some examples/test cases together. FWIW, I'm using
it in drgn to get register values like so:

LIBDRGN_PUBLIC struct drgn_error *
drgn_stack_frame_register(struct drgn_stack_frame frame,
			  enum drgn_register_number regno, uint64_t *ret)
{
	const Dwarf_Op op = { .atom = DW_OP_regx, .number = regno, };
	Dwarf_Addr value;

	if (!dwfl_frame_eval_expr(frame.trace->frames[frame.i], &op, 1, &value))
		return drgn_error_libdwfl();
	*ret = value;
	return NULL;
}

Later, I plan to use this for location expressions for local variables
that I get out of DWARF.

Thanks,
Omar

Re: [PATCH 5/5] libdwfl: add interface for evaluating DWARF expressions in a frame

[Mark Wielaard]

On Wed, 2019-10-30 at 16:59 -0700, Omar Sandoval wrote:
> On Wed, Oct 30, 2019 at 02:23:06PM +0100, Mark Wielaard wrote:
> > Having some examples/testcases would also show how/where to get the
> > DWARF expressions to use with this new function.
> 
> Sounds good, I'll put some examples/test cases together. FWIW, I'm using
> it in drgn to get register values like so:
> 
> LIBDRGN_PUBLIC struct drgn_error *
> drgn_stack_frame_register(struct drgn_stack_frame frame,
> 			  enum drgn_register_number regno, uint64_t *ret)
> {
> 	const Dwarf_Op op = { .atom = DW_OP_regx, .number = regno, };
> 	Dwarf_Addr value;
> 
> 	if (!dwfl_frame_eval_expr(frame.trace->frames[frame.i], &op, 1, &value))
> 		return drgn_error_libdwfl();
> 	*ret = value;
> 	return NULL;
> }

O! I hadn't even thought of that. Funny using it with "hand
constructed" DWARF expressions.

> Later, I plan to use this for location expressions for local variables
> that I get out of DWARF.

Yes, that was what I assumed you were using it for already.
I think it will work for that. But it would be nice to have some
code/example that actually does it.

Thanks,

Mark

[PATCH 2/5] libdwfl: only use thread->unwound for initial frame

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

thread->unwound is only used for set_initial_registers (via
dwfl_thread_state_registers, dwfl_thread_state_register_pc, and a
special case in core_set_initial_registers). At that point,
thread->unwound is always the initial frame, so there's no need to
update it as we unwind the stack. Let's set it to NULL after we do the
initial setup. This simplifies the next change.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libdwfl/ChangeLog    |  5 +++++
 libdwfl/dwfl_frame.c | 48 ++++++++++++++++++--------------------------
 libdwfl/libdwflP.h   |  3 +--
 3 files changed, 25 insertions(+), 31 deletions(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index f4a3cad9..07a1e8df 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -3,6 +3,11 @@
 	* dwfl_frame.c (dwfl_getthreads): Get rid of unnecessary
 	thread_free_all_states calls.
 	(getthread): Ditto.
+	(state_free): Remove function.
+	(thread_free_all_states): Remove function.
+	(free_states): Add function.
+	(dwfl_thread_getframes): Don't update thread->unwound while unwinding.
+	* libdwflP.h (struct Dwfl_Thread): Update comment for unwound member.
 
 2019-08-12  Mark Wielaard  <mark@klomp.org>
 
diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index 20bdbd9b..5bbf850e 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -71,19 +71,14 @@ state_fetch_pc (Dwfl_Frame *state)
 /* Do not call it on your own, to be used by thread_* functions only.  */
 
 static void
-state_free (Dwfl_Frame *state)
+free_states (Dwfl_Frame *state)
 {
-  Dwfl_Thread *thread = state->thread;
-  assert (thread->unwound == state);
-  thread->unwound = state->unwound;
-  free (state);
-}
-
-static void
-thread_free_all_states (Dwfl_Thread *thread)
-{
-  while (thread->unwound)
-    state_free (thread->unwound);
+  while (state)
+    {
+      Dwfl_Frame *next = state->unwound;
+      free(state);
+      state = next;
+    }
 }
 
 static Dwfl_Frame *
@@ -399,12 +394,6 @@ dwfl_thread_getframes (Dwfl_Thread *thread,
 		       int (*callback) (Dwfl_Frame *state, void *arg),
 		       void *arg)
 {
-  if (thread->unwound != NULL)
-    {
-      /* We had to be called from inside CALLBACK.  */
-      __libdwfl_seterrno (DWFL_E_ATTACH_STATE_CONFLICT);
-      return -1;
-    }
   Ebl *ebl = thread->process->ebl;
   if (ebl_frame_nregs (ebl) == 0)
     {
@@ -420,33 +409,34 @@ dwfl_thread_getframes (Dwfl_Thread *thread,
   if (! process->callbacks->set_initial_registers (thread,
 						   thread->callbacks_arg))
     {
-      thread_free_all_states (thread);
+      free_states (thread->unwound);
+      thread->unwound = NULL;
       return -1;
     }
-  if (! state_fetch_pc (thread->unwound))
+  Dwfl_Frame *state = thread->unwound;
+  thread->unwound = NULL;
+  if (! state_fetch_pc (state))
     {
       if (process->callbacks->thread_detach)
 	process->callbacks->thread_detach (thread, thread->callbacks_arg);
-      thread_free_all_states (thread);
+      free_states (state);
       return -1;
     }
-
-  Dwfl_Frame *state;
   do
     {
-      state = thread->unwound;
       int err = callback (state, arg);
       if (err != DWARF_CB_OK)
 	{
 	  if (process->callbacks->thread_detach)
 	    process->callbacks->thread_detach (thread, thread->callbacks_arg);
-	  thread_free_all_states (thread);
+	  free_states (state);
 	  return err;
 	}
       __libdwfl_frame_unwind (state);
+      Dwfl_Frame *next = state->unwound;
       /* The old frame is no longer needed.  */
-      state_free (thread->unwound);
-      state = thread->unwound;
+      free (state);
+      state = next;
     }
   while (state && state->pc_state == DWFL_FRAME_STATE_PC_SET);
 
@@ -455,12 +445,12 @@ dwfl_thread_getframes (Dwfl_Thread *thread,
     process->callbacks->thread_detach (thread, thread->callbacks_arg);
   if (state == NULL || state->pc_state == DWFL_FRAME_STATE_ERROR)
     {
-      thread_free_all_states (thread);
+      free_states (state);
       __libdwfl_seterrno (err);
       return -1;
     }
   assert (state->pc_state == DWFL_FRAME_STATE_PC_UNDEFINED);
-  thread_free_all_states (thread);
+  free_states (state);
   return 0;
 }
 INTDEF(dwfl_thread_getframes)
diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
index 941a8b66..6b2d4867 100644
--- a/libdwfl/libdwflP.h
+++ b/libdwfl/libdwflP.h
@@ -242,8 +242,7 @@ struct Dwfl_Thread
 {
   Dwfl_Process *process;
   pid_t tid;
-  /* The current frame being unwound.  Initially it is the bottom frame.
-     Later the processed frames get freed and this pointer is updated.  */
+  /* Bottom (innermost) frame while we're initializing, NULL afterwards.  */
   Dwfl_Frame *unwound;
   void *callbacks_arg;
 };
-- 
2.23.0

Re: [PATCH 2/5] libdwfl: only use thread->unwound for initial frame

[Mark Wielaard]

Hi Omar,

On Mon, Oct 07, 2019 at 02:05:36AM -0700, Omar Sandoval wrote:
> thread->unwound is only used for set_initial_registers (via
> dwfl_thread_state_registers, dwfl_thread_state_register_pc, and a
> special case in core_set_initial_registers). At that point,
> thread->unwound is always the initial frame, so there's no need to
> update it as we unwind the stack. Let's set it to NULL after we do the
> initial setup. This simplifies the next change.

Very nice cleanup. Pushed to master.

Thanks,

Mark