This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFA: LOC_COMPUTED support


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?  :)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]