Re: [rfa] annotate blocks with C++ namespace information

[David Carlton <carlton at math dot stanford dot edu>]

On Tue, 11 Mar 2003 12:11:33 -0500, Daniel Jacobowitz <drow at mvista dot com> 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 at math dot stanford dot edu