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