From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9094 invoked by alias); 11 Mar 2003 21:14:35 -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 9086 invoked from network); 11 Mar 2003 21:14:34 -0000 Received: from unknown (HELO jackfruit.Stanford.EDU) (171.64.38.136) by 172.16.49.205 with SMTP; 11 Mar 2003 21:14:34 -0000 Received: (from carlton@localhost) by jackfruit.Stanford.EDU (8.11.6/8.11.6) id h2BLEGn18347; Tue, 11 Mar 2003 13:14:16 -0800 X-Authentication-Warning: jackfruit.Stanford.EDU: carlton set sender to carlton@math.stanford.edu using -f To: Daniel Jacobowitz Cc: gdb-patches@sources.redhat.com, Elena Zannoni , Jim Blandy Subject: Re: [rfa] annotate blocks with C++ namespace information References: <20030311171133.GA3362@nevyn.them.org> From: David Carlton Date: Tue, 11 Mar 2003 21:14:00 -0000 In-Reply-To: <20030311171133.GA3362@nevyn.them.org> Message-ID: User-Agent: Gnus/5.0808 (Gnus v5.8.8) XEmacs/21.4 (Common Lisp) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2003-03/txt/msg00260.txt.bz2 On Tue, 11 Mar 2003 12:11:33 -0500, Daniel Jacobowitz said: > On Fri, Feb 21, 2003 at 05:47:35PM -0800, David Carlton wrote: >> --- 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 :) You weren't joking about the 'nit-picking pedantic'. :-) >> + /* 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. They're ints, not pointers. You might be remembering an earlier version of this where I used pointers in this loop; I decided that it was cleaner for cp_find_first_component to return an int instead of a const char *. >> +/* 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"? Right. >> + { >> + /* 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. Yeah, there's some thinking to be done on this matter down the road; we'll want to specify some of this behavior more precisely once we really tackle the nested classes issue. For now, though, it works fine: about all that can happen is that class members might get detected in search for namespace members (once the relevant code has been added to lookup_symbol_aux), which is innocuous enough: they'll already have gotten detected by the is_a_field_of_this test anyways. (Which, of course, opens up the tantalizing possibility of eventually reorganizing all of the namespace/class code to make the is_a_field_of_this test unnecessary, but that's for far in the future.) >> 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. I don't really care one way or another; I just figure that, since I typed most of the characters in that file (at least in the version on the branch), I might as well get some credit. :-) Honestly, though, I'm happy just to leave it as "Contributed by MontaVista Software." > - 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. My situation is, I am learning (because I get asked this question not infrequently), a bit unusual: no, I don't have an employer disclaimer from Stanford, but that's okay since the FSF and I agree that I don't need one. Stanford's employment contract for faculty members makes it quite clear that Stanford doesn't own the copyright for software that I write (with some exceptions that clearly don't apply to me), even if I write it using Stanford's computers. (Similarly, Stanford doesn't own the copyright to articles or books that I write.) I'm just mentioning Stanford out of politeness. >> +/* 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. Yes; it should be easy enough to rewrite, say, find_last_component using cp_find_first_component. The only reason why I didn't do that (other than laziness) was that then I'd want to go figure out a situation where find_last_component actually gets called, to make sure I didn't make a boneheaded mistake while doing so, and I didn't feel like doing that. But I'll definitely add a FIXME comment about that? >> + - 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. Right. >> +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. There's also operator-> to worry about. And, now that you mention it, I'm not handling the possibility of whitespace between 'operator' and the actual operator. The function isn't entirely bullet-proof when given user input (you could construct malformed user input containing single colons that would cause it problems, I think); I should probably add a comment saying it's intended for internal use only, at least for now. (Though the similar function in linespec.c has the same flaw.) Do any demanglers put in spaces after 'operator'? I hope not... > 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. Okay, will do. >> +/* 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. Sure, that makes sense. > The rest looks good to me; let's see what the symtabs/dwarf2 people > have to say. Thanks! David Carlton carlton@math.stanford.edu