Re: RFA: Location list support for DWARF-2

Jim Blandy
2003-03-12T21:40:44Z

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.