From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12397 invoked by alias); 8 Jul 2002 21:01:44 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 12358 invoked from network); 8 Jul 2002 21:01:41 -0000 Received: from unknown (HELO zwingli.cygnus.com) (208.245.165.35) by sources.redhat.com with SMTP; 8 Jul 2002 21:01:41 -0000 Received: by zwingli.cygnus.com (Postfix, from userid 442) id 7B8585EA11; Mon, 8 Jul 2002 16:01:36 -0500 (EST) To: Daniel Berlin Cc: gdb-patches@sources.redhat.com Subject: Re: [RFA]: LOC_COMPUTED + abstracted dwarf2 evaluator, again References: From: Jim Blandy Date: Mon, 08 Jul 2002 14:30:00 -0000 In-Reply-To: Message-ID: User-Agent: Gnus/5.09 (Gnus v5.9.0) Emacs/21.1 MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2002-07/txt/msg00125.txt.bz2 Daniel Berlin 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 ("", 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 * 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!