RFA: Location list support for DWARF-2

Jim Blandy jimb@redhat.com
Wed Mar 12 21:40:00 GMT 2003


Daniel Jacobowitz <drow@mvista.com> writes:
> Here's initial support for location lists.  I'd like to take a moment
> to thank Jim Blandy and Daniel Berlin for hashing out the interface to
> LOC_COMPUTED so thoroughly; it turned out that I didn't need to do
> anything at all to the interface or to its clients in order to make
> this work.
> 
> To test this, you'll need a GCC CVS checkout from the "rtlopt-branch"
> branch tag.  This patch has no effect if location lists aren't used,
> and is a strict improvement if they are, since otherwise we won't find
> things at all.  However, debug output from that branch is still a
> little immature.  One problem I've noticed so far is that we emit
> incomplete location lists for global variables; see my post on the gcc@
> list for more details.  So sometimes we can't find globals, which is a
> regression in the debug info.
> 
> This patch also does not support multiple overlapping location lists;
> it simply uses the first match.  The rest of GDB isn't ready for
> multiple locations yet; that's down the list after DW_OP_piece I
> think.
> 
> I had to add the CU base address to each symbol's baton.  Long term we
> can get rid of it by making sure we can go from symbol to symtab and
> storing it in the symtab; I think that's a good idea but it's a patch
> for another day.
> 
> Jim, is this OK?  I'd appreciate another set of eyes on it.

Rather than putting 'if (foo->is_list)' everywhere, why not just
introduce a separate 'struct location_funcs' for location lists?
Aside from just being cleaner, you'll save a word in each baton.

> Index: dwarf2loc.c
> ===================================================================
> RCS file: /big/fsf/rsync/src-cvs/src/gdb/dwarf2loc.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 dwarf2loc.c
> --- dwarf2loc.c	5 Mar 2003 18:00:02 -0000	1.3
> +++ dwarf2loc.c	10 Mar 2003 15:22:17 -0000
> @@ -40,6 +40,68 @@
>  #define DWARF2_REG_TO_REGNUM(REG) (REG)
>  #endif
>  
> +
> +/* A helper function for dealing with location lists.  Given a
> +   symbol baton (BATON) and a pc value (PC), find the appropriate
> +   location expression, set *LOCEXPR_LENGTH, and return a pointer
> +   to the beginning of the expression.  Returns NULL on failure.
> +
> +   For now, only return the first matching location expression; there
> +   can be more than one in the list.  */
> +
> +static char *
> +find_location_expression (struct dwarf2_locexpr_baton *baton,
> +			  int *locexpr_length, CORE_ADDR pc)
> +{
> +  CORE_ADDR base_address = baton->base_address;
> +  CORE_ADDR low, high;
> +  char *loc_ptr;
> +  unsigned int addr_size = TARGET_ADDR_BIT / TARGET_CHAR_BIT, length;
> +  CORE_ADDR base_mask = ~(~(CORE_ADDR)1 << (addr_size * 8 - 1));

Why not just ~(~(CORE_ADDR)0 << (addr_size * 8))?

> +
> +  if (! baton->is_list)
> +    {
> +      *locexpr_length = baton->size;
> +      return baton->data;
> +    }
> +
> +  loc_ptr = baton->data;
> +
> +  while (1)
> +    {
> +      low = dwarf2_read_address (loc_ptr, loc_ptr + addr_size, &length);
> +      loc_ptr += length;
> +      high = dwarf2_read_address (loc_ptr, loc_ptr + addr_size, &length);
> +      loc_ptr += length;

Shouldn't you pass baton->data + baton_size as the 'buf_end' argument?
You're defeating the range-checking that function performs.

> Index: dwarf2read.c
> ===================================================================
> RCS file: /big/fsf/rsync/src-cvs/src/gdb/dwarf2read.c,v
> retrieving revision 1.88
> diff -u -p -r1.88 dwarf2read.c
> --- dwarf2read.c	25 Feb 2003 21:36:17 -0000	1.88
> +++ dwarf2read.c	10 Mar 2003 05:01:11 -0000
> @@ -220,9 +220,13 @@ struct comp_unit_head
>  
>      struct abbrev_info *dwarf2_abbrevs[ABBREV_HASH_SIZE];
>  
> -    /* Pointer to the DIE associated with the compilation unit.  */
> +    /* Base address of this compilation unit.  */
>  
> -    struct die_info *die;
> +    CORE_ADDR base_address;
> +
> +    /* Flag representing whether base_address is accurate.  */

You can be more concrete: "Non-zero if base_address has been set."

> @@ -1642,8 +1670,32 @@ psymtab_to_symtab_1 (struct partial_symt
>  
>    make_cleanup_free_die_list (dies);
>  
> +  /* Find the base address of the compilation unit for range lists and
> +     location lists.  It will normally be specified by DW_AT_low_pc.
> +     In DWARF-3 draft 4, the base address could be overridden by
> +     DW_AT_entry_pc.  It's been removed, but GCC still uses this for
> +     compilation units with discontinuous ranges.  */
> +
> +  cu_header.base_known = 0;
> +  cu_header.base_address = 0;
> +
> +  attr = dwarf_attr (dies, DW_AT_entry_pc);
> +  if (attr)
> +    {
> +      cu_header.base_address = DW_ADDR (attr);
> +      cu_header.base_known = 0;

Shouldn't this set it to 1, not zero?

> @@ -6481,6 +6511,7 @@ dump_die (struct die_info *die)
>  	case DW_FORM_ref1:
>  	case DW_FORM_ref2:
>  	case DW_FORM_ref4:
> +	case DW_FORM_ref8:
>  	case DW_FORM_udata:
>  	case DW_FORM_sdata:
>  	  fprintf_unfiltered (gdb_stderr, "constant: %ld", DW_UNSND (&die->attrs[i]));

This should be a separate patch (which you are welcome to commit).

> @@ -7330,23 +7361,39 @@ dwarf2_symbol_mark_computed (struct attr
>  {
>    struct dwarf2_locexpr_baton *baton;
>  
> -  /* When support for location lists is added, this will go away.  */
> -  if (!attr_form_is_block (attr))
> -    {
> -      dwarf2_complex_location_expr_complaint ();
> -      return;
> -    }
> -
>    baton = obstack_alloc (&objfile->symbol_obstack,
>  			 sizeof (struct dwarf2_locexpr_baton));
>    baton->objfile = objfile;
>  
> -  /* Note that we're just copying the block's data pointer here, not
> -     the actual data.  We're still pointing into the dwarf_info_buffer
> -     for SYM's objfile; right now we never release that buffer, but
> -     when we do clean up properly this may need to change.  */
> -  baton->size = DW_BLOCK (attr)->size;
> -  baton->data = DW_BLOCK (attr)->data;
> +  /* When support for location lists is added, this will go away.  */

This comment should go away, shouldn't it?

> +  if (attr_form_is_block (attr))
> +    {
> +      /* Note that we're just copying the block's data pointer here, not
> +	 the actual data.  We're still pointing into the dwarf_info_buffer
> +	 for SYM's objfile; right now we never release that buffer, but
> +	 when we do clean up properly this may need to change.  */
> +      baton->size = DW_BLOCK (attr)->size;
> +      baton->data = DW_BLOCK (attr)->data;
> +      baton->is_list = 0;
> +    }
> +  else if (attr->form == DW_FORM_data4 || attr->form == DW_FORM_data8)
> +    {
> +      baton->size = 0;

We don't have good information about the range list's extent
available, but you should set this to 'dwarf_loc_size - DW_UNSND
(attr)' or something like that.

> +      baton->data = dwarf_loc_buffer + DW_UNSND (attr);
> +      baton->is_list = 1;
> +      baton->base_address = cu_header->base_address;
> +      if (cu_header->base_known == 0)
> +	complain (&symfile_complaints,
> +		  "Location list used without specifying the CU base address.");
'complain' has been gone for a month and a half now.  You'd probably
better check this patch against more recent sources.



More information about the Gdb-patches mailing list