Re: [RFA] massively speed up "info var foo" on large programs

[Pedro Alves <palves at redhat dot com>]

On 05/28/2012 05:49 AM, Doug Evans wrote:

> On Fri, May 25, 2012 at 1:50 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 05/25/2012 09:21 AM, Doug Evans wrote:
>>
>>>
>>> The output is different from the previous code, I didn't take into
>>> account the symbols that gdb creates for @plt entries.  I think if we
>>> want to continue to provide the current output, we should add an
>>> option to "info var|fun|type" to produce it: the normal case shouldn't
>>> be that slow.
>>
>>
>> Different how?  The patch has no testsuite updates, so the email reader is
>> left wondering.  ;-)
> 
> In the "Non-debugging symbols" section of the output, when a symbol
> would have been found in another objfile, the code would have not
> printed the non-@plt form of the function name.
> With this patch we have a decision to make.  Searching all the other
> objfiles is not reasonable (IMO) so what to do?  I can think of two
> possibilities: always print it or never print it.  Since the symbol in
> question is an artificial symbol created by gdb I have opted for never
> printing it.
> 
> Thus instead of seeing this in the "Non-debugging symbols" section of
> the output:
> 
> 0x1234 foo@plt
> 0x1234 foo
> 
> the output will contain:
> 
> 0x1234 foo@plt
> 
> Here is v3 of the patch.  I added a testcase.
> Regression tested on amd64-linux.
> 


Took me a while, and a gdb.log diff of the new testcase between
patched and unpatched gdb to grok this.  I think this output change
is okay.

> @@ -3463,21 +3505,13 @@ search_symbols (char *regexp, enum searc
>  		|| regexec (&datum.preg, SYMBOL_NATURAL_NAME (msymbol), 0,
>  			    NULL, 0) == 0)
>  	      {
> -		if (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol)))
> -		  {
> -		    /* FIXME: carlton/2003-02-04: Given that the
> -		       semantics of lookup_symbol keeps on changing
> -		       slightly, it would be a nice idea if we had a
> -		       function lookup_symbol_minsym that found the
> -		       symbol associated to a given minimal symbol (if
> -		       any).  */
> -		    if (kind == FUNCTIONS_DOMAIN
> -			|| lookup_symbol (SYMBOL_LINKAGE_NAME (msymbol),
> -					  (struct block *) NULL,
> -					  VAR_DOMAIN, 0)

There's a comment above all this that talks about "lookup_symbol" that needs
updating.

> -			== NULL)
> -		      found_misc = 1;
> -		  }
> +		/* Note: An important side-effect of these lookup functions
> +		   is to expand the symbol table if msymbol is found.  */

Okay, it's an important side-effect, but why do we want the side effect
should IMO be mentioned. It's not obvious to me (a casual reader).

> +		if (kind == FUNCTIONS_DOMAIN
> +		    ? find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol)) == NULL
> +		    : lookup_msymbol_in_objfile (objfile, msymbol,
> +						 VAR_DOMAIN) == NULL)

The "lookup_msymbol_in_objfile" function name threw me off for a few
minutes, as in, "if we already know the msymbol exists in objfile, wth
are we looking it up again?  For those important side-effects perhaps?"
More below.

> +		  found_misc = 1;
>  	      }
>
> +/* Wrapper around lookup_symbol_aux_objfile for search_symbols.
> + Look up MSYMBOL in DOMAIN in the global and static blocks of OBJFILE
> + and all related objfiles. */
> +
> +static struct symbol *
> +lookup_msymbol_in_objfile (struct objfile *objfile,
> + struct minimal_symbol *msymbol,
> + domain_enum domain)
> +{
> + const char *name = SYMBOL_LINKAGE_NAME (msymbol);

Ah, MSYMBOL is the input, not the output!  The lookup_symbol_xxx
functions return a symbol, so I was naively expecting this to return
an msymbol.  IMO it'd be clearer if it'd be called something like
lookup_symbol_of_msymbol_in_objfile then.  But, the only usage of MSYMBOL
in the entire function is extracting its name. This would then be much
simpler, clearer and reusable IMO if the prototype and name of
the function was:

static struct symbol *
lookup_symbol_in_objfile (struct objfile *objfile,
                          const char *name,
                          domain_enum domain);

And the caller did the SYMBOL_LINKAGE_NAME (msymbol) thing.

-- 
Pedro Alves