From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29262 invoked by alias); 11 Mar 2003 17:11:41 -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 29254 invoked from network); 11 Mar 2003 17:11:41 -0000 Received: from unknown (HELO crack.them.org) (65.125.64.184) by 172.16.49.205 with SMTP; 11 Mar 2003 17:11:41 -0000 Received: from nevyn.them.org ([66.93.61.169] ident=mail) by crack.them.org with asmtp (Exim 3.12 #1 (Debian)) id 18spBE-000307-00; Tue, 11 Mar 2003 13:12:52 -0600 Received: from drow by nevyn.them.org with local (Exim 3.36 #1 (Debian)) id 18snHp-0001tH-00; Tue, 11 Mar 2003 12:11:33 -0500 Date: Tue, 11 Mar 2003 17:11:00 -0000 From: Daniel Jacobowitz To: David Carlton Cc: gdb-patches@sources.redhat.com, Elena Zannoni , Jim Blandy Subject: Re: [rfa] annotate blocks with C++ namespace information Message-ID: <20030311171133.GA3362@nevyn.them.org> Mail-Followup-To: David Carlton , gdb-patches@sources.redhat.com, Elena Zannoni , Jim Blandy References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.1i X-SW-Source: 2003-03/txt/msg00254.txt.bz2 On Fri, Feb 21, 2003 at 05:47:35PM -0800, David Carlton wrote: > I've tested this as follows: I've run it through the test suite, both > with a stock GCC 3.1 and with a version of GCC 3.2 modified to > generate debug info for namespaces. (All on i686-pc-linux-gnu with > DWARF 2.) There were no regressions; good thing, since it's not > supposed to change GDB's output at all! I also made sure that the > versions of these functions that I'm submitting here were identical to > the versions of the functions on my branch, wherever possible; that > branch has a much wider range of functionality (and testsuite tests), > which actually do exercise the functionality that this patch adds. > > How does it look? I guess I need approval both from the symtab side > and from the C++ side. Well, here's comments from the C++ side. More precisely, I guess, comments from the nit-picking pedantic side... > --- buildsym.c 20 Feb 2003 17:17:23 -0000 1.29 > +++ buildsym.c 22 Feb 2003 00:46:55 -0000 > @@ -44,6 +44,7 @@ > #include "macrotab.h" > #include "demangle.h" /* Needed by SYMBOL_INIT_DEMANGLED_NAME. */ > #include "block.h" > +#include "cp-support.h" > /* Ask buildsym.h to define the vars it normally declares `extern'. */ Blank line :) > + /* We've found a component of the name that's an anonymous > + namespace. So add symbols in it to the namespace given > + by the previous component if there is one, or to the > + global namespace if there isn't. */ > + add_using_directive (name, > + previous_component == 0 > + ? 0 : previous_component - 2, > + next_component); Use NULL for zero pointers, please. > +/* Add a using directive to using_list. NAME is the start of a string > + that should contain the namespaces we want to add as initial > + substrings, OUTER_LENGTH is the end of the outer namespace, and > + INNER_LENGTH is the end of the inner namespace. If the using > + directive in question has already been added, don't add it > + twice. */ > + > +void > +add_using_directive (const char *name, unsigned int outer_length, > + unsigned int inner_length) > +{ > + struct using_direct *current; > + struct using_direct *new; > + > + /* Has it already been added? */ > + > + for (current = using_list; current != NULL; current = current->next) > + { > + if ((strncmp (current->inner, name, inner_length) == 0) > + && (strlen (current->inner) == inner_length) > + && (strlen (current->outer) == outer_length)) > + return; > + } > + > + using_list = cp_add_using (name, inner_length, outer_length, > + using_list); > +} > + Just checking, but right now this adds namespaces like "foo::(anonymous namespace)" to the using list of "foo"? And eventually, it will add things like "foo::bar" to the using list of "foo"? > + { > + /* Try to figure out the appropriate namespace from the > + demangled name. */ > + > + /* FIXME: carlton/2003-02-21: If the function in > + question is a method of a class, the name will > + actually include the name of the class as well. This > + should be harmless, but is a little unfortunate. */ > + > + const char *name = SYMBOL_CPLUS_DEMANGLED_NAME (symbol); > + unsigned int prefix_len = cp_entire_prefix_len (name); > + > + block_set_scope (block, > + obsavestring (name, prefix_len, > + &objfile->symbol_obstack), > + &objfile->symbol_obstack); > + } > + } Yes, that is a bit unfortunate. It may cause us issues down the line; but we'll deal with it when it comes up. > Index: cp-support.c > =================================================================== > RCS file: /cvs/src/src/gdb/cp-support.c,v > retrieving revision 1.1 > diff -u -p -r1.1 cp-support.c > --- cp-support.c 14 Sep 2002 02:09:39 -0000 1.1 > +++ cp-support.c 22 Feb 2003 00:46:29 -0000 > @@ -1,7 +1,7 @@ > /* Helper routines for C++ support in GDB. > - Copyright 2002 Free Software Foundation, Inc. > + Copyright 2002, 2003 Free Software Foundation, Inc. > > - Contributed by MontaVista Software. > + Contributed by MontaVista Software and Stanford University. Two nits: - How about adding "Namespace supported contributed by Stanford University" instead? The file wasn't originally contributed by Stanford. - This may be obvious, but it implies that you have an employer disclaimer from Stanford in addition to a personal assignment, if Stanford is contributing code. I'd just like to double-check that that's accurate. I don't have access to the assignments data. > +/* Here are some random pieces of trivia to keep in mind while trying > + to take apart demangled names: You're adding cp_find_first_component, which seems to me to duplicate logic from find_last_component among other places. I think we have either three or four subtly different copies of this logic now. Is it really necessary? It's not precisely the same (you're going in the other direction) but it would be really nice to condense this. > + - Conversely, even if you're trying to deal with a function, its > + demangled name might not end with ')': it could be a const or > + volatile class method, in which case it ends with "const" or > + "volatile". However, in a demangled method-name-with-arguments the rightmost ) is the end of the arguments list. Right? I know we're already using that assumption. > +unsigned int > +cp_find_first_component (const char *name) > +{ > + /* Names like 'operator<<' screw up the recursion, so let's > + special-case them. I _hope_ they can only occur at the start of > + a component. */ > + > + unsigned int index = 0; > + > + if (strncmp (name, "operator", LENGTH_OF_OPERATOR) == 0) I think that handling operators correctly would be simpler than I thought previously. All we should have to do is, when we hit a '<', check if the preceding word is "operator". It's still not entirely trivial (there might be a space after operator, or not; there must be something indicating word-break or beginning-of-string before operator) but it's pretty simple. If you're not interested in trying this that's OK. I can look at it later once this is used. We should probably expose this function via maint (perhaps "maint cplus first_component"?) so that we can unit-test it. Could you add that to this patch? Creating the new command menu is pretty easy; see MIPS for an example how. There may be better examples. > +/* Create a zero-terminated copy of the initial substring of STRING of > + length LEN. Allocate memory via xmalloc. */ > + > +static char * > +xstrndup (const char *string, size_t len) > +{ > + char *retval = xmalloc (len + 1); > + > + strncpy (retval, string, len); > + retval[len] = '\0'; > + > + return retval; > } Please put this in utils.c rather than in a C++ support file. The rest looks good to me; let's see what the symtabs/dwarf2 people have to say. -- Daniel Jacobowitz MontaVista Software Debian GNU/Linux Developer