Re: [Libunwind-devel] [PATCH] Implement DWARF DW_CFA_val_expression for x86_64

[Tim Deegan]

Hi,

At 00:35 -0800 on 31 Jan (1391124901), Arun Sharma wrote:
> On Mon, Jan 20, 2014 at 3:41 AM, Tim Deegan <address@hidden> wrote:
> >  static inline int
> > @@ -172,9 +183,12 @@ dwarf_put (struct dwarf_cursor *c, dwarf_loc_t loc, 
> > unw_word_t val)
> >    if (DWARF_IS_REG_LOC (loc))
> >      return (*c->as->acc.access_reg) (c->as, DWARF_GET_LOC (loc), &val,
> >                                      1, c->as_arg);
> > -  else
> > +  if (DWARF_IS_MEM_LOC (loc))
> >      return (*c->as->acc.access_mem) (c->as, DWARF_GET_LOC (loc), &val,
> >                                      1, c->as_arg);
> > +  assert(DWARF_IS_VAL_LOC (loc));
> > +  loc = DWARF_VAL_LOC(c, val);
> 
> Is this code necessary?

You mean, because nothing ever calls dwarf_put() with a VAL_LOC?  I
suppose not; would you be happy with an assert(!DWARF_IS_VAL_LOC (loc)) 
at the head of the function instead?

> > +       case DWARF_WHERE_EXPR_VAL:
> 
> DWARF_WHERE_VAL_EXPR reads better.

Sure; I'll rename that for a v2.

> > +         addr = rs->reg[i].val;
> > +         if ((ret = eval_location_expr (c, as, a, addr, c->loc + i, arg, 
> > 1)) < 0)
> >             return ret;
> 
> This is fine. Another way to do it would be to update the
> DWARF_LOC_TYPE after the eval() instead of passing in an extra
> parameter (is_val).

I actually did it that way in my first fix but reworked it for
upstreaming because I thought this was cleaner. :)  Happy to switch
back if you prefer.

Cheers,

Tim,

-- 
                                         Tim Deegan <address@hidden>
         I understand that authors become either WEALTHY and CELEBRATED,
             or STARVE in BOXES, and I'm much disinclined to the latter.
                                [ Sydney Padua, "2D Goggles", 08/02/11 ]