On Sun, 2013-10-27 at 14:53 +0100, Jan Kratochvil wrote: > On Sat, 26 Oct 2013 23:47:04 +0200, Mark Wielaard wrote: > > On Fri, 2013-10-25 at 18:22 +0200, Jan Kratochvil wrote: > > > 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. */ > > > > Do we need to extend the memory_read callback to take a size argument? > > Or can we do something clever (depending on byte order) with the current > > memory_read of a full word and chop it down? The size will never be > > bigger than the number of bytes in an address. So it will never be > > bigger than 8. > > OK, true, implemented DW_OP_deref_size.
Nice. Even simpler than I thought. > > > BTW elfutils should really get DW_OP_GNU_encoded_addr implemented I guess > > > as it > > > may appear in CFI. > > > > It is indeed documented (at the end of) > > https://sourceware.org/binutils/docs-2.22/as/CFI-directives.html > > .cfi_val_encoded_addr will generate a DW_OP_GNU_encoded_addr according > > to http://gcc.gnu.org/ml/gcc-patches/2008-09/msg01713.html > > But I haven't actually seen it used. Would be nice to have an example > > usage. > > It is true that gcc/dwarf2cfi.c does not use it, if I read it correctly. That might explain why I never encountered it. Would still be good to get support eventually since some hand-coded assembly might be using it. But it seems somewhat obscure. > So I have finally run the elfutils unwinder on Jakub's > gcc/testsuite/gcc.dg/cleanup-13.c and it really found DW_OP_rot is buggy. > Besides that it found also DW_OP_pick had a bug (fixed it below). > And then it works (patched it with the simple attached patch). Nice! I forgot about that testcase. It is a nasty one. Very good the code works well against it. > Just I do not know if cleanup-13.c is covered by GPL3 or if it is public > domain but I guess the former. In such case I do not think it is compatible > with elfutils due to its LGPL option. The LGPLv3+ option is only for the libraries. Tools and tests are GPLv3+ mostly. So it should not be a problem to include it, if it in the testcase under GPLv3+ (or public domain, which obviously is compatible). If in doubt, lets just ask Jakub if he is OK with that. > plain text document attachment (2) > diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c > index eca7a04..a875e98 100644 > --- a/libdwfl/frame_unwind.c > +++ b/libdwfl/frame_unwind.c > @@ -231,8 +231,13 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const > Dwarf_Op *ops, > } > break; > case DW_OP_pick: > - if (! pop (&val1) || stack_used <= val1 > - || ! push (stack[stack_used - 1 - val1])) > + if (stack_used <= op->number) > + { > + free (stack); > + __libdwfl_seterrno (DWFL_E_INVALID_DWARF); > + return false; > + } > + if (! push (stack[stack_used - 1 - op->number])) > { > free (stack); > return false; Aha, I missed that DW_OP_pick didn't pop from the stack. And push () will set DWFL_E_NOMEM on failure already. Good fix. > + if (op->atom == DW_OP_deref_size) > + { > + if (op->number > 8) > + { > + free (stack); > + __libdwfl_seterrno (DWFL_E_INVALID_DWARF); > + return false; > + } This is fine, but to be pedantically correct (in an error case), you could check against the actual address_size, which can be gotten from the cfi->e_ident[EI_CLASS] == ELFCLASS32 ? 4 : 8. > diff --git a/libdwfl/linux-pid-attach.c b/libdwfl/linux-pid-attach.c > index 0721c88..66a0814 100644 > --- a/libdwfl/linux-pid-attach.c > +++ b/libdwfl/linux-pid-attach.c > @@ -122,9 +122,14 @@ pid_memory_read (Dwfl *dwfl, Dwarf_Addr addr, Dwarf_Word > *result, void *arg) > Dwfl_Process *process = dwfl->process; > if (ebl_get_elfclass (process->ebl) == ELFCLASS64) > { > +#if SIZEOF_LONG == 8 > errno = 0; > *result = ptrace (PTRACE_PEEKDATA, tid, (void *) (uintptr_t) addr, > NULL); > return errno == 0; > +#else /* SIZEOF_LONG != 8 */ > + /* This should not happen. */ > + return false; > +#endif /* SIZEOF_LONG != 8 */ I think an assert is in order here. Looks good, all the above are just tiny unimportant nitpicks. Thanks, Mark
