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

[Doug Evans <dje@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]

On Mon, Jun 3, 2013 at 10:27 AM, Pedro Alves <palves@redhat.com> wrote:
> What I guess I'm still missing to understand it,
> is a short blurb describing what is being skipped and why is
> it safe to be skipped.  :-)
>
> Just to make sure I understand the change -- I see
> cp_lookup_symbol_namespace does:
>
> ...
>   /* Search for name in namespaces imported to this and parent
>      blocks.  */
>   while (block != NULL)
>     {
>       sym = cp_lookup_symbol_imports (scope, name, block,
>                                       domain, 0, 1);
>
>       if (sym)
>         return sym;
>
>       block = BLOCK_SUPERBLOCK (block);
>     }
>
> 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.

[-- Attachment #2: gdb-130605-cp-namespace-3.patch.txt --]
[-- Type: text/plain, Size: 7104 bytes --]

2013-06-05  Doug Evans  <dje@google.com>
	    Keith Seitz  <keiths@redhat.com>

	PR 15519
	* cp-namespace.c (find_symbol_in_baseclass): Call
	cp_lookup_symbol_in_namespace instead of cp_lookup_symbol_namespace.
	Check result of call to lookup_symbol_static.
	Call lookup_static_symbol_aux unconditionally.
	Call check_typedef on base types before accessing them.
	(cp_lookup_nested_symbol): Fix comment.

	testsuite/
	* gdb.cp/derivation2.cc: New file.
	* gdb.cp/derivation.cc (main): Call foo2.
	* gdb.cp/derivation.exp: Add tests for typedefs in another
	file, and when there's an active block.

Index: cp-namespace.c
===================================================================
RCS file: /cvs/src/src/gdb/cp-namespace.c,v
retrieving revision 1.67
diff -u -p -r1.67 cp-namespace.c
--- cp-namespace.c	30 May 2013 17:29:06 -0000	1.67
+++ cp-namespace.c	5 Jun 2013 22:21:17 -0000
@@ -720,36 +720,40 @@ find_symbol_in_baseclass (struct type *p
   for (i = 0; i < TYPE_N_BASECLASSES (parent_type); ++i)
     {
       size_t len;
+      struct type *base_type = TYPE_BASECLASS (parent_type, i);
       const char *base_name = TYPE_BASECLASS_NAME (parent_type, i);
 
       if (base_name == NULL)
 	continue;
 
       /* Search this particular base class.  */
-      sym = cp_lookup_symbol_namespace (base_name, name, block, VAR_DOMAIN);
+      sym = cp_lookup_symbol_in_namespace (base_name, name, block,
+					   VAR_DOMAIN, 0);
       if (sym != NULL)
 	break;
 
+      /* Now search all static file-level symbols.  We have to do this for
+	 things like typedefs in the class.  First search in this symtab,
+	 what we want is possibly there.  */
       len = strlen (base_name) + 2 + strlen (name) + 1;
       concatenated_name = xrealloc (concatenated_name, len);
       xsnprintf (concatenated_name, len, "%s::%s", base_name, name);
       sym = lookup_symbol_static (concatenated_name, block, VAR_DOMAIN);
+      if (sym != NULL)
+	break;
 
-      /* If there is currently no BLOCK, e.g., the inferior hasn't yet
-	 been started, then try searching all STATIC_BLOCK symbols in
-	 all objfiles.  */
-      if (block == NULL)
-	{
-	  sym = lookup_static_symbol_aux (concatenated_name, VAR_DOMAIN);
-	  if (sym != NULL)
-	    break;
-	}
+      /* Nope.  We now have to search all static blocks in all objfiles,
+	 even if block != NULL, because there's no guarantees as to which
+	 symtab the symbol we want is in.  */
+      sym = lookup_static_symbol_aux (concatenated_name, VAR_DOMAIN);
+      if (sym != NULL)
+	break;
 
       /* If this class has base classes, search them next.  */
-      if (TYPE_N_BASECLASSES (TYPE_BASECLASS (parent_type, i)) > 0)
+      CHECK_TYPEDEF (base_type);
+      if (TYPE_N_BASECLASSES (base_type) > 0)
 	{
-	  sym = find_symbol_in_baseclass (TYPE_BASECLASS (parent_type, i),
-					  name, block);
+	  sym = find_symbol_in_baseclass (base_type, name, block);
 	  if (sym != NULL)
 	    break;
 	}
@@ -797,8 +801,8 @@ cp_lookup_nested_symbol (struct type *pa
 	if (sym != NULL)
 	  return sym;
 
-	/* Now search all static file-level symbols.  Not strictly
-	   correct, but more useful than an error.  We do not try to
+	/* Now search all static file-level symbols.  We have to do this
+	   for things like typedefs in the class.  We do not try to
 	   guess any imported namespace as even the fully specified
 	   namespace search is already not C++ compliant and more
 	   assumptions could make it too magic.  */
Index: testsuite/gdb.cp/derivation.cc
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/derivation.cc,v
retrieving revision 1.5
diff -u -p -r1.5 derivation.cc
--- testsuite/gdb.cp/derivation.cc	1 Jan 2013 06:33:27 -0000	1.5
+++ testsuite/gdb.cp/derivation.cc	5 Jun 2013 22:21:18 -0000
@@ -16,6 +16,8 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.
    */
 
+extern void foo2 (); /* from derivation2.cc */
+
 namespace N {
   typedef double value_type;
   struct Base { typedef int value_type; };
@@ -306,9 +308,7 @@ int main(void)
     N::Derived::value_type d = 1;
     N::value_type n = 3.0;
     dobj.doit ();
+    foo2 ();
     return 0;
     
 }
-
-    
-    
Index: testsuite/gdb.cp/derivation.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/derivation.exp,v
retrieving revision 1.24
diff -u -p -r1.24 derivation.exp
--- testsuite/gdb.cp/derivation.exp	1 Jan 2013 06:33:27 -0000	1.24
+++ testsuite/gdb.cp/derivation.exp	5 Jun 2013 22:21:18 -0000
@@ -32,14 +32,15 @@ if { [skip_cplus_tests] } { continue }
 
 load_lib "cp-support.exp"
 
-standard_testfile .cc
+standard_testfile derivation.cc derivation2.cc
 
-if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}]} {
+if {[prepare_for_testing $testfile.exp $testfile \
+	[list $srcfile $srcfile2] {debug c++}]} {
     return -1
 }
 
 # 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"
+}
+
 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 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+   */
+
+/* A copy of some classes in derivation.cc so that we can test symbol lookup
+   in other CUs.  */
+
+class A2 {
+public:
+    typedef int value_type;
+    value_type a;
+
+    A2()
+    {
+        a=1;
+    }
+};
+
+class D2 : public A2 {
+public:
+    value_type d;
+
+    D2()
+    {
+        d=7;
+    }
+};
+
+void
+foo2 ()
+{
+  D2 d2_instance;
+  d2_instance.a = 42;
+  d2_instance.d = 43;
+}