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

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

On Fri, 25 Oct 2013 16:56:23 +0200, Mark Wielaard wrote:
> Yes, now all the comments and the code make sense. Sorry for all the
> confusion. This is tricky. Thanks for the example.

I agree with all your conclusions about signal frame.


> I would really like it if these were all just in one block and had:
> 
>         __libdwfl_seterrno (DWFL_E_UNSUPPORTED_DWARF);
>         return false;
> 
> And then change the default: to:
> 
>         __libdwfl_seterrno (DWFL_E_INVALID_DWARF);
>         return false;
> 
> That makes it slightly easier to see if it is "our fault" or bad unwind
> info.

I have implemented all the missing ops except for:
+      case DW_OP_GNU_encoded_addr:
+       /* Missing support in the rest of elfutils.  */
+      case DW_OP_deref_size:
+       /* Missing appropriate callback.  */

BTW elfutils should really get DW_OP_GNU_encoded_addr implemented I guess as it
may appear in CFI.


> > > > > > +  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
> 
> Sorry for a late suggestion, but I only thought of it now. Before I
> didn't fully appreciate why you wanted to see the difference between
> "real error" and "no information available".
> 
> Since dwfl_thread_getframes returns an int, could we change it to:

Sorry but I do not think it matters too much to differentiate
CFI-interpretation-error vs. CFI-not-found-error.  In most cases we end up on
CFI-not-found-error because the inferior is not come from RH-distro, therefore
it does not have -fasynchronous-unwind-tables, and it is stripped, therefore it
does not have .debug_frame.  And it is a bug, not end-of-unwind.

For the cases where we end up with CFI-not-found-error where the backtrace
should have been really finished we should fix the inferior CFI.  We are on
Free OS so we can do so.

CFI-interpretation-error will not happen in practice I think to have to deal
with it somehow.

I understand your patch makes sense but I think it rather needlessly
complicates the situation.  Do you still want to get it applied?


> 
> /* Iterate through the frames for a thread.  Returns zero if all
>    frames have been processed successfully by the callback.  Returns
>    1 when there are no more frames because there is no unwind
>    information found for the last frame's PC value,
>    returns -1 on error, or the value of the callback when not
>    DWARF_CB_OK.  When the return value is -1 or 1 will set
>    dwfl_errno () to indicate why unwinding stopped.  dwfl_errno will be
>    zero if the callback returned something different from DWARF_CB_OK.
>    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.  */
> 
> That way you can more easily decide whether to report an error or not.
> -1 is a "true" error. 1 is most likely not a real error (though it could
> be).
> 
> Then in dwfl_thread_getframes you would just do:
> 
>   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);
>       __libdwfl_seterrno (DWFL_E_NOERROR);
>       return err;
>     }
> 
> and
> 
>   if (state == NULL || state->pc_state == DWFL_FRAME_STATE_ERROR)
>     {
>       __libdwfl_seterrno (err);
>       return (err == DWFL_E_NO_DWARF
>               || (err == DWFL_E_ADDR_OUTOFRANGE) ? 1 :-1;
>     }
> 
> Then we do need to notice the DWARF_E_NO_MATCH in handle_cfi and
> transform it to a DWFL_E_NO_MATCH with the following trick:
> 
>    if (INTUSE(dwarf_cfi_addrframe) (cfi, pc, &frame) != 0)
>     {
> 
>       int err = INTUSE(dwarf_errno) ();
>       __libdwfl_seterrno (DWARF_E_NO_MATCH
> 		     	  ? DWFL_E_NO_MATCH : DWFL_E (LIBDW, err));
>       return;
>     }


Thanks,
Jan

diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c
index e80c063..eca7a04 100644
--- a/libdwfl/frame_unwind.c
+++ b/libdwfl/frame_unwind.c
@@ -94,7 +94,7 @@ bra_compar (const void *key_voidp, const void *elem_voidp)
 
 static bool
 expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op *ops,
-	   size_t nops, Dwarf_Addr *result)
+	   size_t nops, Dwarf_Addr *result, Dwarf_Addr bias)
 {
   Dwfl_Process *process = state->thread->process;
   if (nops == 0)
@@ -148,8 +148,17 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op *ops,
 	    return false;
 	  }
 	break;
-      // case DW_OP_addr:
-      // case DW_OP_GNU_encoded_addr:
+      case DW_OP_addr:
+	if (! push (op->number + bias))
+	  {
+	    free (stack);
+	    return false;
+	  }
+	break;
+      case DW_OP_GNU_encoded_addr:
+	/* Missing support in the rest of elfutils.  */
+	__libdwfl_seterrno (DWFL_E_UNSUPPORTED_DWARF);
+	return false;
       case DW_OP_const1u:
       case DW_OP_const1s:
       case DW_OP_const2u:
@@ -221,10 +230,40 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op *ops,
 	    return false;
 	  }
 	break;
-      // case DW_OP_pick:
-      // case DW_OP_over:
-      // case DW_OP_swap:
-      // case DW_OP_rot:
+      case DW_OP_pick:
+	if (! pop (&val1) || stack_used <= val1
+	    || ! push (stack[stack_used - 1 - val1]))
+	  {
+	    free (stack);
+	    return false;
+	  }
+	break;
+      case DW_OP_over:
+	if (! pop (&val1) || ! pop (&val2)
+	    || ! push (val2) || ! push (val1) || ! push (val2))
+	  {
+	    free (stack);
+	    return false;
+	  }
+	break;
+      case DW_OP_swap:
+	if (! pop (&val1) || ! pop (&val2) || ! push (val1) || ! push (val2))
+	  {
+	    free (stack);
+	    return false;
+	  }
+	break;
+      case DW_OP_rot:
+	{
+	  Dwarf_Addr val3;
+	if (! pop (&val1) || ! pop (&val2) || ! pop (&val3)
+	    || ! push (val1) || ! push (val2) || ! push (val3))
+	  {
+	    free (stack);
+	    return false;
+	  }
+	}
+	break;
       case DW_OP_deref:
 	if (process->callbacks->memory_read == NULL)
 	  {
@@ -241,10 +280,22 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op *ops,
 	    return false;
 	  }
 	break;
-      // case DW_OP_deref_size:
-      // case DW_OP_abs:
-      // case DW_OP_neg:
-      // case DW_OP_not:
+      case DW_OP_deref_size:
+	/* Missing appropriate callback.  */
+	__libdwfl_seterrno (DWFL_E_UNSUPPORTED_DWARF);
+	return false;
+#define UNOP(atom, expr)						\
+      case atom:							\
+	if (! pop (&val1) || ! push (expr))				\
+	  {								\
+	    free (stack);						\
+	    return false;						\
+	  }								\
+	break;
+      UNOP (DW_OP_abs, abs ((int64_t) val1))
+      UNOP (DW_OP_neg, -(int64_t) val1)
+      UNOP (DW_OP_not, ~val1)
+#undef UNOP
       case DW_OP_plus_uconst:
 	if (! pop (&val1) || ! push (val1 + op->number))
 	  {
@@ -260,18 +311,6 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op *ops,
 	    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)				\
@@ -281,12 +320,58 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op *ops,
 	    return false;						\
 	  }								\
 	break;
+      BINOP (DW_OP_and, &)
+      case DW_OP_div:
+	if (! pop (&val2) || ! pop (&val1))
+	  {
+	    free (stack);
+	    return false;
+	  }
+	if (val2 == 0)
+	  {
+	    free (stack);
+	    __libdwfl_seterrno (DWFL_E_INVALID_DWARF);
+	    return false;
+	  }
+	if (! push ((int64_t) val1 / (int64_t) val2))
+	  {
+	    free (stack);
+	    return false;
+	  }
+	break;
+      BINOP (DW_OP_minus, -)
+      case DW_OP_mod:
+	if (! pop (&val2) || ! pop (&val1))
+	  {
+	    free (stack);
+	    return false;
+	  }
+	if (val2 == 0)
+	  {
+	    free (stack);
+	    __libdwfl_seterrno (DWFL_E_INVALID_DWARF);
+	    return false;
+	  }
+	if (! push (val1 % val2))
+	  {
+	    free (stack);
+	    return false;
+	  }
+	break;
+      BINOP (DW_OP_mul, *)
+      BINOP (DW_OP_or, |)
+      BINOP (DW_OP_plus, +)
+      BINOP (DW_OP_shl, <<)
+      BINOP (DW_OP_shr, >>)
+      BINOP_SIGNED (DW_OP_shra, >>)
+      BINOP (DW_OP_xor, ^)
       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
 #undef BINOP_SIGNED
       case DW_OP_bra:
 	if (! pop (&val1))
@@ -321,7 +406,7 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op *ops,
 	Dwarf_Addr cfa;
 	if (frame == NULL
 	    || dwarf_frame_cfa (frame, &cfa_ops, &cfa_nops) != 0
-	    || ! expr_eval (state, NULL, cfa_ops, cfa_nops, &cfa)
+	    || ! expr_eval (state, NULL, cfa_ops, cfa_nops, &cfa, bias)
 	    || ! push (cfa))
 	  {
 	    __libdwfl_seterrno (DWFL_E_LIBDW);
@@ -335,7 +420,7 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op *ops,
 	is_location = false;
 	break;
       default:
-	__libdwfl_seterrno (DWFL_E_UNSUPPORTED_DWARF);
+	__libdwfl_seterrno (DWFL_E_INVALID_DWARF);
 	return false;
     }
   if (! pop (result))
@@ -364,7 +449,7 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op *ops,
    later.  Therefore we continue unwinding leaving the registers undefined.  */
 
 static void
-handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, Dwarf_CFI *cfi)
+handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, Dwarf_CFI *cfi, Dwarf_Addr bias)
 {
   Dwarf_Frame *frame;
   if (INTUSE(dwarf_cfi_addrframe) (cfi, pc, &frame) != 0)
@@ -419,7 +504,7 @@ handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, Dwarf_CFI *cfi)
 	      continue;
 	    }
 	}
-      else if (! expr_eval (state, frame, reg_ops, reg_nops, &regval))
+      else if (! expr_eval (state, frame, reg_ops, reg_nops, &regval, bias))
 	{
 	  /* PPC32 vDSO has various invalid operations, ignore them.  The
 	     register will look as unset causing an error later, if used.
@@ -470,14 +555,14 @@ __libdwfl_frame_unwind (Dwfl_Frame *state)
       Dwarf_CFI *cfi_eh = INTUSE(dwfl_module_eh_cfi) (mod, &bias);
       if (cfi_eh)
 	{
-	  handle_cfi (state, pc - bias, 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);
       if (cfi_dwarf)
 	{
-	  handle_cfi (state, pc - bias, cfi_dwarf);
+	  handle_cfi (state, pc - bias, cfi_dwarf, bias);
 	  if (state->unwound)
 	    return;
 	}