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

[Doug Evans <dje@google.com>]

On Fri, Jun 1, 2012 at 12:38 PM, Pedro Alves <palves@redhat.com> wrote:
> 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.

Yeah, already updated in my sandbox.

>> -                     == 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).

Most of gdb's symbol handling is arguably non-obvious.   1/2 :-)
I've expanded the comment.

>> +             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.

The comment for the function is pretty unambiguous.

>
>> +               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.

Actually, I had a similarly named function and prototype early on, but
I didn't like it.  GDB's symbol support is a mess (IMO), and I wanted
a name and usage to restrict it to the task at hand.  Anything more
general and I was pretty sure this patch would get bogged down.
Later on I want to revamp the symbol API, but I don't want this patch
tied down by that.
(For example, I don't want to bubble up the semantics of
demangle_for_lookup to the caller of this function.   Here we have an
msymbol, we know we have a mangled name.  If you want, I can go with
lookup_symbol_in_objfile but rename it to
lookup_symbol_in_objfile_from_linkage_name or some such.)