Re: [RFA]: LOC_COMPUTED + abstracted dwarf2 evaluator, again

[Jim Blandy <jimb at redhat dot com>]

Daniel Berlin <dberlin@dberlin.org> writes:
> > > + /* This is a struct location function, this one returns 1 if we need a
> > > +    frame to read the value of the variable. */
> > > + static int 
> > > + locexpr_read_needs_frame (void *baton)
> > > + {
> > > +   return 1;
> > > + }
> > 
> > This needs to be smarter, in light of this code in find_var_value:
> We can't know if it's got a constant address without looking through the 
> location codes.
> But since the expression evaluator is opaque, *it* is what looks at the 
> actual location expressions, we (the reader) treat them as magic blocks of 
> data.
> :)
> The simple workaround is to have a function in the same file as the 
> evaluator that tells us if a location contains a register reference, since 
> it has access to the data in question.

Yes, that sounds like the right way to do it.  Keep all the knowledge
about DW_OP_* and their operands in the same place.  (Perhaps we could
put the dwarf expression printing code there, too, and they could be
driven by the same table.  But that should be a separate patch, please.)

> > > > *************** read_func_scope (struct die_info *die, s > 
> > *** 1886,1891 ****
> > > --- 1912,1931 ----
> > >   
> > >     new = push_context (0, lowpc);
> > >     new->name = new_symbol (die, die->type, objfile, cu_header);
> > > + 
> > > +   /* If it has a location expression for fbreg, record it.  
> > > +      Since not all symbols use location expressions exclusively yet,
> > > +      we still need to decode it above. */
> > > +   if (attr)
> > > +     {
> > > +       baton = obstack_alloc (&objfile->symbol_obstack, sizeof (struct locexpr_baton));
> > > +       baton->sym = new->name;
> > > +       baton->obj = objfile;
> > > +       memcpy (&baton->locexpr, DW_BLOCK (attr), sizeof (struct dwarf_block));
> > > +       SYMBOL_LOCATION_FUNCS (new->name) = &locexpr_funcs;
> > > +       SYMBOL_LOCATION_BATON (new->name) = baton;
> > > +     }
> > > + 
> > >     list_in_scope = &local_symbols;
> > >   
> > >     if (die->has_children)
> > 
> > Whoa, this is problematic.
> > 
> > - Big issue: you're assigning a meaning to the aux_value field of a
> >   function's symbol, where previously it had none.
> 
> So?
> 
> The aux value field is there because "Some symbols require an additional 
> value to be recorded on a per-symbol basis. Stash those values here".
> 
> Function symbols require an additional value to be recorded on a per 
> symbol basis.
> So I stashed it there.
> 
> Seemed to make sense at the time.

Well, right, but if it's data specific to a particular debug format,
well, that needs to go someplace debug-reader specific.


> 
> If you've got a better idea, i'm happy to implement it.
> >   This at the very
> >   least needs to be documented --- I almost didn't notice the issue,
> >   since it isn't mentioned in the comments for `struct symbol'.
> Oversight on my part.
> 
> >   And
> >   while we certainly need some way to find the fbreg expression, I'm
> >   sure that taking over generic fields of function symbols for a
> >   rather Dwarf-specific optimization is not the right way to do this.
> 
> It's actually neither a generic field, nor dwarf specific, 
> nor an optimization.
>
> The field is per-symbol specific, and has no specific purpose given, other 
> than to be a place to stash values.

You're right.  The fbreg info is strictly necessary for evaluating
Dwarf expressions --- it's not an optimization.

The "generic fields" bit is sloppy, too.  Here's what I should have
written:

It's important that one should be able to look at sym->aclass and know
exactly what aux_value means.  They're not really separate values ---
together aclass and aux_value form a discriminated union.  (This is
the same reason I wanted the location function pointer in a struct in
aux_value.)

As it stands, the patch effectively says, "when aclass == LOC_BLOCK,
aux_value holds debug-format-specific info."  I think if you
documented it that way, I'd mind less, but it still feels really
wrong.

> >   I think it would be better if all the batons for a function's
> >   locals, etc. had a pointer to a shared thingy for the fbreg
> >   expression. 
> 
> If you like, but IMHO, it just makes the code more complex, and less 
> understandable to debug. It makes less sense from a perspective of someone 
> trying to 
> understand what's going on, because the pointer would only be different in 
> different functions anyway.  And if they looked at the debug info, they 
> would see the expression associated only with the function, and only 
> appear once.

I agree with everything you say here.  The frame_base_reg and
frame_base_offset global vars are a crock.  But they are a crock local
to dwarf2read.c that *could* be replaced by something cleaner, as part
of a general cleanup of that area in Dwarf.  But the end result ---
having the batons point to a shared structure giving the data for
DW_OP_fbreg --- seems quite clean to me.

You won't need to do that function lookup by PC each time you print a
variable, either...

> >   Are there any other cases where the symbol table points into
> >   .dwarf_info?  The names get copied for partial symbols and full
> >   symbols.  I think we need to copy the whole location expression into
> >   the objfile's obstack.  Similarly for the DW_TAG_variable case.
> 
> Okeydokey.
> 
> Some year, when bfd supports mmap, ...

I used to be skeptical about your enthusiasm for mmap, but I'm being
persuaded.  I think I remember Ulrich Drepper talking about how he
found that mmap was much faster in libelf and libdwarf.

> > > +   struct dwarf_expr_baton  *debaton = (struct dwarf_expr_baton *)baton;
> > 
> > (a `debaton' being the fundamental particle of argument...  :) )
> 
> dwarf-expr-baton
> :)

Oh, that was clear.  It just hit me funny.  :)

> > > + /* Read a register for the dwarf2 expression evaluator.  */
> > > + CORE_ADDR dwarf_expr_read_reg (void *baton, int regnum)
> > > + {
> > > +   CORE_ADDR result;
> > > +   struct dwarf_expr_baton  *debaton = (struct dwarf_expr_baton *)baton;
> > > +   char * buf = (char *) alloca (MAX_REGISTER_RAW_SIZE);
> > > +   get_saved_register (buf, NULL, NULL, debaton->frame, regnum, NULL);
> > > +   result = extract_address  (buf, REGISTER_RAW_SIZE (regnum));
> > > +   return result;
> > > + }
> > 
> > Would `read_register' work here?
> 
> No, because we read_register gives the register for the *current* 
> frame, and the frame *we want* is associated with a given evaluator baton. 
> (debaton->frame).

Oh --- right.  It's a shame there isn't something like read_register
that takes a frame as an argument.  Or at least, I can't find one.

> So the answer is "for reading variables, debaton->frame is, AFAIK,
> always going to be the same as the frame read_register would use,
> but from an abstraction perspective, we shouldn't use this fact
> since it's using magic knowledge of how we happened to initalize the
> evaluator context below, which is bad."  IIRC, the whole reason for
> this at all is that it's necessary to be able to reuse the evaluator
> in dwarf2cfi.

No, I think it's absolutely correct to insist on using debaton->frame.
The fact that you have to allocate a little buffer and then parse it
with extract_address isn't your fault.

> > If your evaluator is based on the one in dwarf2cfi.c, did you
> > incorporate Mark's recent fix?
> It's not based on dwarf2cfi.
> 
> The routine that reads memory (remember, it's abstracted, as you 
> requested) for dwarf2read's usage of it (location expressions) is in 
> dwarf_expr_read_mem, which is simply:
> 
> 
> /* Read memory for the dwarf2 expression evaluator.  */
> CORE_ADDR dwarf_expr_read_mem (void *baton, CORE_ADDR addr, size_t len)
> {
>   return (CORE_ADDR) read_memory_unsigned_integer (addr, len);
> }

Right --- you've already had to replace exactly the part that Mark
fixed.