[RFA] c++/11734

[RFA] c++/11734

[Keith Seitz]

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

Hi,

c++/11734 involves being unable to set a breakpoint at a method defined 
in a different CU from the class declaration. This regression occurs 
because the dwarf2_physname patch removes the use of 
DW_AT_MIPS_linkage_name.

[You might ask why this is. I refer you to c++/11734 for a more detailed 
explanation of how gdb "appeared" to work prior to the dwarf2_physname 
patch.]

The attached patch fixes this regression. While testing, I also noticed 
a related issue with overloaded methods defined in multiple CUs, and 
I've fixed that, too.

Questions/comments/concerns?

Keith

ChangeLog
2010-06-21  Keith Seitz  <keiths@redhat.com>

	c++/11734
	* linespec.c (decode_compound): If saved_arg is surrounded
	in single quotes, strip them.
	* psymtab.c (lookup_partial_symbol): If the search name
	contains overload information, expand any psymtabs that
	match the un-overoaded name.

testsuite/ChangeLog
2010-06-21  Keith Seitz  <keiths@redhat.com>

	c++/11734
	* gdb.cp/pr11734.exp: New test.
	* gdb.cp/pr11734-1.cc, gdb.cp/pr11734-2.cc, gdb.cp/pr11734-3.cc,
	  gdb.cp/pr11734-4.cc, gdb.cp/pr11734.h: New files.

[-- Attachment #2: pr11734.patch --]
[-- Type: text/plain, Size: 10927 bytes --]

Index: linespec.c
===================================================================
RCS file: /cvs/src/src/gdb/linespec.c,v
retrieving revision 1.103
diff -u -p -r1.103 linespec.c
--- linespec.c	14 May 2010 23:41:04 -0000	1.103
+++ linespec.c	22 Jun 2010 01:19:12 -0000
@@ -1217,6 +1217,19 @@ decode_compound (char **argptr, int funf
   struct type *t;
   char *saved_java_argptr = NULL;
 
+  /* Strip single quotes from SAVED_ARG.  This interferes with this function
+     which might, e.g., later call strcmp_iw with SYMBOL_LINKAGE_NAME
+     (which never contains quotes).  */
+  if (*saved_arg == '\'')
+    {
+      char *close = strrchr (saved_arg, '\'');
+      if (close)
+	{
+	  ++saved_arg;
+	  *close = '\0';
+	}
+    }
+
   /* First check for "global" namespace specification, of the form
      "::foo".  If found, skip over the colons and jump to normal
      symbol processing.  I.e. the whole line specification starts with
Index: psymtab.c
===================================================================
RCS file: /cvs/src/src/gdb/psymtab.c,v
retrieving revision 1.4
diff -u -p -r1.4 psymtab.c
--- psymtab.c	16 May 2010 01:27:02 -0000	1.4
+++ psymtab.c	22 Jun 2010 01:19:12 -0000
@@ -476,12 +476,39 @@ lookup_partial_symbol (struct partial_sy
       if (!(top == bottom))
 	internal_error (__FILE__, __LINE__, _("failed internal consistency check"));
 
-      while (top <= real_top
-	     && SYMBOL_MATCHES_SEARCH_NAME (*top, name))
+      while (top <= real_top)
 	{
-	  if (symbol_matches_domain (SYMBOL_LANGUAGE (*top),
-				     SYMBOL_DOMAIN (*top), domain))
-	    return (*top);
+	  if (SYMBOL_MATCHES_SEARCH_NAME (*top, name))
+	    {
+	      if (symbol_matches_domain (SYMBOL_LANGUAGE (*top),
+					 SYMBOL_DOMAIN (*top), domain))
+		return (*top);
+	    }
+	  else
+	    {
+	      char *paren = strchr (name, '(');
+
+	      if (paren)
+		{
+		  char *new;
+
+		  /* NAME has overload information.  Partial symbols, however,
+		     do not.  This is a case of mistaken identity.
+
+		     Although hacky, this is fixed by expanding this psymtab,
+		     which will allow any subsequent symtab search to succeed.
+
+		     For more details/test case, please refer to c++/11734.  */
+
+		  new = alloca (strlen (name));
+		  memcpy (new, name, paren - name);
+		  new[name - paren] = '\0';
+		  if (SYMBOL_MATCHES_SEARCH_NAME (*top, new))
+		    PSYMTAB_TO_SYMTAB (pst);
+		}
+	      else
+		break;
+	    }
 	  top++;
 	}
     }
Index: testsuite/gdb.cp/pr11734-1.cc
===================================================================
RCS file: testsuite/gdb.cp/pr11734-1.cc
diff -N testsuite/gdb.cp/pr11734-1.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr11734-1.cc	22 Jun 2010 01:19:14 -0000
@@ -0,0 +1,30 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@gnu.org  */
+
+#include "pr11734.h"
+
+int
+main ()
+{
+  pr11734 *p = new pr11734;
+  p->foo ();
+  return 0;
+}
+
Index: testsuite/gdb.cp/pr11734-2.cc
===================================================================
RCS file: testsuite/gdb.cp/pr11734-2.cc
diff -N testsuite/gdb.cp/pr11734-2.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr11734-2.cc	22 Jun 2010 01:19:14 -0000
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@gnu.org  */
+
+#include "pr11734.h"
+
+void
+pr11734::foo(void)
+{
+}
+
Index: testsuite/gdb.cp/pr11734-3.cc
===================================================================
RCS file: testsuite/gdb.cp/pr11734-3.cc
diff -N testsuite/gdb.cp/pr11734-3.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr11734-3.cc	22 Jun 2010 01:19:14 -0000
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@gnu.org  */
+
+#include "pr11734.h"
+
+void
+pr11734::foo (int a)
+{
+}
+
Index: testsuite/gdb.cp/pr11734-4.cc
===================================================================
RCS file: testsuite/gdb.cp/pr11734-4.cc
diff -N testsuite/gdb.cp/pr11734-4.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr11734-4.cc	22 Jun 2010 01:19:14 -0000
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@gnu.org  */
+
+#include "pr11734.h"
+
+void
+pr11734::foo (char *a)
+{
+}
+
Index: testsuite/gdb.cp/pr11734.exp
===================================================================
RCS file: testsuite/gdb.cp/pr11734.exp
diff -N testsuite/gdb.cp/pr11734.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr11734.exp	22 Jun 2010 01:19:14 -0000
@@ -0,0 +1,67 @@
+# Copyright 2010 Free Software Foundation, Inc.
+#
+# Contributed by Red Hat, originally written by Keith Seitz.
+#
+# 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/>.
+
+# This file is part of the gdb testsuite.
+
+if { [skip_cplus_tests] } { continue }
+
+set testfile "pr11734"
+set class $testfile
+set binfile [file join $objdir $subdir $testfile]
+
+set srcfiles {}
+for {set i 1} {$i < 5} {incr i} {
+    lappend srcfiles [file join $srcdir $subdir $testfile-$i.cc]
+}
+if  {[gdb_compile $srcfiles $binfile executable {debug c++}] != "" } {
+     untested pr11734.exp
+     return -1
+}
+
+if {[get_compiler_info $binfile "c++"]} {
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load $binfile
+
+if {![runto_main]} {
+    perror "couldn't run to breakpoint"
+    continue
+}
+
+# An array holding the overload types for the method pr11734::foo.  The
+# first element is the overloaded method parameter.  The second element
+# is the expected source file number, e.g. "pr11734-?.cc".
+array set tests {
+    "char*"  4
+    "int"    3
+    ""       2
+}
+
+# Test each overload instance twice: once quoted, once unquoted
+foreach ovld [array names tests] {
+    set method "${class}::foo\($ovld\)"
+    set result "Breakpoint (\[0-9\]).*file .*/$class-$tests($ovld).*"
+    gdb_test "break $method" $result
+    gdb_test "break '$method'" $result
+}
+
+gdb_exit
+return 0
Index: testsuite/gdb.cp/pr11734.h
===================================================================
RCS file: testsuite/gdb.cp/pr11734.h
diff -N testsuite/gdb.cp/pr11734.h
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr11734.h	22 Jun 2010 01:19:14 -0000
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@gnu.org  */
+
+class pr11734
+{
+ public:
+  void foo ();
+  void foo (int);
+  void foo (char *);
+};
+

Re: [RFA] c++/11734

[Pedro Alves]

No comments on the idea of the patch itself, just
a quick comment on something that made me stop for a second
while skimming your patch:

On Tuesday 22 June 2010 02:22:03, Keith Seitz wrote:
> +                 new = alloca (strlen (name));

Note that alloca in a loop in general isn't a good idea.  The
stack memory added in each iteration is only released on function
exit, not on each iteration, so you're prone to stack overflow
given enough loops.  It looks like `name' is invariant in
this loop, so you could for example, move the `new' declaration
out of the loop, initialized as NULL, and allocate it only once
on first need.

> +                 memcpy (new, name, paren - name);
> +                 new[name - paren] = '\0';

-- 
Pedro Alves

Re: [RFA] c++/11734

[Keith Seitz]

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

On 06/22/2010 02:45 AM, Pedro Alves wrote:
> It looks like `name' is invariant in
> this loop, so you could for example, move the `new' declaration
> out of the loop, initialized as NULL, and allocate it only once
> on first need.

I completely missed that. [Forest/trees kind of thing, I guess.]

I noticed that I also missed adding this logic to the linear search 
case. I've attached a revised patch which addresses this missed bit and 
moves the alloca out of the loop.

Good catch!

Keith

PS. In case it isn't well-known: regression-free on x86_64 linux.

[-- Attachment #2: pr11734-2.patch --]
[-- Type: text/plain, Size: 11880 bytes --]

Index: linespec.c
===================================================================
RCS file: /cvs/src/src/gdb/linespec.c,v
retrieving revision 1.103
diff -u -p -r1.103 linespec.c
--- linespec.c	14 May 2010 23:41:04 -0000	1.103
+++ linespec.c	22 Jun 2010 15:39:18 -0000
@@ -1217,6 +1217,19 @@ decode_compound (char **argptr, int funf
   struct type *t;
   char *saved_java_argptr = NULL;
 
+  /* Strip single quotes from SAVED_ARG.  This interferes with this function
+     which might, e.g., later call strcmp_iw with SYMBOL_LINKAGE_NAME
+     (which never contains quotes).  */
+  if (*saved_arg == '\'')
+    {
+      char *close = strrchr (saved_arg, '\'');
+      if (close)
+	{
+	  ++saved_arg;
+	  *close = '\0';
+	}
+    }
+
   /* First check for "global" namespace specification, of the form
      "::foo".  If found, skip over the colons and jump to normal
      symbol processing.  I.e. the whole line specification starts with
Index: psymtab.c
===================================================================
RCS file: /cvs/src/src/gdb/psymtab.c,v
retrieving revision 1.4
diff -u -p -r1.4 psymtab.c
--- psymtab.c	16 May 2010 01:27:02 -0000	1.4
+++ psymtab.c	22 Jun 2010 15:39:20 -0000
@@ -432,6 +432,7 @@ lookup_partial_symbol (struct partial_sy
   struct partial_symbol **top, **real_top, **bottom, **center;
   int length = (global ? pst->n_global_syms : pst->n_static_syms);
   int do_linear_search = 1;
+  char *simple_name = NULL, *paren;
 
   if (length == 0)
     {
@@ -441,6 +442,14 @@ lookup_partial_symbol (struct partial_sy
 	   pst->objfile->global_psymbols.list + pst->globals_offset :
 	   pst->objfile->static_psymbols.list + pst->statics_offset);
 
+  paren = strchr (name, '(');
+  if (paren)
+    {
+      simple_name = alloca (strlen (name));
+      memcpy (simple_name, name, paren - name);
+      simple_name[name - paren] = '\0';
+    }
+
   if (global)			/* This means we can use a binary search. */
     {
       do_linear_search = 0;
@@ -476,12 +485,32 @@ lookup_partial_symbol (struct partial_sy
       if (!(top == bottom))
 	internal_error (__FILE__, __LINE__, _("failed internal consistency check"));
 
-      while (top <= real_top
-	     && SYMBOL_MATCHES_SEARCH_NAME (*top, name))
+      while (top <= real_top)
 	{
-	  if (symbol_matches_domain (SYMBOL_LANGUAGE (*top),
-				     SYMBOL_DOMAIN (*top), domain))
-	    return (*top);
+	  if (SYMBOL_MATCHES_SEARCH_NAME (*top, name))
+	    {
+	      if (symbol_matches_domain (SYMBOL_LANGUAGE (*top),
+					 SYMBOL_DOMAIN (*top), domain))
+		return (*top);
+	    }
+	  else
+	    {
+	      if (simple_name)
+		{
+		  /* NAME has overload information.  Partial symbols, however,
+		     do not.  This is a case of mistaken identity.
+
+		     Although hacky, this is fixed by expanding this psymtab,
+		     which will allow any subsequent symtab search to succeed.
+
+		     For more details/test case, please refer to c++/11734.  */
+
+		  if (SYMBOL_MATCHES_SEARCH_NAME (*top, simple_name))
+		    PSYMTAB_TO_SYMTAB (pst);
+		}
+	      else
+		break;
+	    }
 	  top++;
 	}
     }
@@ -497,6 +526,11 @@ lookup_partial_symbol (struct partial_sy
 				     SYMBOL_DOMAIN (*psym), domain)
 	      && SYMBOL_MATCHES_SEARCH_NAME (*psym, name))
 	    return (*psym);
+	  else if (simple_name && SYMBOL_MATCHES_SEARCH_NAME (*psym, simple_name))
+	    {
+	      PSYMTAB_TO_SYMTAB (pst);
+	      break;
+	    }
 	}
     }
 
Index: testsuite/gdb.cp/pr11734-1.cc
===================================================================
RCS file: testsuite/gdb.cp/pr11734-1.cc
diff -N testsuite/gdb.cp/pr11734-1.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr11734-1.cc	22 Jun 2010 15:39:25 -0000
@@ -0,0 +1,30 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@gnu.org  */
+
+#include "pr11734.h"
+
+int
+main ()
+{
+  pr11734 *p = new pr11734;
+  p->foo ();
+  return 0;
+}
+
Index: testsuite/gdb.cp/pr11734-2.cc
===================================================================
RCS file: testsuite/gdb.cp/pr11734-2.cc
diff -N testsuite/gdb.cp/pr11734-2.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr11734-2.cc	22 Jun 2010 15:39:25 -0000
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@gnu.org  */
+
+#include "pr11734.h"
+
+void
+pr11734::foo(void)
+{
+}
+
Index: testsuite/gdb.cp/pr11734-3.cc
===================================================================
RCS file: testsuite/gdb.cp/pr11734-3.cc
diff -N testsuite/gdb.cp/pr11734-3.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr11734-3.cc	22 Jun 2010 15:39:25 -0000
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@gnu.org  */
+
+#include "pr11734.h"
+
+void
+pr11734::foo (int a)
+{
+}
+
Index: testsuite/gdb.cp/pr11734-4.cc
===================================================================
RCS file: testsuite/gdb.cp/pr11734-4.cc
diff -N testsuite/gdb.cp/pr11734-4.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr11734-4.cc	22 Jun 2010 15:39:25 -0000
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@gnu.org  */
+
+#include "pr11734.h"
+
+void
+pr11734::foo (char *a)
+{
+}
+
Index: testsuite/gdb.cp/pr11734.exp
===================================================================
RCS file: testsuite/gdb.cp/pr11734.exp
diff -N testsuite/gdb.cp/pr11734.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr11734.exp	22 Jun 2010 15:39:25 -0000
@@ -0,0 +1,67 @@
+# Copyright 2010 Free Software Foundation, Inc.
+#
+# Contributed by Red Hat, originally written by Keith Seitz.
+#
+# 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/>.
+
+# This file is part of the gdb testsuite.
+
+if { [skip_cplus_tests] } { continue }
+
+set testfile "pr11734"
+set class $testfile
+set binfile [file join $objdir $subdir $testfile]
+
+set srcfiles {}
+for {set i 1} {$i < 5} {incr i} {
+    lappend srcfiles [file join $srcdir $subdir $testfile-$i.cc]
+}
+if  {[gdb_compile $srcfiles $binfile executable {debug c++}] != "" } {
+     untested pr11734.exp
+     return -1
+}
+
+if {[get_compiler_info $binfile "c++"]} {
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load $binfile
+
+if {![runto_main]} {
+    perror "couldn't run to breakpoint"
+    continue
+}
+
+# An array holding the overload types for the method pr11734::foo.  The
+# first element is the overloaded method parameter.  The second element
+# is the expected source file number, e.g. "pr11734-?.cc".
+array set tests {
+    "char*"  4
+    "int"    3
+    ""       2
+}
+
+# Test each overload instance twice: once quoted, once unquoted
+foreach ovld [array names tests] {
+    set method "${class}::foo\($ovld\)"
+    set result "Breakpoint (\[0-9\]).*file .*/$class-$tests($ovld).*"
+    gdb_test "break $method" $result
+    gdb_test "break '$method'" $result
+}
+
+gdb_exit
+return 0
Index: testsuite/gdb.cp/pr11734.h
===================================================================
RCS file: testsuite/gdb.cp/pr11734.h
diff -N testsuite/gdb.cp/pr11734.h
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr11734.h	22 Jun 2010 15:39:25 -0000
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@gnu.org  */
+
+class pr11734
+{
+ public:
+  void foo ();
+  void foo (int);
+  void foo (char *);
+};
+

Re: [RFA] c++/11734

[Keith Seitz]

On 06/22/2010 08:43 AM, Keith Seitz wrote:
> I noticed that I also missed adding this logic to the linear search
> case. I've attached a revised patch which addresses this missed bit and
> moves the alloca out of the loop.

BTW, after reviewing my own patch again, I think it should not break out 
of either loop prematurely: an exact match might later appear in the list.

While this doesn't really affect the ultimate outcome (because we know 
that symtabs will be searched next), prematurely breaking could cause 
lookup_partial_symbol to return NULL when an exact match is really found.

Keith

Re: [RFA] c++/11734

[Doug Evans]

On Tue, Jun 22, 2010 at 8:43 AM, Keith Seitz <keiths@redhat.com> wrote:
> I noticed that I also missed adding this logic to the linear search case.
> I've attached a revised patch which addresses this missed bit and moves the
> alloca out of the loop.

I looked at this a bit.

One thing I noticed is that "break c::foo" is only working by accident.
"break c::foo" needs full symbols because we look up each overloaded
method, so we need a lookup of "c::foo()" to succeed.
"c::foo" looks enough like an objc symbol that decode_objc will call
lookup_symbol ("c::foo") which will expand the psymtab and hence
"c::foo()" is found.
With "break c::foo()", "c::foo()" doesn't look enough like an objc
symbol so decode_objc ignores it - so we don't get full symtabs and
thus "c::foo()" isn't found.
[I'm not suggesting we fix this by making decode_objc recognize
"c::foo()" as a potentially valid objc symbol though. :-)]

I have a few comments on your patch.

diff -u -p -r1.103 linespec.c
--- linespec.c	14 May 2010 23:41:04 -0000	1.103
+++ linespec.c	22 Jun 2010 15:39:18 -0000
@@ -1217,6 +1217,19 @@ decode_compound (char **argptr, int funf
   struct type *t;
   char *saved_java_argptr = NULL;

+  /* Strip single quotes from SAVED_ARG.  This interferes with this function
+     which might, e.g., later call strcmp_iw with SYMBOL_LINKAGE_NAME
+     (which never contains quotes).  */
+  if (*saved_arg == '\'')
+    {
+      char *close = strrchr (saved_arg, '\'');
+      if (close)
+	{
+	  ++saved_arg;
+	  *close = '\0';
+	}
+    }
+
   /* First check for "global" namespace specification, of the form
      "::foo".  If found, skip over the colons and jump to normal
      symbol processing.  I.e. the whole line specification starts with

I think the change to linespec.c should be a separate patch.
And I think decode_compound shouldn't be modifying what saved_arg
points to.  Plus the caller is expecting the trailing quote to be
there when decode_compound returns.  At least that's what it seems.
[Time goes by ...]  Looking deeper I see that for break 'c::foo()',
is_quote_enclosed is zero. That's because locate_first_half only looks
for double-quotes, but it will remove them! (which is what you're
doing in decode_compound).  So it seems like a better way to go is to
teach locate_first_half about all quote characters.  There may be more
going on here though, I don't understand why decode_line_1 has *both*
is_quoted and is_quote_enclosed (is there some cleanup that can be
done here, or is there a technical reason to handle " different from
'?).

For the lookup_partial_symbol patch:

diff -u -p -r1.4 psymtab.c
--- psymtab.c	16 May 2010 01:27:02 -0000	1.4
+++ psymtab.c	22 Jun 2010 15:39:20 -0000
@@ -432,6 +432,7 @@ lookup_partial_symbol (struct partial_sy
   struct partial_symbol **top, **real_top, **bottom, **center;
   int length = (global ? pst->n_global_syms : pst->n_static_syms);
   int do_linear_search = 1;
+  char *simple_name = NULL, *paren;

   if (length == 0)
     {
@@ -441,6 +442,14 @@ lookup_partial_symbol (struct partial_sy
 	   pst->objfile->global_psymbols.list + pst->globals_offset :
 	   pst->objfile->static_psymbols.list + pst->statics_offset);

+  paren = strchr (name, '(');
+  if (paren)
+    {
+      simple_name = alloca (strlen (name));
+      memcpy (simple_name, name, paren - name);
+      simple_name[name - paren] = '\0';
+    }
+
   if (global)			/* This means we can use a binary search. */
     {
       do_linear_search = 0;
@@ -476,12 +485,32 @@ lookup_partial_symbol (struct partial_sy
       if (!(top == bottom))
 	internal_error (__FILE__, __LINE__, _("failed internal consistency check"));

-      while (top <= real_top
-	     && SYMBOL_MATCHES_SEARCH_NAME (*top, name))
+      while (top <= real_top)
 	{
-	  if (symbol_matches_domain (SYMBOL_LANGUAGE (*top),
-				     SYMBOL_DOMAIN (*top), domain))
-	    return (*top);
+	  if (SYMBOL_MATCHES_SEARCH_NAME (*top, name))
+	    {
+	      if (symbol_matches_domain (SYMBOL_LANGUAGE (*top),
+					 SYMBOL_DOMAIN (*top), domain))
+		return (*top);
+	    }
+	  else
+	    {
+	      if (simple_name)
+		{
+		  /* NAME has overload information.  Partial symbols, however,
+		     do not.  This is a case of mistaken identity.
+
+		     Although hacky, this is fixed by expanding this psymtab,
+		     which will allow any subsequent symtab search to succeed.
+
+		     For more details/test case, please refer to c++/11734.  */
+
+		  if (SYMBOL_MATCHES_SEARCH_NAME (*top, simple_name))
+		    PSYMTAB_TO_SYMTAB (pst);
+		}
+	      else
+		break;
+	    }
 	  top++;
 	}
     }
@@ -497,6 +526,11 @@ lookup_partial_symbol (struct partial_sy
 				     SYMBOL_DOMAIN (*psym), domain)
 	      && SYMBOL_MATCHES_SEARCH_NAME (*psym, name))
 	    return (*psym);
+	  else if (simple_name && SYMBOL_MATCHES_SEARCH_NAME (*psym, simple_name))
+	    {
+	      PSYMTAB_TO_SYMTAB (pst);
+	      break;
+	    }
 	}
     }

There are several callers and rather than changing all of them to
strip method overloading of the name to search for, it seems
reasonable to handle it in lookup_partial_symbol.
[But there's more to the story here - expanding the psymtab here feels
wrong, see below.]

In the case of simple_name != NULL, do we need to call
SYMBOL_MATCHES_SEARCH_NAME twice?
IWBN if psymtabs didn't require that complexity and *only* recorded
the un-overloaded name.

Although, note that if a psymtab did record an overloaded name,
because of the order of arguments to strcmp_iw, we have to worry about
matching c::foo(int) in the psymtab with c::foo(char*) in the search
string.  strcmp_iw ("c::foo(int)", "c::foo") is a match.

Could we set name to simple_name when simple_name is created, and then
have only one call to SYMBOL_MATCHES_SEARCH_NAME?
There's still the issue of handling the "found" case differently for
non-overloaded names vs overloaded-names.
AIUI, what you're doing here is having the lookup "fail" if an
overloaded-name is found, so that a subsequent search in the full
symtab will be done and, having expanded the psymtab here, that search
will succeed.  However psymtabs are searched *after* symtabs.  This
patch works because it turns out that we will later do a search in the
full symtab, but that's more by accident than design:

struct symbol *
cp_lookup_symbol_nonlocal (const char *name,
                           const struct block *block,
                           const domain_enum domain)
{
  struct symbol *sym;
  const char *scope = block_scope (block);

  sym = lookup_namespace_scope (name, block, domain, scope, 0);  //<<<
psymtab expanded here
  if (sym != NULL)
    return sym;

  return cp_lookup_symbol_namespace (scope, name, block, domain);
//<<< overloaded symbol found here
}

[Unless I'm missing something of course.  That's what I see as I
single step through gdb watching what's happening.]

This situation is not well solved by our normal psymtabs->symtabs lazy
expansion design.
I don't have a specific proposal for a better fix at the moment.
Comments?

Re: [RFA] c++/11734

[Matt Rice]

On Wed, Jun 23, 2010 at 7:25 PM, Doug Evans <dje@google.com> wrote:

> "c::foo" looks enough like an objc symbol that decode_objc will call
> lookup_symbol ("c::foo")

its possibly worth noting that c::foo is not really a valid
objective-c selector/method name

a method which would contain unnamed message pieces (not really sure
the correct wording)
which would lead to '::' also must end in a colon.

so the following method is valid:

- (int) c:(int)a :(int)b foo:(int)c;
{
  return a + b + c;
}

which would equal 'c::foo:'

while 'foo' is a valid for a name for a method which has no parameters
and does not end in a colon.
you can't mix and match parameter taking message pieces and
non-paramater accepting ones in the same message,
they either do or they don't accept parameters.

and making it foo:(void) isn't valid either.

Re: [RFA] c++/11734

[Doug Evans]

On Wed, Jun 23, 2010 at 7:25 PM, Doug Evans <dje@google.com> wrote:
> From Keith's patch:
> +             if (simple_name)
> +               {
> +                 /* NAME has overload information.  Partial symbols, however,
> +                    do not.  This is a case of mistaken identity.
> +
> +                    Although hacky, this is fixed by expanding this psymtab,
> +                    which will allow any subsequent symtab search to succeed.
> +
> +                    For more details/test case, please refer to c++/11734.  */
> +
> +                 if (SYMBOL_MATCHES_SEARCH_NAME (*top, simple_name))
> +                   PSYMTAB_TO_SYMTAB (pst);
> +               }
> [...]
> I don't have a specific proposal for a better fix at the moment.

Maybe after expanding the psymtab you could search the full symtab here?
Still hacky, but maybe good enough for 7.2?

Re: [RFA] c++/11734

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> There may be more going on here though, I don't understand why
Doug> decode_line_1 has *both* is_quoted and is_quote_enclosed (is there
Doug> some cleanup that can be done here, or is there a technical reason
Doug> to handle " different from '?).

Based on my last foray into decode_line_1, I would say that it is likely
that nobody knows the answer to this question.

That code is seriously twisted and horrible.  E.g., see PR 8883, or
11289, or 11614.  I think we should plan to rewrite it -- Keith was
talking about this a while ago... maybe he got scared off ;-)

In the short run I am inclined to approve anything that fixes problems
and doesn't regress.

Doug> There are several callers and rather than changing all of them to
Doug> strip method overloading of the name to search for, it seems
Doug> reasonable to handle it in lookup_partial_symbol.

One thing I would like to know is the exact semantics required of these
lookup functions.  That is, if the implementation of one or more quick
functions is expected to strip the "(" stuff, then that ought to be
documented in symfile.h.

Doug> IWBN if psymtabs didn't require that complexity and *only* recorded
Doug> the un-overloaded name.

And then...?  I suppose we could expand all psymtabs with a matching
name, then let the symtab code be intelligent about picking out matches.

It is pretty easy to add a pre-expansion call like this.  This is what
the index branch does -- it records names only, not symbol types, so
before a name lookup is done it reads all the CUs that define the name,
regardless of how it is defined there.

FWIW, Sami is working on an approach like this for template functions.
I think it is a promising approach, though I'm concerned about how we
will handle different instantiations of the same unadorned name
appearing in multiple objfiles.

Doug> AIUI, what you're doing here is having the lookup "fail" if an
Doug> overloaded-name is found, so that a subsequent search in the full
Doug> symtab will be done and, having expanded the psymtab here, that search
Doug> will succeed.  However psymtabs are searched *after* symtabs.  This
Doug> patch works because it turns out that we will later do a search in the
Doug> full symtab, but that's more by accident than design:

Do you know what exact code path is taken here?

I think what is intended to happen is that a call to a quick function
may result in symtab expansion.  Then the caller is supposed to look in
this symtab for the answer.  E.g., lookup_symbol_aux_quick does this.

Doug> This situation is not well solved by our normal psymtabs->symtabs lazy
Doug> expansion design.
Doug> I don't have a specific proposal for a better fix at the moment.
Doug> Comments?

We should probably also plan on a symtab overhaul.

Right now gdb's behavior is dependent on the order of psymtab expansion.
This can result in weird results for users, even for pretty ordinary
code.  I don't think this is very good.  Pre-expansion would fix this,
though maybe at a cost; and maybe that alone would introduce new
problems as well.

Also I think we have some bugs that we could fix if symtab lookups
returned all the symbols for a given name -- not just the first one
matching.  Perhaps we would even need to extend this to work across
objfiles; I imagine we have plenty of lurking bugs in this scenario.

We may also want to consider the multi-inferior objfile reuse problem
when doing this.  Perhaps a new symtab API could return relocated
symbols or something along those lines.

I'm very interested to learn about other cases where we have trouble.

Tom

Re: [RFA] c++/11734

[Doug Evans]

On Thu, Jun 24, 2010 at 1:00 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>
> Doug> There may be more going on here though, I don't understand why
> Doug> decode_line_1 has *both* is_quoted and is_quote_enclosed (is there
> Doug> some cleanup that can be done here, or is there a technical reason
> Doug> to handle " different from '?).
>
> Based on my last foray into decode_line_1, I would say that it is likely
> that nobody knows the answer to this question.
>
> That code is seriously twisted and horrible.  E.g., see PR 8883, or
> 11289, or 11614.  I think we should plan to rewrite it -- Keith was
> talking about this a while ago... maybe he got scared off ;-)
>
> In the short run I am inclined to approve anything that fixes problems
> and doesn't regress.

IWBN to keep the code at least as coherent as it is now.
Having decode_compound strip quotes doesn't do this for me.
[This feels easy to fix, so I don't see this as asking too much, fwiw,]

> Doug> There are several callers and rather than changing all of them to
> Doug> strip method overloading of the name to search for, it seems
> Doug> reasonable to handle it in lookup_partial_symbol.
>
> One thing I would like to know is the exact semantics required of these
> lookup functions.  That is, if the implementation of one or more quick
> functions is expected to strip the "(" stuff, then that ought to be
> documented in symfile.h.

I think that, for this particular case, it comes down to having a
definition of what *is* recorded in a psymtab for overloaded methods
(and documenting it of course).

> Doug> IWBN if psymtabs didn't require that complexity and *only* recorded
> Doug> the un-overloaded name.
>
> And then...?  I suppose we could expand all psymtabs with a matching
> name, then let the symtab code be intelligent about picking out matches.

My premise for saying that is that psymtabs, at least for the case at
hand, *do* store the un-overloaded name.
My point was IWBN to not have them sometimes store un-overloaded names
and sometimes store overloaded names (i.e., don't be inconsistent).
Of course, if they always *only* stored overloaded names then that's
another way, but they don't do that today (and I *think* that's
intentional).

> It is pretty easy to add a pre-expansion call like this.  This is what
> the index branch does -- it records names only, not symbol types, so
> before a name lookup is done it reads all the CUs that define the name,
> regardless of how it is defined there.
>
> FWIW, Sami is working on an approach like this for template functions.
> I think it is a promising approach, though I'm concerned about how we
> will handle different instantiations of the same unadorned name
> appearing in multiple objfiles.
>
> Doug> AIUI, what you're doing here is having the lookup "fail" if an
> Doug> overloaded-name is found, so that a subsequent search in the full
> Doug> symtab will be done and, having expanded the psymtab here, that search
> Doug> will succeed.  However psymtabs are searched *after* symtabs.  This
> Doug> patch works because it turns out that we will later do a search in the
> Doug> full symtab, but that's more by accident than design:
>
> Do you know what exact code path is taken here?

Essentially we're in linespec.c:find_methods, given "foo()" (from
"break c::foo()") and are looking up all the overloaded methods.

Digging deeper, the caller of lookup_partial_symbol for the case at
hand is lookup_symbol_aux_psymtabs, and it does call PSYMTAB_TO_SYMTAB
(as do other callers, except one, but it doesn't need to).  So we
don't have to call PSYMTAB_TO_SYMTAB in lookup_partial_symbol and only
need to return the match.
And it's caller is lookup_symbol_aux_quick ... but we've returned NULL
so it early exits.
Changing lookup_partial_symbol to simply strip the overloading info
before doing the comparison sounds good.

diff -u -p -r1.5 psymtab.c
--- psymtab.c   24 Jun 2010 20:17:52 -0000      1.5
+++ psymtab.c   24 Jun 2010 20:45:03 -0000
@@ -432,6 +432,7 @@ lookup_partial_symbol (struct partial_sy
   struct partial_symbol **top, **real_top, **bottom, **center;
   int length = (global ? pst->n_global_syms : pst->n_static_syms);
   int do_linear_search = 1;
+  char *simple_name = NULL, *paren;

   if (length == 0)
     {
@@ -441,6 +442,16 @@ lookup_partial_symbol (struct partial_sy
           pst->objfile->global_psymbols.list + pst->globals_offset :
           pst->objfile->static_psymbols.list + pst->statics_offset);

+  /* FIXME: What about "(anonymous namespace)".  */
+  paren = strchr (name, '(');
+  if (paren)
+    {
+      simple_name = alloca (strlen (name));
+      memcpy (simple_name, name, paren - name);
+      simple_name[name - paren] = '\0';
+      name = simple_name;
+    }
+
   if (global)                  /* This means we can use a binary search. */
     {
       do_linear_search = 0;

I added the FIXME just as a reminder, I wasn't suggesting checking
this in of course.

> I think what is intended to happen is that a call to a quick function
> may result in symtab expansion.  Then the caller is supposed to look in
> this symtab for the answer.  E.g., lookup_symbol_aux_quick does this.

I see that now.

> Doug> This situation is not well solved by our normal psymtabs->symtabs lazy
> Doug> expansion design.
> Doug> I don't have a specific proposal for a better fix at the moment.
> Doug> Comments?
>
> We should probably also plan on a symtab overhaul.

No disagreement here.

> Right now gdb's behavior is dependent on the order of psymtab expansion.
> This can result in weird results for users, even for pretty ordinary
> code.  I don't think this is very good.  Pre-expansion would fix this,
> though maybe at a cost; and maybe that alone would introduce new
> problems as well.
>
> Also I think we have some bugs that we could fix if symtab lookups
> returned all the symbols for a given name -- not just the first one
> matching.  Perhaps we would even need to extend this to work across
> objfiles; I imagine we have plenty of lurking bugs in this scenario.
>
> We may also want to consider the multi-inferior objfile reuse problem
> when doing this.  Perhaps a new symtab API could return relocated
> symbols or something along those lines.
>
> I'm very interested to learn about other cases where we have trouble.
>
> Tom
>

Re: [RFA] c++/11734

[Keith Seitz]

On 06/23/2010 07:25 PM, Doug Evans wrote:
> I think the change to linespec.c should be a separate patch.

I shall try to isolate the linespec.c change and a test.

> And I think decode_compound shouldn't be modifying what saved_arg
> points to.

Ah, that's right. Gdb passes these things around like they were going 
out of style... That is easy enough to avoid, though.

I guess this raises the follow-on question, does something like 
"'c::foo()' const" work? I suspect not (with or without my patch). I 
will write some new tests for this and see what I can do.

> So it seems like a better way to go is to
> teach locate_first_half about all quote characters.

I'll investigate that avenue, which is, I gather, to treat all single 
and double quotes "equivalently." [i.e, probably not exactly 
identically: "XXX" == 'XXX'; "XXX' and 'XXX" illegal; not even going to 
try "X'Y'Z" -- I'd be better off rewriting linespec.c than tackle some 
of these problems!]

> There may be more
> going on here though, I don't understand why decode_line_1 has *both*
> is_quoted and is_quote_enclosed (is there some cleanup that can be
> done here, or is there a technical reason to handle " different from
> '?).

At one time, I convinced myself that I understood the difference between 
is_quoted and is_quote_enclosed... I'll have to spend some time to 
remind/convince myself again.

> For the lookup_partial_symbol patch:
 >
> In the case of simple_name != NULL, do we need to call
> SYMBOL_MATCHES_SEARCH_NAME twice?
> IWBN if psymtabs didn't require that complexity and *only* recorded
> the un-overloaded name.

If I understand your argument correctly, yes, it seems that if we know 
that SYMBOL_MATCHES_SEARCH_NAME uses strcmp_iw, then we do not need to 
call it twice, since strcmp_iw ("foo", "foo()") will not match. But this 
seems just as hacky as relying on a subsequent symtab search to find the 
right symbol. What happens if SYMBOL_MATCHES_SEARCH_NAME is changed to 
use something other than strcmp_iw. [I know that is highly improbable in 
the near future.]

> Could we set name to simple_name when simple_name is created, and then
> have only one call to SYMBOL_MATCHES_SEARCH_NAME?

I've reversed the logic in this (check simple name first), 
short-circuiting the second call (again, since we know that it can never 
match because SYMBOL_MATCHES_SEARCH_NAME boils down to strcmp_iw, for 
which the second argument cannot contain parenthesis unless the first 
argument does). This currently shows no regressions, either.

> However psymtabs are searched *after* symtabs.

That's right. psymtabs are searched later:

> This patch works because it turns out that we will later do a search in the
> full symtab, but that's more by accident than design:

Yup. That was what I meant by "hacky."

> This situation is not well solved by our normal psymtabs->symtabs lazy
> expansion design.
> I don't have a specific proposal for a better fix at the moment.

I don't either, but I'm going to try to address your concerns and your 
other suggestion to immediately search for the symbol in the symtab.

Thank you for reviewing this.
Keith