On Mon, 28 Oct 2013 10:28:07 +0100, Mark Wielaard wrote:
On Sun, 2013-10-27 at 14:53 +0100, Jan Kratochvil wrote:
> 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.
While it tests a lot of ops it for example does not test any DW_OP_*deref*.
> 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).
OK, if elfutils testsuite is GPLv3+ it really should not be a problem.
> + 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.
It is not even just the pedanticality, there was I think a bug for big endian
32-bit hosts.
> 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.
What if you run on 32-bit host, foreign 32-bit process run-time patches its
ELF header to be ELFCLASS64 and you run eu-stack on it? I think you would
assert. This is dependency on external data so I believe there should not be
an assert.
Thanks,
Jan
diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c
index 94213e5..38137ba 100644
--- a/libdwfl/frame_unwind.c
+++ b/libdwfl/frame_unwind.c
@@ -304,7 +304,9 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op
*ops,
}
if (op->atom == DW_OP_deref_size)
{
- if (op->number > 8)
+ const int elfclass = frame->cache->e_ident[EI_CLASS];
+ const unsigned addr_bytes = elfclass == ELFCLASS32 ? 4 : 8;
+ if (op->number > addr_bytes)
{
free (stack);
__libdwfl_seterrno (DWFL_E_INVALID_DWARF);
@@ -314,7 +316,7 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const Dwarf_Op
*ops,
if (op->number == 0)
val1 = 0;
else
- val1 >>= 64 - op->number * 8;
+ val1 >>= (addr_bytes - op->number) * 8;
#else
if (op->number < 8)
val1 &= (1 << (op->number * 8)) - 1;