Re: [patch] Improve symbol lookup performance noted in PR 15519

[Pedro Alves <palves@redhat.com>]

On 06/05/2013 11:31 PM, Doug Evans wrote:
> On Mon, Jun 3, 2013 at 10:27 AM, Pedro Alves <palves@redhat.com> wrote:
>> > Just to make sure I understand the change -- I see
>> > cp_lookup_symbol_namespace does:
...
>> > and a chunk of the speedup comes from skipping that, correct?
>> > That is, it is supposedly unnecessary to look symbol imports
>> > when looking up a symbol in a baseclass, right?
> Right.
> There are two kinds of "using"s: directives and declarations.
> "using directives" cannot appear in class scope, and that is what
> cp_lookup_symbol_namespace handles.
> And, AFAICT, gdb doesn't support "using declarations" yet,
> which does affect base class lookup here.
> 
>> > The patch then also replaces a lookup_symbol_static with a specific
>> > block call followed by a fallback lookup_static_symbol_aux search over all
>> > objfiles, by always doing the lookup_static_symbol_aux search over
>> > all objfiles.  It makes me wonder if it was there for a reason things were
>> > done that way before, like for something like the same named class/methods
>> > being implemented/hidden/private in different sos/dlls (therefore not
>> > really subject to ODR), therefore making it desirable to lookup in the
>> > same context first.  I have no idea, probably not.  :-)  I guess I'm just
>> > after getting the analysis/conclusion that led to the change
>> > recorded for posterity.  :-)
> It's kind of a toss up.
> Even if someone played games with visibility the previous lookup only
> checks the current symtab; there's no guarantee the needed debug info
> isn't in another symtab in that objfile (e.g., due to gcc's aggressive
> debug info pruning).
> Ultimately, I think what we want is to search the current objfile, and
> then search the remaining objfiles, but I left that for another day.
> Still, searching the current symtab isn't expensive enough that
> re-searching it is a problem and it sometimes will win.
> So I've modified the patch to keep that behaviour.
> Still have to search the world if block != NULL though.  Improving
> that's also left for another day.

Thanks.  While at it, a couple comments:

>  # Check inheritance of typedefs.
> -foreach klass {"A" "D" "E" "F"} {
> +foreach klass {"A" "D" "E" "F" "A2" "D2"} {
>      gdb_test "ptype ${klass}::value_type" "type = int"
>      gdb_test "whatis ${klass}::value_type" "type = int"
>      gdb_test "p (${klass}::value_type) 0" " = 0"
> @@ -57,6 +58,13 @@ if ![runto 'marker1'] then {
>      continue
>  }
>  
> +# Check inheritance of typedefs again, but this time with an active block.
> +foreach klass {"A" "D" "A2" "D2"} {
> +    gdb_test "ptype ${klass}::value_type" "type = int"
> +    gdb_test "whatis ${klass}::value_type" "type = int"
> +    gdb_test "p (${klass}::value_type) 0" " = 0"
> +}
> +

Looks like this will create duplicate messages in gdb.sum.
Could you use with_test_prefix to make them unique please?

>  gdb_test "up" ".*main.*" "up from marker1"
>  
>  # Print class types and values.
> Index: testsuite/gdb.cp/derivation2.cc
> ===================================================================
> RCS file: testsuite/gdb.cp/derivation2.cc
> diff -N testsuite/gdb.cp/derivation2.cc
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ testsuite/gdb.cp/derivation2.cc	5 Jun 2013 22:21:18 -0000
> @@ -0,0 +1,49 @@
...
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +   */

(this placement for */ looks a little odd.)

-- 
Pedro Alves