This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: RFA: LOC_COMPUTED support
- From: Jim Blandy <jimb at redhat dot com>
- To: Daniel Jacobowitz <drow at mvista dot com>
- Cc: gdb-patches at sources dot redhat dot com
- Date: 05 Feb 2003 00:24:34 -0500
- Subject: Re: RFA: LOC_COMPUTED support
- References: <20030203174319.GA26299@nevyn.them.org>
Daniel Jacobowitz <drow@mvista.com> writes:
> Here we go again. I've been staring at this patch for a couple of days, so
> my description of it may be slightly incoherent. Let me know if I miss
> anything major.
>
> First, overview, pretty much unchanged from when Daniel Berlin posted it:
> we add two new address classes, LOC_COMPUTED and LOC_COMPUTED_ARG, whose
> address must be determined by calling some set of runtime functions provided
> by the symbol reader.
>
> There are a lot of little structural changes from Daniel's patch; for
> instance, I moved some code from dwarf2read.c out into a new file, because
> the idea of including things like "target.h" and "inferior.h" in a debug
> reader bothered me a little. I also shrunk the size of the batons a bit.
>
> Two things that Jim commented on as problems with the original patch are
> still here, because on reflection I thought it was better this way: the use
> of get_frame_function and the function's aux_value union to hold the frame
> base
What's weird about this is that the LOC_BLOCK symbol's aux_value is
never used by the core GDB code at all --- it's used strictly by
Dwarf-specific code. When you say, "and [used] by LOC_BLOCK to locate
the function's base register", you're referring to something that
doesn't exist in core GDB.
The comment needs to come clean about this:
/* For a LOC_COMPUTED symbol, this is the baton and location_funcs
structure to find its value. For a LOC_BLOCK symbol for a
function in a compilation unit compiled with Dwarf 2 information,
this is information used internally by the Dwarf 2 code ---
specifically, the Dwarf location description that computes frame
base for that function. */
Maybe I'm going too far, but that is at least more honest about how
messy this is. I'm still not sure I'm cool with this.
> and the use of pointers directly into the .debug_info (eventually
> .debug_loc) data. The former is for memory reasons, among others; Jim's
> suggestion of each symbol having a pointer to the frame base would increase
> the per-symbol memory for all LOC_COMPUTED* symbols, since we can't easily
> tell which symbols will need the frame base and which won't. The latter is
> because we never get rid of it anyway, so why duplicate the data? When
> someone tackles cleanups in the reader to let us free the sections, then we
> can talk about alternate memory management.
Right, that was what I was worried about. The relocation patch makes
this harder, too, since BFD doesn't have any interface to relocate
only a portion of a section. But I think you're right --- at this
point it's best to get out of the way for other improvements.
The documentation for dwarf2_locexpr_baton should explain that it's
pointing into dwarf_info_buffer, and should explain exactly which
objfile's dwarf_info_buffer that is.
I may be missing something, but I think your TLS stuff is broken.
Suppose the symbol S's location is computed by a Dwarf location
expression that uses DW_OP_GNU_push_tls_address. SYMBOL_CLASS (S) is
LOC_COMPUTED. Therefore S->aux_value holds a baton and a struct
location_funcs. Its struct location_funcs read_variable is
locexpr_read_variable, which calls dwarf2_evaluate_loc_desc on S, a
frame, and the locexpr. dwarf2_evaluate_loc_desc calls
dwarf_expr_eval with a struct dwarf_expr_baton whose 'var' is S. The
case for DW_OP_GNU_push_tls_address calls the context's
get_tls_address, which is dwarf_expr_tls_address, passing the baton
and the offset. That then tries to access SYMBOL_OBJFILE (S) --- but
that is S->aux_value.objfile, when S->aux_value is actually the
LOC_COMPUTED baton and location_funcs.
But I'm not sure I have anything really helpful or substantial to say.
Some other nits:
The results of DW_OP_shr, DW_OP_shra, and the comparison operators can
depend on CORE_ADDR bits that may be above the current architecture's
address size. All of those operators need to use the
value_from_pointer trick that DW_OP_div uses now, I'm afraid.
Could you move 'struct dwarf2_locexpr_baton' and
'dwarf2_locexpr_funcs' into a new header, dwarf2loc.h?
dwarf2_loc_desc_needs_frame should either have an extern declaration
somewhere, or be static to dwarf2loc.c. The various needs_frame_*
callbacks should all be static.
+/* Expand the memory allocated to CTX's stack to contain at least
+ NEED elements. */
+
This comment isn't accurate: it makes sure that CTX's stack has room
to hold at least NEED more elements than it holds now.
Could read_uleb128, read_sleb128, and read_address take a pointer to
the end of the buffer, and detect misformed constants?
+static struct value *
+dwarf2_evaluate_loc_desc (struct symbol *var, struct frame_info *frame,
+ unsigned char *data, unsigned short size)
+{
+ CORE_ADDR result;
+ struct value *retval;
+ struct dwarf_expr_baton *baton = xmalloc (sizeof (struct dwarf_expr_baton));
+ struct dwarf_expr_context *ctx;
+
You could just allocate the dwarf_expr_baton on the stack directly.
> Particularly notable bits are the comments in var_decode_location. I've
> verified that I got both the LOC_STATIC and relocated symbols bits correct;
> it would be nice to kill the LOC_STATIC hack but that requires cleaning up
> the ways we handle section offsets, and that's way too murky for me right
> now.
I don't really understand what LOC_UNRESOLVED was supposed to do, but
I'm going to take your word that you've handled it here; having
LOC_COMPUTED is worth the risk that you're wrong about LOC_UNRESOLVED
and we need to go back and fix something. Elena may know more about
this than I do.
Could we also detect the DW_OP_reg* and produce LOC_REGISTER
variables? As it stands, this patch is going to make "info address
VAR" pretty useless when Dwarf 2 is in use. It'll save memory, too,
won't it? :)