Re: [rfa] search for fields of this correctly, PR gdb/804

[Elena Zannoni <ezannoni at redhat dot com>]

David Carlton writes:
 > In lookup_symbol_aux, the 'field_of_this' search happens at the wrong
 > time: it happens after all of the superblocks of 'block' have been
 > searched, whereas fields of this should shadow file-level static and
 > global variables.
 > 

I see, the search stops as soon as you find something, so doing it in
that order would find the 'outer' first.

 > Here's a patch to fix this, together with a check for it in the
 > testsuite.  The idea is as follows.
 > 
 > * Modify lookup_symbol_aux_local to stop once it hits a static block.
 >   It also stores the address of that static block in a new argument to
 >   it, 'static_block'.
 > 

ah, I see what you did. It took me a while, must confess. You changed
the order from:

lookup blocks inner->outer included global and static.
test for is_a_field_of_this
lookup in symtabs and psymtabs

to:

lookup blocks inner->outer excluded global and static.
test for is_a_field_of_this
lookup in static block
lookup in symtabs and psymtabs

makes sense. But would the initial version of lookup_symbol_aux_local
look also in global block? Hmm,...I understand your doubts below. I
think you are right, the looking in the global block symtabs that
comes right after seems to take care of that.

 > * Pull out the code from lookup_symbol_aux_local that sets 'symtab'
 >   correctly and that does the fixup_symbol stuff into a new function,
 >   lookup_symbol_aux_block.
 > 

Since this bit is just moving code around, would you mind it checking
it in first, by itself? Then check in the rest.

 > * In lookup_symbol_aux, after doing the field_of_this stuff, call
 >   lookup_symbol_aux_block to search in the static block.
 > 
 > I waffled a bit as to whether or not we should preserve the current
 > behavior of searching the current file's global block before searching
 > all of the global blocks; I decided that doing so was redundant and a
 > bit messy, so I omitted that but left in a comment mentioning the
 > issue.

The only thing I wonder is if there's going to be a performance
penalty for this. Can you mention in the comment that there was old
code that was looking for symbols in the current file's global block?

 > 
 > I think the testsuite stuff is pretty straightforward; the current
 > version of m-data.exp was doing breakpoints on line numbers, which is
 > really fragile, so I modified that to look for a comment, like I did
 > when I changed m-static.exp a couple of months ago.
 > 

looks reasonable.

 > This patch is orthogonal to my lookup_symbol_aux_minsym patch that is
 > currently under review: they touch different parts of
 > lookup_symbol_aux.
 > 

Ok, go ahead.
It would be really nice if that 'goto' would disappear....hint, hint.

Elena


 > David Carlton
 > carlton@math.stanford.edu
 > 
 > 2002-11-13  David Carlton  <carlton@math.stanford.edu>
 > 
 > 	* symtab.c (lookup_symbol_aux_block): New function.
 > 	(lookup_symbol_aux_local): Move code into lookup_symbol_aux_block;
 > 	add 'static_block' argument.
 > 	(lookup_symbol_aux): Do the 'field_of_this' check before checking
 > 	the static block.  See PR gdb/804.
 > 
 > 2002-11-13  David Carlton  <carlton@math.stanford.edu>
 > 
 > 	* gdb.c++/m-data.exp: Add test for members that shadow global
 > 	variables: see PR gdb/804.
 > 	* gdb.c++/m-data.cc: Ditto.
 > 
 > Index: symtab.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/symtab.c,v
 > retrieving revision 1.78
 > diff -u -p -r1.78 symtab.c
 > --- symtab.c	6 Nov 2002 23:27:58 -0000	1.78
 > +++ symtab.c	13 Nov 2002 18:17:00 -0000
 > @@ -83,11 +83,20 @@ static struct symbol *lookup_symbol_aux 
 >  					 int *is_a_field_of_this,
 >  					 struct symtab **symtab);
 >  
 > -static struct symbol *lookup_symbol_aux_local (const char *name,
 > -					       const char *mangled_name,
 > -					       const struct block *block,
 > -					       const namespace_enum namespace,
 > -					       struct symtab **symtab);
 > +static
 > +struct symbol *lookup_symbol_aux_local (const char *name,
 > +					const char *mangled_name,
 > +					const struct block *block,
 > +					const namespace_enum namespace,
 > +					struct symtab **symtab,
 > +					const struct block **static_block);
 > +
 > +static
 > +struct symbol *lookup_symbol_aux_block (const char *name,
 > +					const char *mangled_name,
 > +					const struct block *block,
 > +					const namespace_enum namespace,
 > +					struct symtab **symtab);
 >  
 >  static
 >  struct symbol *lookup_symbol_aux_symtabs (int block_index,
 > @@ -789,11 +798,13 @@ lookup_symbol_aux (const char *name, con
 >    struct symtab *s = NULL;
 >    struct blockvector *bv;
 >    struct minimal_symbol *msymbol;
 > +  const struct block *static_block;
 >  
 > -  /* Search specified block and its superiors.  */
 > +  /* Search specified block and its superiors.  Don't search
 > +     STATIC_BLOCK or GLOBAL_BLOCK.  */
 >  
 >    sym = lookup_symbol_aux_local (name, mangled_name, block, namespace,
 > -				 symtab);
 > +				 symtab, &static_block);
 >    if (sym != NULL)
 >      return sym;
 >  
 > @@ -859,6 +870,28 @@ lookup_symbol_aux (const char *name, con
 >  	}
 >      }
 >  
 > +  /* If there's a static block to search, search it next.  */
 > +
 > +  /* NOTE: carlton/2002-11-13: There is a question as to whether or
 > +     not it would be appropriate to search the current global block
 > +     here as well.  On the one hand, it's redundant with the
 > +     lookup_symbol_aux_symtabs search that happens next.  On the other
 > +     hand, if decode_line_1 is passed an argument like filename:var,
 > +     then the user presumably wants 'var' to be searched for in
 > +     filename.  On the third hand, there shouldn't be multiple global
 > +     variables all of which are named 'var', and it's not like
 > +     decode_line_1 has ever restricted its search to only global
 > +     variables in a single filename.  All in all, only searching the
 > +     static block here seems best: it's correct and it's cleanest.  */
 > +
 > +  if (static_block != NULL)
 > +    {
 > +      sym = lookup_symbol_aux_block (name, mangled_name, static_block,
 > +				     namespace, symtab);
 > +      if (sym != NULL)
 > +	return sym;
 > +    }
 > +
 >    /* Now search all global blocks.  Do the symtab's first, then
 >       check the psymtab's. If a psymtab indicates the existence
 >       of the desired name as a global, then do psymtab-to-symtab
 > @@ -1064,13 +1097,49 @@ lookup_symbol_aux (const char *name, con
 >    return NULL;
 >  }
 >  
 > -/* Check to see if the symbol is defined in BLOCK or its
 > -   superiors.  */
 > +/* Check to see if the symbol is defined in BLOCK or its superiors.
 > +   Don't search STATIC_BLOCK or GLOBAL_BLOCK.  If we don't find a
 > +   match, store the address of STATIC_BLOCK in static_block.  */
 >  
 >  static struct symbol *
 >  lookup_symbol_aux_local (const char *name, const char *mangled_name,
 >  			 const struct block *block,
 >  			 const namespace_enum namespace,
 > +			 struct symtab **symtab,
 > +			 const struct block **static_block)
 > +{
 > +  struct symbol *sym;
 > +  
 > +  /* Check if either no block is specified or it's a global block.  */
 > +
 > +  if (block == NULL || BLOCK_SUPERBLOCK (block) == NULL)
 > +    {
 > +      *static_block = NULL;
 > +      return NULL;
 > +    }
 > +
 > +  while (BLOCK_SUPERBLOCK (BLOCK_SUPERBLOCK (block)) != NULL)
 > +    {
 > +      sym = lookup_symbol_aux_block (name, mangled_name, block, namespace,
 > +				     symtab);
 > +      if (sym != NULL)
 > +	return sym;
 > +      block = BLOCK_SUPERBLOCK (block);
 > +    }
 > +
 > +  /* We've reached the static block.  */
 > +
 > +  *static_block = block;
 > +  return NULL;
 > +}
 > +
 > +/* Look up a symbol in a block; if found, locate its symtab, fixup the
 > +   symbol, and set block_found appropriately.  */
 > +
 > +static struct symbol *
 > +lookup_symbol_aux_block (const char *name, const char *mangled_name,
 > +			 const struct block *block,
 > +			 const namespace_enum namespace,
 >  			 struct symtab **symtab)
 >  {
 >    struct symbol *sym;
 > @@ -1078,32 +1147,28 @@ lookup_symbol_aux_local (const char *nam
 >    struct blockvector *bv;
 >    struct block *b;
 >    struct symtab *s = NULL;
 > -  
 > -  while (block != 0)
 > +
 > +  sym = lookup_block_symbol (block, name, mangled_name, namespace);
 > +  if (sym)
 >      {
 > -      sym = lookup_block_symbol (block, name, mangled_name, namespace);
 > -      if (sym)
 > +      block_found = block;
 > +      if (symtab != NULL)
 >  	{
 > -	  block_found = block;
 > -	  if (symtab != NULL)
 > +	  /* Search the list of symtabs for one which contains the
 > +	     address of the start of this block.  */
 > +	  ALL_SYMTABS (objfile, s)
 >  	    {
 > -	      /* Search the list of symtabs for one which contains the
 > -	         address of the start of this block.  */
 > -	      ALL_SYMTABS (objfile, s)
 > -	      {
 > -		bv = BLOCKVECTOR (s);
 > -		b = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
 > -		if (BLOCK_START (b) <= BLOCK_START (block)
 > -		    && BLOCK_END (b) > BLOCK_START (block))
 > -		  goto found;
 > -	      }
 > -	    found:
 > -	      *symtab = s;
 > +	      bv = BLOCKVECTOR (s);
 > +	      b = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
 > +	      if (BLOCK_START (b) <= BLOCK_START (block)
 > +		  && BLOCK_END (b) > BLOCK_START (block))
 > +		goto found;
 >  	    }
 > -
 > -	  return fixup_symbol_section (sym, objfile);
 > +	found:
 > +	  *symtab = s;
 >  	}
 > -      block = BLOCK_SUPERBLOCK (block);
 > +      
 > +      return fixup_symbol_section (sym, objfile);
 >      }
 >  
 >    return NULL;
 > Index: m-data.exp
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/testsuite/gdb.c++/m-data.exp,v
 > retrieving revision 1.1
 > diff -u -p -r1.1 m-data.exp
 > --- m-data.exp	27 May 2002 21:46:52 -0000	1.1
 > +++ m-data.exp	13 Nov 2002 17:44:41 -0000
 > @@ -54,9 +54,12 @@ if ![runto_main] then {
 >      continue
 >  }
 >  
 > +# First, run to after we've constructed all the gnu_obj_N's:
 > +
 > +gdb_breakpoint [gdb_get_line_number "first-constructs-done"]
 > +gdb_continue_to_breakpoint "end of first constructors"
 > +
 >  # One.
 > -gdb_test "break 50" "Breakpoint \[0-9\]*.*line 50\\."
 > -gdb_test "continue" "Continuing\\.\r\n\r\nBreakpoint.*at.*m-data\\.cc:50\r\n.*" "continue to 50"
 >  
 >  # simple object, const bool
 >  gdb_test "print test1.test" "\\$\[0-9\]* = true" "simple object, const bool"
 > @@ -71,8 +74,6 @@ gdb_test "print test1.key2" "\\$\[0-9\]*
 >  gdb_test "print test1.value" "\\$\[0-9\]* = egyptian" "simple object, enum"
 >  
 >  # Two.
 > -gdb_test "break 51" "Breakpoint \[0-9\]*.*line 51\\."
 > -gdb_test "continue" "Continuing\\.\r\n\r\nBreakpoint.*at.*m-data\\.cc:51\r\n.*" "continue to 51"
 >  
 >  # derived template object, base const bool
 >  gdb_test "print test2.test" "\\$\[0-9\]* = true" "derived template object, base const bool"
 > @@ -90,8 +91,6 @@ gdb_test "print test2.value" "\\$\[0-9\]
 >  gdb_test "print test2.value_derived" "\\$\[0-9\]* = roman" "derived template object, derived enum"
 >  
 >  # Three.
 > -gdb_test "break 52" "Breakpoint \[0-9\]*.*line 52\\."
 > -gdb_test "continue" "Continuing\\.\r\n\r\nBreakpoint.*at.*m-data\\.cc:52\r\n.*" "continue to 52"
 >  
 >  # template object, derived template data member's base const bool
 >  gdb_test "print test3.data.test" "\\$\[0-9\]* = true" "template object, const bool"
 > @@ -107,6 +106,14 @@ gdb_test "print test3.data.value" "\\$\[
 >  
 >  # template object, derived template data member's enum
 >  gdb_test "print test3.data.value_derived" "\\$\[0-9]\* = etruscan" "template object, derived enum"
 > +
 > +# Now some tests for shadowing (see PR gdb/804):
 > +
 > +gdb_breakpoint "C::marker"
 > +gdb_continue_to_breakpoint "continue to shadow breakpoint"
 > +
 > +gdb_test "print shadow" "\\$\[0-9]\* = 1" "shadowing member"
 > +gdb_test "print ::shadow" "\\$\[0-9]\* = 0" "shadowed global variable"
 >  
 >  gdb_exit
 >  return 0
 > Index: m-data.cc
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/testsuite/gdb.c++/m-data.cc,v
 > retrieving revision 1.1
 > diff -u -p -r1.1 m-data.cc
 > --- m-data.cc	27 May 2002 21:46:52 -0000	1.1
 > +++ m-data.cc	13 Nov 2002 17:44:37 -0000
 > @@ -41,7 +41,18 @@ namespace __gnu_test
 >      public:
 >        gnu_obj_3(antiquities b): data(etruscan) { }
 >      }; 
 > -} 
 > +}
 > +
 > +int shadow = 0;
 > +
 > +class C
 > +{
 > +public:
 > +  C (int x) : shadow (x) {}
 > +  void marker () {}
 > +private:
 > +  int shadow;
 > +};
 >  
 >  int main()
 >  {
 > @@ -49,5 +60,9 @@ int main()
 >    gnu_obj_1		test1(egyptian, 4589);
 >    gnu_obj_2<long>	test2(roman);
 >    gnu_obj_3<long>	test3(greek);
 > +
 > +  C theC (1);				// breakpoint: first-constructs-done
 > +  theC.marker ();
 > +  
 >    return 0;
 >  }