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

[Daniel Jacobowitz <drow at mvista dot com>]

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