Re: [rfa] generate symbols associated to namespaces

[Daniel Jacobowitz <drow@mvista.com>]

[First, a request: could your next namespace patch be the
fully-qualified name inference code for full symbols?  Before I end up
hacking together something similar.  i.e. add_partial_structure, and
then all the determine_prefix related code.

I'd really, really like to see that in the next release of GDB; we'll
see...]

On Mon, Jun 16, 2003 at 01:21:36PM -0700, David Carlton wrote:
> This is the next part of my thrilling series of namespace patches; it
> generates symbols associated to namespaces.  The reason why these
> symbols are necessary is that, when evaluating an expression like
> 'C::x', you first evaluate 'C', and then look up 'x' in the class or
> namespace referred to by 'C'; if you don't know what namespaces are
> floating around, you get into trouble.  E.g. given this:
> 
> namespace C {
>   namespace C {
>     int oops;
>   }
> 
>   int x;
> 
>   void foo()
>   {
>     C::x;  // error!
>   }
> }
> 
> the reference to C::x within C::foo is an error, since 'C' refers to
> 'C::C', and there's no variable 'C::C::x'.  If we don't know that
> there's a namespace named 'C::C', we run into trouble when carrying
> out this behavior.

OK.  I see why these symbols are necessary.

> This patch doesn't actually use those symbols very much.  There will
> be a followup patch that modifies the parser and associated functions
> to use them, but this patch is already quite complicated enough as it
> is.
> 
> Unfortunately, if we don't have DW_TAG_namespace, then we have to work
> harder.  The solution I've chosen here is to look at demangled names
> of objects in that situation: if your object's demangled name is
> 'C::x', then maybe C is a namespace.  But the downside is that C might
> be a class instead.  The solution that I've adopted is to maintain two
> collections of namespace symbols: one collection is the ones we know
> to exist (because of DW_TAG_namespace), and the other collection is
> the ones that might be classes or namespaces (we're not sure, we
> deduced their existence via demangled names).  Then the C++-specific
> version of lookup_symbol only searches this latter collection if it
> has tried to look for a class by that name and has failed.

This makes sense.  I was worried about what we'd have to do with the
"potential" namespaces.

> There's (at least) one other piece of strangeness in the
> implementation.  Normally, we store symbols in the symtab associated
> to a file.  But I chose not to do that with namespace symbols, for a
> few reasons.  The main reason is that it makes the machinery to deduce
> possible namespace names from demangled names much more complicated
> and wasteful of memory.  A secondary reason is that namespaces really
> aren't associated to a single file, so why pretend that they are?
> (Also, it saves memory that way, too, since you only ever have one
> symbol associated to a namespace, instead of one symbol for each file
> that mentions the namespace.)  So I put namespaces in an artificial
> block in an artificial symtab; my recent dictionary patch makes this
> quite easy.  (I think that, in the long term, we should consider a
> similar process for C++ classes, especially templated classes, but
> that's a separate issue.)  These symbols are actually created by the
> DWARF 2 psymtab reader instead of the symtab reader.

Hold on a second.  There's a fundamental problem here.  While it is
true that namespaces may be shared across objfiles, and it is true that
it's more elegant to hold them separately from any objefile, you're
creating an always-expanding list of namespaces.  They will never be
freed.  Consider the problems this can cause:

prog1.cc:
namespace A {
  namespace A { int oops; }
  int x;
  void foo() { /* A::x; error */ x; }
}

prog2.cc:
namespace A {
  int x;
  void foo() { A::x; /* no error */ }
}

./gdb prog1
...
(gdb) maint cplus namespace 
Definite namespaces:
Possible namespaces:
A::A
A
(gdb) file ./prog2
Load new symbol table from "./prog2"? (y or n) y
Reading symbols from ./prog2...done.
(gdb) maint cplus namespace 
Definite namespaces:
Possible namespaces:
A::A
A

Oops!  You've still got A::A as a namespace.

In addition there are memory concerns with this fake objfile.  All in
all, I don't think this use of fake objfiles is legitimate.  I think
you're going to have to tie a fake block to each objfile instead, which
should be an acceptable compromise.  Definite namespaces would go in
the global block of any objfile where they are found, possible
namespaces would go in a special block in any objfile in which they are
inferred.  lookup_possible_namespace_symbol will gain a loop over all
objfiles.  A few functions will have to take an objfile parameter.

> The dwarf2read.c part of the patch is hard to read: you might just
> want to apply it and compare it to the original.  I've reorganized
> scan_partial_symbols to behave recursively instead of using that
> nesting_level and file_scope_level stuff; it's cleaner this way.
> 
> I've tested this on GCC 3.2, DWARF 2, i686-pc-linux-gnu.  I also
> tested gdb.c++/namespace.exp on a version of GCC 3.2 hacked to
> generated DW_TAG_namespace entries.  And I've been using this stuff on
> my branch for months.

I'd really appreciate it if you did this differently in the future. 
Believe it or not, I had C++ debugging working very nicely with stabs;
you don't need to make new tests pass with stabs, but please check that
you do not cause existing tests to regress.

The runtest invocation you'd need for this is "runtest --target_board
unix/gdb:debug_flags=-gstabs+".

It appears that your new code does not work with stabs debugging.  When
dwarf2-frame.c reaches the fake objfile, it gets an internal error
because SECT_OFF_TEXT is not initialized.  This appears to be a problem
in the dwarf2-frame code and I just sent a separate message about it.
With a workaround for that in place, the results for stabs look OK.
The three new tests fail:
+FAIL: gdb.c++/namespace.exp: ptype C
+FAIL: gdb.c++/namespace.exp: ptype E
+FAIL: gdb.c++/namespace.exp: maint cplus namespace

(Most of the other tests in namespace.exp fail too.)



By the way, the comment above lookup_namespace_scope is wrong, I think:
   For example, if we're within a function A::B::f and looking for a
   symbol f, this will get called with NAME = "f", SCOPE = "A::B", and
   SCOPE_LEN = 0.  It then calls itself with NAME and SCOPE the same,
   but with SCOPE_LEN = 1.  And then it calls itself with NAME and
   SCOPE the same, but with SCOPE_LEN = 4.  This third call looks for
   "A::B::x"; if it doesn't find it, then the second call looks for
   "A::x", and if that call fails, then the first call looks for
   "x".  */

Shouldn't it be "looking for a symbol x" and "NAME = "x""?  Much
clearer that way.


Comments on the actual code follow.  They're mostly minor.

> This needs symtab and C++ approval.  I'll be on vacation until next
> Tuesday or Wednesday.
> 
> David Carlton
> carlton@kealia.com
> 
> 2003-06-16  David Carlton  <carlton@kealia.com>
> 
> 	* gdbtypes.h: Add TYPE_CODE_NAMESPACE.
> 	* gdbtypes.c (init_type): Handle TYPE_CODE_NAMESPACE.
> 	(recursive_dump_type): Ditto.
> 	* printcmd.c (print_formatted): Ditto.
> 	* typeprint.c (print_type_scalar): Ditto.
> 	* c-typeprint.c (c_type_print_varspec_prefix): Ditto.
> 	(c_type_print_varspec_suffix, c_type_print_base): Ditto.
> 	* cp-support.h: Declare cp_check_namespace_symbol,
> 	cp_check_possible_namespace_symbols, maint_cplus_cmd_list.
> 	* cp-support.c: Make maint_cplus_cmd_list extern.
> 	* cp-namespace.c: Include objfiles.h, dictionary.h, command.h.
> 	(lookup_symbol_file): Look in namespace blocks when appropriate.
> 	(initialize_namespace_blocks): New.
> 	(get_namespace_block, get_possible_namespace_block)
> 	(free_namespace_blocks, get_namespace_objfile)
> 	(cp_check_namespace_symbol, check_namespace_symbol_block)
> 	(lookup_namespace_symbol, cp_check_possible_namespace_symbols)
> 	(check_possible_namespace_symbols_loop)
> 	(check_one_possible_namespace_symbol)
> 	(lookup_possible_namespace_symbol)
> 	(maintenance_cplus_namespace, _initialize_cp_namespace): Ditto.
> 	* block.h: Declare allocate_block.
> 	* block.c (allocate_block): New.
> 	* jv-lang.c (get_java_class_symtab): Allocate blocks via
> 	allocate_block.
> 	* symfile.h: Update declaration of add_psymbol_to_list.
> 	* symfile.c (add_psymbol_to_list): Return the partial symbol in
> 	question.
> 	* dwarf2read.c (dwarf2_build_psymtabs_hard): Do initial setting
> 	lowpc and highpc ourselves.  Add argument to
> 	scan_partial_symbols_call.
> 	(scan_partial_symbols): Restructure into a recursive version,
> 	calling add_partial_namespace and add_partial_enumeration when
> 	appropriate.
> 	(add_partial_symbol): If necessary, scan mangled names for names
> 	of namespaces.
> 	(add_partial_namespace): New.
> 	(add_partial_enumeration, locate_pdi_sibling): Ditto.
> 	* Makefile.in (cp-namespace.o): Depend on objfiles_h, gdbtypes_h,
> 	dictionary_h, command_h.
> 
> 2003-06-16  David Carlton  <carlton@kealia.com>
> 
> 	* gdb.c++/namespace.exp: Add tests for namespace types and maint
> 	cp namespace.
> 	* gdb.c++/maint.exp (test_help): Test 'help maint cp namespace'.
> 	(test_namespace): New.


> @@ -453,13 +493,323 @@ lookup_symbol_file (const char *name,
>        const struct block *global_block = block_global_block (block);
>        
>        if (global_block != NULL)
> -	return lookup_symbol_aux_block (name, linkage_name, global_block,
> -					domain, symtab);
> -      else
> -	return NULL;
> +	sym = lookup_symbol_aux_block (name, linkage_name, global_block,
> +				       domain, symtab);
> +
> +      if (sym == NULL || global_block == NULL)
> +	{
> +	  sym = lookup_namespace_symbol (name);
> +	  if (sym != NULL && symtab != NULL)
> +	    *symtab = NULL;
> +	}

Nit: You can just say if (sym == NULL) here, and it's a little clearer,
IMO.  Sym will be NULL from the call above this chunk.

>      }
>    else
>      {
> -      return lookup_symbol_global (name, linkage_name, domain, symtab);
> +      sym = lookup_symbol_global (name, linkage_name, domain, symtab);
> +    }



> +/* Now come functions for dealing with symbols associated to
> +   namespaces.  (They're used to store the namespaces themselves, not
> +   objects that live in the namespaces.)  Since namespaces span files,
> +   we create special blocks to store those symbols in instead of
> +   storing them in blocks associated to actual files.  That avoids
> +   duplication of symbols, among other issues.
> +
> +   Unfortunately, versions of GCC through at least 3.3 don't generate
> +   debugging information to tell us about the existence of namespaces.
> +   Our solution is to try to guess their existence by looking at
> +   demangled names.  This might cause us to misidentify classes as
> +   namespaces, however.  So we put those symbols in
> +   'possible_namespace_block' instead of 'namespace_block', and we
> +   only search that block as a last resort.  */
> +
> +/* FIXME: carlton/2003-06-12: Once versions of GCC that generate
> +   DW_TAG_namespace have been out for a year or two, we should get rid
> +   of possible_namespace_block and everything associated to it.  */

I don't think that's realistic.  But we can discuss it again in three
years :) Also, if I bother to add stabs support for this, we'll need
this code even longer.

> +
> +/* Allocate everything necessary for namespace_block and
> +   possible_namespace_block.  */
> +
> +static void
> +initialize_namespace_blocks (void)


> +/* A helper function used by cp_check_namespace_symbol and
> +   check_one_possible_namespace_symbol.  Looks to see if there is a
> +   symbol whose name is the initial substring of NAME of length LEN in
> +   block BLOCK; if not, adds it.  Return 1 if the symbol was already
> +   in there, 0 otherwise.  */
> +
> +static int
> +check_namespace_symbol_block (const char *name, int len,
> +			      struct block *block)

Hmm, the name doesn't really match what this function does.  How about
maybe_add_namespace_symbol?

> +{
> +  struct objfile *objfile = get_namespace_objfile ();
> +  char *name_copy = obsavestring (name, len, &objfile->symbol_obstack);
> +  struct symbol *sym = lookup_block_symbol (block, name_copy, NULL,
> +					    VAR_DOMAIN);
> +
> +  if (sym == NULL)
> +    {
> +      struct type *type = init_type (TYPE_CODE_NAMESPACE, 0, 0,
> +				     name_copy, objfile);
> +      TYPE_TAG_NAME (type) = TYPE_NAME (type);
> +
> +      sym = obstack_alloc (&objfile->symbol_obstack, sizeof (struct symbol));
> +      memset (sym, 0, sizeof (struct symbol));
> +      SYMBOL_LANGUAGE (sym) = language_cplus;
> +      SYMBOL_SET_NAMES (sym, name_copy, len, objfile);
> +      SYMBOL_CLASS (sym) = LOC_TYPEDEF;
> +      SYMBOL_TYPE (sym) = type;
> +      SYMBOL_DOMAIN (sym) = VAR_DOMAIN;
> +
> +      dict_add_symbol (BLOCK_DICT (block), sym);
> +
> +      return 0;
> +    }
> +  else
> +    {
> +      obstack_free (&objfile->symbol_obstack, name_copy);
> +
> +      return 1;
> +    }
> +}



> +/* This is a helper loop for cp_check_possible_namespace_symbols; it
> +   ensures that there are namespace symbols for all namespaces that
> +   are initial substrings of NAME of length at least LEN.  It returns
> +   1 if a previous loop had already created the shortest such symbol
> +   and 0 otherwise.
> +
> +   This function assumes that if there is already a symbol associated
> +   to a substring of NAME of a given length, then there are already
> +   symbols associated to all substrings of NAME whose length is less
> +   than that length.  So if cp_check_possible_namespace_symbols has
> +   been called once with argument "A::B::C::member", then that will
> +   create symbols "A", "A::B", and "A::B::C".  If it is then later
> +   called with argument "A::B::D::member", then the new call will
> +   generate a new symbol for "A::B::D", but once it sees that "A::B"
> +   has already been created, it doesn't bother checking to see if "A"
> +   has also been created.  */
> +
> +static int
> +check_possible_namespace_symbols_loop (const char *name, int len)
> +{
> +  if (name[len] == ':')
> +    {
> +      int done;
> +      int next_len = len + 2;
> +
> +      next_len += cp_find_first_component (name + next_len);
> +      done = check_possible_namespace_symbols_loop (name, next_len);
> +
> +      if (!done)
> +	{
> +	  done = check_one_possible_namespace_symbol (name, len);
> +	}

No braces here, please.

> +
> +      return done;
> +    }
> +  else
> +    return 0;
> +}
> +
> +/* Check to see if there's already a possible namespace symbol whose
> +   name is the initial substring of NAME of length LEN.  If not,
> +   create one and return 0; otherwise, return 1.  */
> +
> +static int
> +check_one_possible_namespace_symbol (const char *name, int len)
> +{
> +  return check_namespace_symbol_block (name, len,
> +				       get_possible_namespace_block ());
> +}
> +
> +/* Look for a symbol in possible_namespace_block named NAME.  */
> +
> +static struct symbol *
> +lookup_possible_namespace_symbol (const char *name)
> +{
> +  return lookup_block_symbol (get_possible_namespace_block (),
> +			      name, NULL, VAR_DOMAIN);
> +}

This function is less than the size of the comment describing why we
call it, a few pages up, and only called once.  Please just inline it
there, unless you have plans for it...

> +/* Read a partial die corresponding to an enumeration type.  */
> +
> +static char *
> +add_partial_enumeration (struct partial_die_info *enum_pdi, char *info_ptr,
> +			 struct objfile *objfile,
> +			 const struct comp_unit_head *cu_header,
> +			 const char *namespace)
> +{
> +  bfd *abfd = objfile->obfd;
> +  struct partial_die_info pdi;
> +
> +  if (enum_pdi->name != NULL)
> +    add_partial_symbol (enum_pdi, objfile, cu_header, namespace);
> +  
> +  while (1)
> +    {
> +      info_ptr = read_partial_die (&pdi, abfd, info_ptr, cu_header);
> +      if (pdi.tag == 0)
> +	break;
> +      gdb_assert (pdi.tag == DW_TAG_enumerator);
> +      gdb_assert (pdi.name != NULL);

No, that's not right.  Bad debug information should not cause GDB to
crash.  Complain and skip it instead.

> +      add_partial_symbol (&pdi, objfile, cu_header, namespace);
> +    }
> +
> +  return info_ptr;
> +}
> +
> +/* Locate ORIG_PDI's sibling; INFO_PTR should point to the next PDI
> +   after ORIG_PDI.  */

Strictly speaking INFO_PTR points to a DIE, not to a "PDI" which is
this GDB-internal construction.

> +
> +static char *
> +locate_pdi_sibling (struct partial_die_info *orig_pdi, char *info_ptr,
> +		    bfd *abfd, const struct comp_unit_head *cu_header)
> +{

> @@ -217,3 +219,18 @@ gdb_test "print X" "\\$\[0-9\].* = 9"
>  gdb_test "print 'G::Xg'" "\\$\[0-9\].* = 10"
>  gdb_test "print cXOtherFile" "No symbol \"cXOtherFile\" in current context."
>  gdb_test "print XOtherFile" "No symbol \"XOtherFile\" in current context."
> +
> +# Test to make sure that 'maint cplus namespace' is at least picking
> +# up one of the namespaces in this file.
> +
> +# FIXME: carlton/2003-06-16: We should check to make sure it picks up
> +# all of the namespaces.  Unfortunately, I can't guarantee what order
> +# they'll be listed in when you do 'maint cplus namespace'.  Probably
> +# I should stash the output of that command in a variable somewhere
> +# and examine that variable for all of the relevant namespaces.

Yeah, that would be nice.  It's not terribly hard but it requires a
little bit of TCL fu.

> +# FIXME: carlton/2003-06-16: This test (like many others in this file,
> +# doubtless) will fail in non-DWARF-2 situations; I need to go through
> +# and audit the tests accordingly at some point.
> +
> +gdb_test "maint cplus namespace" ".*C::C.*"

As I mentioned above, I'd rather fix this than add the FIXME.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer