This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Tue, 22 Oct 2013 23:31:50 +0200, Mark Wielaard wrote:
> I meant you wrote it all, I only nitpick. You really don't need to
> credit me for the code, just for making your life difficult with all my
> nitpicking. Nitpicking-by line would be fine of course :)

You have provided even some code in some patch but OK, removed you.


> > There are various ways how to solve it but none of them find as a clear win:
> >  * As is.
> >  * New backend ebl method like normalize_reg() which cuts b32..b63 on i386.
> >    For example s390 backend later will have normalize_pc() to cut b31.
> >  * New ebl_get_class().
> 
> We already have the last solution. ebl_get_elfclass ().

I was somehow searching for it but did not find it.  I have used it now.
Therefore the Makefile.am libasm/ -I and also libeblP.h in libdwflP.h are
gone.


> I would just call it when constructing the Dwfl_Process and store it
> there since we are reusing it every time. Then the above simply becomes
> if (process->class == ELFCLASS32)

I do not think such microoptimizations are worth it nowadays.

Moreover I even removed the inline definition and changed it to normal
declaration (with definition in libdwfl/frame_unwind.c).  gcc -flto should do
such kind of stuff (it degrades performance in my tests but thats offtopic).

I hope that's fine with you.


> > I wanted to make each object type self-contained, this is destructor of
> > Dwfl_Frame.  Mixing it with Dwfl_Thread object seems not right for OO design.
> > 
> > Do you still want that change?
> 
> Since that same pattern is used 5 times in the same file I do think it
> makes sense to put it in its own function.

Put in libdwfl/dwfl_frame.c a new function:

static void
thread_free_all_states (Dwfl_Thread *thread)
{
  while (thread->unwound)
    state_free (thread->unwound);
}


> state_free on the other hand is never called on its own,

It was not but currently it is dwfl_thread_getframes() after freeing the older
frames now.


> > > > +/* Allocate new Dwfl_Process for DWFL.  */
> > > > +static void
> > > > +process_alloc (Dwfl *dwfl)
> > > > +{
> > > > +  Dwfl_Process *process = malloc (sizeof (*process));
> > > > +  if (process == NULL)
> > > > +    return;
> > > > +  process->dwfl = dwfl;
> > > > +  process->thread = NULL;
> > > > +  dwfl->process = process;
> > > > +}
> > > 
> > > I think this would be slightly clearer if it returned the Dwfl_Process
> > > pointer (or NULL on failure). But not a big deal.
> > 
> > Just the caller would have to assert (process == dwfl->process); anyway so
> > I found it easier this way.
> 
> I would have written it by removing the last line and change the caller
> to:
> 
> dwfl->process = process_alloc (dwfl);
> if (dwfl->process == NULL)
>   ...
> 
> But the above is fine too, just different styles.

I find fragile that it creates double-reference (dwfl<->process), therefore
creating such double-reference at two places...


> > thread_alloc() always works, both for allocating first thread and the last
> > thread.  It asserts you do not pass prev_thread as a non-last one.
> 
> hmmm, I don't like that design, it seems to mean we cannot rewalk the
> thread list after the first time. But that is the part we would discuss
> separately.

thread_alloc() no longer exists now, Dwfl_Thread now exists always only as one
instance of local variable in dwfl_getthreads().


> > > > diff --git a/libdwfl/dwfl_frame_pc.c b/libdwfl/dwfl_frame_pc.c
> > > > +bool
> > > > +dwfl_frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool *isactivation)
> > > > +{
> > > > +  assert (state->pc_state == DWFL_FRAME_STATE_PC_SET);
> > > > +  *pc = state->pc;
> > > > +  if (isactivation)
> > > > +    {
> > > > +      /* Bottom frame?  */
> > > > +      if (state == state->thread->unwound)
> > > > +	*isactivation = true;
> > > > +      /* *ISACTIVATION is logical or of current and previous frame state.  */
> > > > +      else if (state->signal_frame)
> > > > +	*isactivation = true;
> > > > +      else
> > > > +	{
> > > > +	  /* Not affected by unsuccessfully unwound frame.  */
> > > > +	  __libdwfl_frame_unwind (state);
> > > > +	  if (state->unwound == NULL
> > > > +	      || state->unwound->pc_state != DWFL_FRAME_STATE_PC_SET)
> > > > +	    *isactivation = false;
> > > > +	  else
> > > > +	    *isactivation = state->unwound->signal_frame;
> > > > +	}
> > > 
> > > I don't understand this last else block part where isactivation depends
> > > on the unwound state. Could you add a comment explaining or a reference
> > > to some document that defines this part?
> > 
> > I considered it was documented by these two lines:
> > 
> > > > +      /* *ISACTIVATION is logical or of current and previous frame state.  */
> > > > +     /* Not affected by unsuccessfully unwound frame.  */
> > 
> > Is this expansion of comments good enough now?
> > 
> >       /* Bottom frame?  */
> >       if (state == state->thread->unwound)
> >         *isactivation = true;
> >       /* *ISACTIVATION is logical union of whether current or previous frame
> >          state is SIGNAL_FRAME.  */
> >       else if (state->signal_frame)
> >         *isactivation = true;
> >       else
> >         {
> >           /* If the previous frame has unwound unsuccessfully just silently do
> >              not consider it could be a SIGNAL_FRAME.  */
> >           __libdwfl_frame_unwind (state);
> >           if (state->unwound == NULL
> >               || state->unwound->pc_state != DWFL_FRAME_STATE_PC_SET)
> >             *isactivation = false;
> >           else
> >             *isactivation = state->unwound->signal_frame;
> >         }

------------------------------------------------------------------------------
(gdb) l 1
1	#include <signal.h>
2	#include <unistd.h>
3	void f(int signo) {
4	}
5	int main(void) {
6	  signal(SIGINT,f);
7	  pause ();
8	  return 0;
9	}
(gdb) b f
Breakpoint 1 at 0x400587: file sigint.c, line 4.
(gdb) r
Starting program: sigint 
<ctrl-c>
Program received signal SIGINT, Interrupt.
0x00000030304bd330 in __pause_nocancel () at ../sysdeps/unix/syscall-template.S:81
81	T_PSEUDO (SYSCALL_SYMBOL, SYSCALL_NAME, SYSCALL_NARGS)
(gdb) signal SIGINT
Continuing with signal SIGINT.
Breakpoint 1, f (signo=2) at sigint.c:4
4	}
(gdb) bt
#0  f (signo=2) at sigint.c:4
#1  <signal handler called>
#2  0x00000030304bd330 in __pause_nocancel () at ../sysdeps/unix/syscall-template.S:81
#3  0x00000000004005a1 in main () at sigint.c:7
(gdb) up
#1  <signal handler called>
(gdb) disas
Dump of assembler code for function __restore_rt:
=> 0x0000003030435a90 <+0>:	mov    $0xf,%rax
   0x0000003030435a97 <+7>:	syscall 
   0x0000003030435a99 <+9>:	nopl   0x0(%rax)
End of assembler dump.
------------------------------------------------------------------------------
> 
> I think I just don't fully understand the semantics of the 'S'
> augmentation. That is an eh_frame CFI extension, not described in the
> DWARF spec. I understand that if this is a signal_frame then the PC
> doesn't need to be adjusted.

state->signal_frame is true only for frame #1.  I rather do not see so clear
why we should not adjust PC for the signal frame itself.  But as one can see
we must not adjust PC for the signal frame as we are on very first instruction
of __restore_rt.


> But why don't we have adjust the PC for
> this frame if we were called from a signal_frame?

We are unwinding the opposite direction so we are determining whether to
adjust PC for the frame that "called" (was interrupted by) the signal frame.
The frame called from a signal frame (#0) is already processed before we ever
found there is some signal frame and that one is unrelated.

I hope it is clear why the frame unwound from a signal frame (#2) must not
have adjusted PC.


> > Some CFI-valid OPs are really missing, they just were not in the critical-path
> > initial post.
> 
> IMHO it would be really good if we listed the known missing ones in one
> (fall-through) case statement that is UNSUPPORTED_DWARF and make the
> default INVALID_DWARF. That way we also have a concrete TODO list.

Put into libdwfl/frame_unwind.c:
      // case DW_OP_addr:
      // case DW_OP_GNU_encoded_addr:
      // case DW_OP_pick:
      // case DW_OP_over:
      // case DW_OP_swap:
      // case DW_OP_rot:
      // case DW_OP_deref_size:
      // case DW_OP_abs:
      // case DW_OP_neg:
      // case DW_OP_not:
      // case DW_OP_div:
      // case DW_OP_minus:
      // case DW_OP_mod:
      // case DW_OP_or:
      // case DW_OP_shr:
      // case DW_OP_shra:
      // case DW_OP_xor:


> > > > +  if (state != state->thread->unwound && ! state->signal_frame)
> > > > +    pc--;
> > > > +  Dwfl_Module *mod = INTUSE(dwfl_addrmodule) (state->thread->process->dwfl, pc);
> > > > +  if (mod != NULL)
> > > > +    {
> > > > +      Dwarf_Addr bias;
> > > > +      Dwarf_CFI *cfi_eh = INTUSE(dwfl_module_eh_cfi) (mod, &bias);
> > > > +      if (cfi_eh)
> > > > +	{
> > > > +	  handle_cfi (state, pc - bias, cfi_eh);
> > > > +	  if (state->unwound)
> > > > +	    return;
> > > > +	}
> > > > +      Dwarf_CFI *cfi_dwarf = INTUSE(dwfl_module_dwarf_cfi) (mod, &bias);
> > > > +      if (cfi_dwarf)
> > > > +	{
> > > > +	  handle_cfi (state, pc - bias, cfi_dwarf);
> > > > +	  if (state->unwound)
> > > > +	    return;
> > > > +	}
> > > > +    }
> > > > +  __libdwfl_seterrno (DWFL_E_NO_DWARF);
> > > > +}
> > > 
> > > handle_cfi might set dwflerrno when state->unwound == NULL, in that case
> > > we probably shouldn't override it with the generic DWFL_E_NO_DWARF,
> > > might want to put that in the else branch of (mod != NULL)?
> > 
> > The reason is that while the idea of DWFL_FRAME_STATE_PC_UNDEFINED terminating
> > the backtrace is nice in real world it does not happen.  It was (I have) only
> > recently fixed it for x86* and non-x86* archs may never be fixed.  So the
> > decision is left up to the caller whether DWFL_E_NO_DWARF is considered as
> > a proper backtrace terminator.  tests/run-backtrace.sh has for it:
> >   # In some cases we cannot reliably find out we got behind _start.
> >   if cmp -s <(echo "${abs_builddir}/backtrace: dwfl_thread_getframes: No DWARF information found") <(uniq <$1); then
> > 
> > There may be multiple different error codes if CFI is not found, at least
> > besides DWFL_E_NO_DWARF also DWFL_E_NO_MATCH.  So that DWFL_E_NO_DWARF is
> > rather a part of API.
> 
> hmmm, that doesn't really work since the DWFL_E_ error codes are private
> (libdwflP.h). You can get an error string describing the error, but not
> really compare them easily. An error is an error as far as the user is
> concerned.
> 
> Since we cannot guarantee unwinding finishes cleanly, the user just has
> to accept that unwinding often just ends in an error. Ugly, but
> unavoidable I am afraid. We will just have to work towards making the
> whole toolchain DWARF unwinding "clean" eventually (some far future
> dream). At least DWARF4 6.4.4 seems very clear on how to mark the end of
> an unwind.

OK, true, in such case I have removed the DWFL_E_NO_DWARF override and updated
the testcase:
   # In some cases we cannot reliably find out we got behind _start.
-  if cmp -s <(echo "${abs_builddir}/backtrace: dwfl_thread_getframes: No DWARF information found") <(uniq <$1); then
+  if cmp -s <(echo "${abs_builddir}/backtrace: dwfl_thread_getframes: no matching address range") <(uniq <$1); then

Updated dwfl_thread_getframes comment to:
/* 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 
   set dwfl_errno ().  Some systems return error instead of zero on end of the 
   backtrace, for cross-platform compatibility callers should consider error as
   a zero.  Keeps calling the callback with the next frame while the callback
...


> > 
> > +	if (process->callbacks->memory_read == NULL)
> > +	  {
> > +	    __libdwfl_seterrno (DWFL_E_LIBDW);
> > +	    free (stack);
> > +	    return false;
> > +	  }
> > [...]
> > +    {
> > +      if (process->callbacks->memory_read == NULL)
> > +	{
> > +	  __libdwfl_seterrno (DWFL_E_LIBDW);
> > +	  return false;
> > +	}
> 
> DWFL_E_LIBDW isn't the right error code here since that will lookup
> dwarf_errno (see dwfl_error.c). DWFL_E_INVALID_ARGUMENT maybe?

Yes, DWFL_E_INVALID_ARGUMENT is now used for missing callbacks at the top of
dwfl_attach_state().


Thanks,
Jan

diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index 2810c6a..0806678 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -76,7 +76,12 @@ state_free (Dwfl_Frame *state)
   free (state);
 }
 
-/* Do not call it on your own, to be used by thread_* functions only.  */
+static void
+thread_free_all_states (Dwfl_Thread *thread)
+{
+  while (thread->unwound)
+    state_free (thread->unwound);
+}
 
 static Dwfl_Frame *
 state_alloc (Dwfl_Thread *thread)
@@ -238,23 +243,20 @@ dwfl_getthreads (Dwfl *dwfl, int (*callback) (Dwfl_Thread *thread, void *arg),
       if (thread.tid < 0)
 	{
 	  Dwfl_Error saved_errno = dwfl_errno ();
-	  while (thread.unwound)
-	    state_free (thread.unwound);
+	  thread_free_all_states (&thread);
 	  __libdwfl_seterrno (saved_errno);
 	  return -1;
 	}
       if (thread.tid == 0)
 	{
-	  while (thread.unwound)
-	    state_free (thread.unwound);
+	  thread_free_all_states (&thread);
 	  __libdwfl_seterrno (DWFL_E_NOERROR);
 	  return 0;
 	}
       int err = callback (&thread, arg);
       if (err != DWARF_CB_OK)
 	{
-	  while (thread.unwound)
-	    state_free (thread.unwound);
+	  thread_free_all_states (&thread);
 	  return err;
 	}
     }
@@ -288,16 +290,14 @@ dwfl_thread_getframes (Dwfl_Thread *thread,
   if (! process->callbacks->set_initial_registers (thread,
 						   thread->callbacks_arg))
     {
-      while (thread->unwound)
-	state_free (thread->unwound);
+      thread_free_all_states (thread);
       return -1;
     }
   if (! state_fetch_pc (thread->unwound))
     {
       if (process->callbacks->thread_detach)
 	process->callbacks->thread_detach (thread, thread->callbacks_arg);
-      while (thread->unwound)
-	state_free (thread->unwound);
+      thread_free_all_states (thread);
       return -1;
     }
 
@@ -310,8 +310,7 @@ dwfl_thread_getframes (Dwfl_Thread *thread,
 	{
 	  if (process->callbacks->thread_detach)
 	    process->callbacks->thread_detach (thread, thread->callbacks_arg);
-	  while (thread->unwound)
-	    state_free (thread->unwound);
+	  thread_free_all_states (thread);
 	  return err;
 	}
       __libdwfl_frame_unwind (state);
@@ -324,8 +323,7 @@ dwfl_thread_getframes (Dwfl_Thread *thread,
   Dwfl_Error err = dwfl_errno ();
   if (process->callbacks->thread_detach)
     process->callbacks->thread_detach (thread, thread->callbacks_arg);
-  while (thread->unwound)
-    state_free (thread->unwound);
+  thread_free_all_states (thread);
   if (state == NULL || state->pc_state == DWFL_FRAME_STATE_ERROR)
     {
       __libdwfl_seterrno (err);
diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c
index e834cbf..e80c063 100644
--- a/libdwfl/frame_unwind.c
+++ b/libdwfl/frame_unwind.c
@@ -140,6 +140,32 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op *ops,
   for (const Dwarf_Op *op = ops; op < ops + nops; op++)
     switch (op->atom)
     {
+      /* DW_OP_* order matches libgcc/unwind-dw2.c execute_stack_op:  */
+      case DW_OP_lit0 ... DW_OP_lit31:
+	if (! push (op->atom - DW_OP_lit0))
+	  {
+	    free (stack);
+	    return false;
+	  }
+	break;
+      // case DW_OP_addr:
+      // case DW_OP_GNU_encoded_addr:
+      case DW_OP_const1u:
+      case DW_OP_const1s:
+      case DW_OP_const2u:
+      case DW_OP_const2s:
+      case DW_OP_const4u:
+      case DW_OP_const4s:
+      case DW_OP_const8u:
+      case DW_OP_const8s:
+      case DW_OP_constu:
+      case DW_OP_consts:
+	if (! push (op->number))
+	  {
+	    free (stack);
+	    return false;
+	  }
+	break;
       case DW_OP_reg0 ... DW_OP_reg31:
 	if (! state_get_reg (state, op->atom - DW_OP_reg0, &val1)
 	    || ! push (val1))
@@ -181,43 +207,29 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op *ops,
 	    return false;
 	  }
 	break;
-      case DW_OP_lit0 ... DW_OP_lit31:
-	if (! push (op->atom - DW_OP_lit0))
-	  {
-	    free (stack);
-	    return false;
-	  }
-	break;
-      case DW_OP_plus_uconst:
-	if (! pop (&val1) || ! push (val1 + op->number))
+      case DW_OP_dup:
+	if (! pop (&val1) || ! push (val1) || ! push (val1))
 	  {
 	    free (stack);
 	    return false;
 	  }
 	break;
-      case DW_OP_call_frame_cfa:;
-	Dwarf_Op *cfa_ops;
-	size_t cfa_nops;
-	Dwarf_Addr cfa;
-	if (frame == NULL
-	    || dwarf_frame_cfa (frame, &cfa_ops, &cfa_nops) != 0
-	    || ! expr_eval (state, NULL, cfa_ops, cfa_nops, &cfa)
-	    || ! push (cfa))
+      case DW_OP_drop:
+	if (! pop (&val1))
 	  {
-	    __libdwfl_seterrno (DWFL_E_LIBDW);
 	    free (stack);
 	    return false;
 	  }
-	is_location = true;
-	break;
-      case DW_OP_stack_value:
-	is_location = false;
 	break;
+      // case DW_OP_pick:
+      // case DW_OP_over:
+      // case DW_OP_swap:
+      // case DW_OP_rot:
       case DW_OP_deref:
 	if (process->callbacks->memory_read == NULL)
 	  {
-	    __libdwfl_seterrno (DWFL_E_LIBDW);
 	    free (stack);
+	    __libdwfl_seterrno (DWFL_E_INVALID_ARGUMENT);
 	    return false;
 	  }
 	if (! pop (&val1)
@@ -229,31 +241,53 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op *ops,
 	    return false;
 	  }
 	break;
-      case DW_OP_nop:
-	break;
-      case DW_OP_dup:
-	if (! pop (&val1) || ! push (val1) || ! push (val1))
+      // case DW_OP_deref_size:
+      // case DW_OP_abs:
+      // case DW_OP_neg:
+      // case DW_OP_not:
+      case DW_OP_plus_uconst:
+	if (! pop (&val1) || ! push (val1 + op->number))
 	  {
 	    free (stack);
 	    return false;
 	  }
 	break;
-      case DW_OP_const1u:
-      case DW_OP_const1s:
-      case DW_OP_const2u:
-      case DW_OP_const2s:
-      case DW_OP_const4u:
-      case DW_OP_const4s:
-      case DW_OP_const8u:
-      case DW_OP_const8s:
-      case DW_OP_constu:
-      case DW_OP_consts:
-	if (! push (op->number))
-	  {
-	    free (stack);
-	    return false;
-	  }
+#define BINOP(atom, op)							\
+      case atom:							\
+	if (! pop (&val2) || ! pop (&val1) || ! push (val1 op val2))	\
+	  {								\
+	    free (stack);						\
+	    return false;						\
+	  }								\
+	break;
+      BINOP (DW_OP_and, &)
+      // case DW_OP_div:
+      // case DW_OP_minus:
+      // case DW_OP_mod:
+      BINOP (DW_OP_mul, *)
+      // case DW_OP_or:
+      BINOP (DW_OP_plus, +)
+      BINOP (DW_OP_shl, <<)
+#undef BINOP
+      // case DW_OP_shr:
+      // case DW_OP_shra:
+      // case DW_OP_xor:
+#define BINOP_SIGNED(atom, op)						\
+      case atom:							\
+	if (! pop (&val2) || ! pop (&val1)				\
+	    || ! push ((int64_t) val1 op (int64_t) val2))		\
+	  {								\
+	    free (stack);						\
+	    return false;						\
+	  }								\
 	break;
+      BINOP_SIGNED (DW_OP_le, <=)
+      BINOP_SIGNED (DW_OP_ge, >=)
+      BINOP_SIGNED (DW_OP_eq, ==)
+      BINOP_SIGNED (DW_OP_lt, <)
+      BINOP_SIGNED (DW_OP_gt, >)
+      BINOP_SIGNED (DW_OP_ne, !=)
+#undef BINOP_SIGNED
       case DW_OP_bra:
 	if (! pop (&val1))
 	  {
@@ -277,42 +311,29 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op *ops,
 	/* Undo the 'for' statement increment.  */
 	op = found - 1;
 	break;
-      case DW_OP_drop:
-	if (! pop (&val1))
+      case DW_OP_nop:
+	break;
+      /* DW_OP_* not listed in libgcc/unwind-dw2.c execute_stack_op:  */
+      case DW_OP_call_frame_cfa:;
+	// Not used by CFI itself but it is synthetized by elfutils internation.
+	Dwarf_Op *cfa_ops;
+	size_t cfa_nops;
+	Dwarf_Addr cfa;
+	if (frame == NULL
+	    || dwarf_frame_cfa (frame, &cfa_ops, &cfa_nops) != 0
+	    || ! expr_eval (state, NULL, cfa_ops, cfa_nops, &cfa)
+	    || ! push (cfa))
 	  {
+	    __libdwfl_seterrno (DWFL_E_LIBDW);
 	    free (stack);
 	    return false;
 	  }
+	is_location = true;
 	break;
-#define BINOP(atom, op)							\
-      case atom:							\
-	if (! pop (&val2) || ! pop (&val1) || ! push (val1 op val2))	\
-	  {								\
-	    free (stack);						\
-	    return false;						\
-	  }								\
-	break;
-      BINOP (DW_OP_and, &)
-      BINOP (DW_OP_shl, <<)
-      BINOP (DW_OP_plus, +)
-      BINOP (DW_OP_mul, *)
-#undef BINOP
-#define BINOP_SIGNED(atom, op)						\
-      case atom:							\
-	if (! pop (&val2) || ! pop (&val1)				\
-	    || ! push ((int64_t) val1 op (int64_t) val2))		\
-	  {								\
-	    free (stack);						\
-	    return false;						\
-	  }								\
+      case DW_OP_stack_value:
+	// Not used by CFI itself but it is synthetized by elfutils internation.
+	is_location = false;
 	break;
-      BINOP_SIGNED (DW_OP_le, <=)
-      BINOP_SIGNED (DW_OP_ge, >=)
-      BINOP_SIGNED (DW_OP_eq, ==)
-      BINOP_SIGNED (DW_OP_lt, <)
-      BINOP_SIGNED (DW_OP_gt, >)
-      BINOP_SIGNED (DW_OP_ne, !=)
-#undef BINOP_SIGNED
       default:
 	__libdwfl_seterrno (DWFL_E_UNSUPPORTED_DWARF);
 	return false;
@@ -327,7 +348,7 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op *ops,
     {
       if (process->callbacks->memory_read == NULL)
 	{
-	  __libdwfl_seterrno (DWFL_E_LIBDW);
+	  __libdwfl_seterrno (DWFL_E_INVALID_ARGUMENT);
 	  return false;
 	}
       if (! process->callbacks->memory_read (process->dwfl, *result, result,
@@ -441,7 +462,9 @@ __libdwfl_frame_unwind (Dwfl_Frame *state)
   if (! state->initial_frame && ! state->signal_frame)
     pc--;
   Dwfl_Module *mod = INTUSE(dwfl_addrmodule) (state->thread->process->dwfl, pc);
-  if (mod != NULL)
+  if (mod == NULL)
+    __libdwfl_seterrno (DWFL_E_NO_DWARF);
+  else
     {
       Dwarf_Addr bias;
       Dwarf_CFI *cfi_eh = INTUSE(dwfl_module_eh_cfi) (mod, &bias);
@@ -459,5 +482,4 @@ __libdwfl_frame_unwind (Dwfl_Frame *state)
 	    return;
 	}
     }
-  __libdwfl_seterrno (DWFL_E_NO_DWARF);
 }
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index 4aa1f0a..83cabb8 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -676,16 +676,14 @@ int dwfl_getthreads (Dwfl *dwfl,
   __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
-   value of the callback when not DWARF_CB_OK.  -1 returned on error will set
-   dwfl_errno ().  DWFL_E_NO_DWARF means no further unwind information was
-   found, which is up to the caller whether it is considered the same as
-   returned zero; some backtraced systems will return value -1 and
-   DWFL_E_NO_DWARF instead of the more correct value zero.  Keeps calling the
-   callback with the next frame while the callback returns DWARF_CB_OK, till
-   there are no more frames.  On start will call the set_initial_registers
-   callback and on return will call the detach_thread callback of the
-   Dwfl_Thread.  */
+   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 ().  Some systems return error instead of zero on end of the
+   backtrace, for cross-platform compatibility callers should consider error as
+   a zero.  Keeps calling the callback with the next frame while the callback
+   returns DWARF_CB_OK, till there are no more frames.  On start will call the
+   set_initial_registers 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)
diff --git a/tests/run-backtrace.sh b/tests/run-backtrace.sh
index f09e772..3f77da3 100755
--- a/tests/run-backtrace.sh
+++ b/tests/run-backtrace.sh
@@ -57,7 +57,7 @@ check_err()
     return
   fi
   # In some cases we cannot reliably find out we got behind _start.
-  if cmp -s <(echo "${abs_builddir}/backtrace: dwfl_thread_getframes: No DWARF information found") <(uniq <$1); then
+  if cmp -s <(echo "${abs_builddir}/backtrace: dwfl_thread_getframes: no matching address range") <(uniq <$1); then
     return
   fi
   cat >&2 $1

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]