From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24601 invoked by alias); 12 Mar 2003 21:40: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 24367 invoked from network); 12 Mar 2003 21:40:41 -0000 Received: from unknown (HELO zenia.red-bean.com) (66.244.67.22) by sources.redhat.com with SMTP; 12 Mar 2003 21:40:41 -0000 Received: from zenia.red-bean.com (localhost.localdomain [127.0.0.1]) by zenia.red-bean.com (8.12.5/8.12.5) with ESMTP id h2CLbP8A011504; Wed, 12 Mar 2003 16:37:26 -0500 Received: (from jimb@localhost) by zenia.red-bean.com (8.12.5/8.12.5/Submit) id h2CLbLmE011500; Wed, 12 Mar 2003 16:37:21 -0500 To: Daniel Jacobowitz Cc: gdb-patches@sources.redhat.com, Andreas Jaeger , Josef Zlomek , Michal Ludvig Subject: Re: RFA: Location list support for DWARF-2 References: <20030310160628.GA1988@nevyn.them.org> From: Jim Blandy Date: Wed, 12 Mar 2003 21:40:00 -0000 In-Reply-To: <20030310160628.GA1988@nevyn.them.org> Message-ID: User-Agent: Gnus/5.09 (Gnus v5.9.0) Emacs/21.2.92 MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2003-03/txt/msg00288.txt.bz2 Daniel Jacobowitz 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.