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

[Mark Wielaard <mjw at redhat dot com>]

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