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, ®val))
+ else if (! expr_eval (state, frame, reg_ops, reg_nops, ®val, 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;
}