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

[Jan Kratochvil]

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;