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

Jim Blandy
2002-07-08T21:01:44Z

Daniel Berlin <dberlin@dberlin.org> writes:
> For yer convenience, I rediffed it against the current gdb sources.

Thanks!

> I'll update the changelog dates if it ever gets reviewed or
> approved.

I do appreciate your re-posting the patch. I have been trying to be
more prompt with reviews in the last few weeks, and not fall behind.

ChangeLog entries like this don't work:

> (Simple cases of *.c containing LOC_): Handle LOC_COMPUTED and
> LOC_COMPUTED_ARG.

Nobody reads a ChangeLog from beginning to end; they search it. So
even when the list of files and functions is a dozen times longer than
the actual ChangeLog entry itself, it's best to actually list all the
files explicitly, with no `*-tdep.c', `mips-{tdep,nat}.c', at all.
It takes time and it's boring, but I think it's a good rule.

> Slightly updated accompanying text (from the april submission):
> "
> The only real things left to do is give some *real* description in
> locexpr_describe_location

Like something that prints out the expression? Yeah, that would be
nice, but it can get involved if you've got branches. Maybe you can
steal code from readelf; that prints location expressions.

> add tracepoint support

In general, you're looking at a Dwarf -> Agent Expression compiler.
Yahoo. I don't know if anyone's even using the tracepoint stuff; it
seems okay to me to leave this unsupported for now. (Although it
seems to me that tracepoints would be The Thing for debugging the
kernel. Just write a kernel module that logs stuff to a buffer
visible via /proc, write a custom gdbserver, and away you go,
debugging page fault handlers.)

> and remove the dwarf2cfi.c evaluator . None of these should be
> blockers to committing it or reviewing it.

No, I agree.

> It would also be nice to use location expressions in *all* cases
> (currently, it only does it for variables) so you can get rid of
> decode_loc_desc, but i'll leave that for the next patch.

Makes sense.

> There are a few lines that are too long (<10, i *think*), sorry about
> that, i couldn't see an easy way to break them up and have them make
> sense.

Well, this does need to be corrected before you commit. Come on, the
rest of us all do it, I think you can handle the agony. :)

> Splitting this patch into loc_computed addition, and dwarf2 implementation
> of loc_computed, doesn't make it much smaller (loc_computed_addition is
> maybe 5% of the patch), so if size is a concern, sorry, but it's pretty
> much unsplittable beyond that (the biggest portion is the addition of the
> abstracted evaluator, but the dwarf2 implementation doesn't work without
> it).

It's not so much that a patch is too large or too small, it's the
additional brain load from having to figure out which change each hunk
belongs to. I^HSome of us have limited resources to work with.

The patch misses some cases:

- I think ch-exp.c:ch_lex and m2-exp.y:yylex need some additional
`case's for LOC_COMPUTED{,_ARG}. I grant you, Chill is dead, but
they're easy tweaks.
- print_frame_args needs a case for LOC_COMPUTED_ARG.
- print_block_frame_locals needs a case for LOC_COMPUTED.
- print_frame_arg_vars needs a case for LOC_COMPUTED_ARG.

> Index: symmisc.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/symmisc.c,v
> retrieving revision 1.9
> diff -c -3 -p -w -B -b -r1.9 symmisc.c
> *** symmisc.c 15 May 2002 21:19:21 -0000 1.9
> --- symmisc.c 8 Jul 2002 16:52:40 -0000
> *************** print_partial_symbols (struct partial_sy
> *** 862,867 ****
> --- 862,871 ----
> case LOC_OPTIMIZED_OUT:
> fputs_filtered ("optimized out", outfile);
> break;
> + case LOC_COMPUTED:
> + case LOC_COMPUTED_ARG:
> + fputs_filtered ("computed at runtime", outfile);
> + break;
> default:
> fputs_filtered ("<invalid location>", outfile);
> break;

I guess we can't do anything more detailed here because we only have
partial symbol table information --- the address class, but not the
function group or the baton. That's a pity, but I don't see how to do
better.

> + /* 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:

> + case LOC_COMPUTED:
> + case LOC_COMPUTED_ARG:
> + {
> + struct location_funcs *funcs = SYMBOL_LOCATION_FUNCS (var);
> + void *baton = SYMBOL_LOCATION_BATON (var);
> +
> + if (frame == 0 && (funcs->read_needs_frame)(baton))
> + return 0;
> + return (funcs->read_variable) (baton, frame);
> +
> + }
> + break;

So read_var_value will return zero whenever it's given a
LOC_COMPUTED{,_ARG} symbol and zero for a frame. But read_var_value
gets zero for the frame whenever you try to print a global or static
variable when the program isn't running, even though the lack of a
frame is harmless. (value_struct_elt_for_reference seems to pass a
frame of zero, too, but I'm not sure in what contexts that occurs.)

> ! LOC_INDIRECT,
> !
> ! /* The variable address is computed by a set of location
> ! functions. */
> ! LOC_COMPUTED,
> !
> ! /* The variable is an argument, and it's address is computed by a
> ! set of location functions. */
> ! LOC_COMPUTED_ARG
> !
> ! };

These comments should explicitly say how one computes the address ---
or at least refer to the comment above `struct location_funcs', just
below.

> *************** struct symbol
> *** 660,665 ****
> --- 712,719 ----
> {
> /* Used by LOC_BASEREG and LOC_BASEREG_ARG. */
> short basereg;
> + /* Location baton to pass to location functions. */
> + void *locbaton;
> }
> aux_value;
>
> *************** struct symbol
> *** 671,676 ****
> --- 725,734 ----
> /* List of ranges where this symbol is active. This is only
> used by alias symbols at the current time. */
> struct range_list *ranges;
> +
> + /* Location functions for LOC_COMPUTED and LOC_COMPUTED_ARGS. */
> + struct location_funcs *locfuncs;
> +
> };

I think both locbaton and locfuncs should be in the union (as a
struct, of course).

> *************** 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. 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'. 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.

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. That means the Dwarf reader has to build this new fbreg
structure when it enters the function, and just leave a pointer to
it lying around for all the new batons to use, which is a bit icky.
But that's basically what we do now, with the `frame_base_reg' and
`frame_base_offset' global variables.

- Small issue: DW_BLOCK points into the .dwarf_info buffer, right?
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.

> + struct dwarf_expr_baton *debaton = (struct dwarf_expr_baton *)baton;

(a `debaton' being the fundamental particle of argument... :) )

> + /* 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?

Your post doesn't include dwarf2expr.[ch].

If your evaluator is based on the one in dwarf2cfi.c, did you
incorporate Mark's recent fix?

2002-07-08 Mark Kettenis <kettenis@gnu.org>

* dwarf2cfi.c: Include "gcore.h".
(execute_stack_op): Fix implementation of the
DW_OP_deref and DW_OP_deref_size operators by letting do their
lookup in the target.

But overall --- this is great. I'm looking forward to the next
revision. Thanks!