Re: RFA: patch to fix multi-breakpoint enable/disable handling of inline functions

["Douglas Evans" <dje@google.com>]

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

How about this version?

Here I use libiberty/hashtab.c to determine function name ambiguity.

It turns out the bug exists for constructors too.  I.e.
l->function_name for "Derived" in the gdb.cp/mb-ctor testcase is
"Derived" for both locations.  I wonder if for this particular
situation l->function_name should record an "enhanced" name to
distinguish them.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gdb-071126-mb-inline-3.patch --]
[-- Type: text/x-patch; name=gdb-071126-mb-inline-3.patch, Size: 10442 bytes --]

Consider:

dje@ruffy:~/gnu/sourceware/head/obj/gdb/testsuite/gdb.cp$ ~/gnu/sourceware/head/rel/bin/gdb mb-inline
GNU gdb 6.7.50-20071009-cvs
Copyright (C) 2007 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i386-linux"...
Using host libthread_db library "/lib/tls/i686/cmov/libthread_db.so.1".
(gdb) b mb-inline.h:6
Breakpoint 1 at 0x80483e9: file ../../../src/gdb/testsuite/gdb.cp/mb-inline.h, line 6. (2 locations)
(gdb) i b
Num     Type           Disp Enb  Address    What
1       breakpoint     keep y    <MULTIPLE>
1.1                         y    0x080483e9 in foo
                                      at ../../../src/gdb/testsuite/gdb.cp/mb-inline.h:6
1.2                         y    0x0804843d in foo
                                      at ../../../src/gdb/testsuite/gdb.cp/mb-inline.h:6
(gdb) dis 1.2
(gdb) i b
Num     Type           Disp Enb  Address    What
1       breakpoint     keep y    <MULTIPLE>
1.1                         y    0x080483e9 in foo
                                      at ../../../src/gdb/testsuite/gdb.cp/mb-inline.h:6
1.2                         n    0x0804843d in foo
                                      at ../../../src/gdb/testsuite/gdb.cp/mb-inline.h:6
(gdb) r
Starting program: /home/dje/gnu/sourceware/head/obj/gdb/testsuite/gdb.cp/mb-inline

Breakpoint 1, foo (i=1) at ../../../src/gdb/testsuite/gdb.cp/mb-inline.h:6
6         return i; // set breakpoint here
(gdb) i b
Num     Type           Disp Enb  Address    What
1       breakpoint     keep y    <MULTIPLE>
       breakpoint already hit 1 time
1.1                         n    0x080483e9 in foo
                                      at ../../../src/gdb/testsuite/gdb.cp/mb-inline.h:6
1.2                         y    0x0804843d in foo
                                      at ../../../src/gdb/testsuite/gdb.cp/mb-inline.h:6
(gdb)

Note that 1.1 is now disabled and 1.2 is enabled.

Here's a patch.
There is a heuristic involved in knowing when to not compare function names.
I've tried to come up with something reasonable.

2007-10-15  Doug Evans  <dje@google.com>

	* breakpoint.c: #include "hashtab.h".
	(ambiguous_names_p): New fn.
	(update_breakpoint_locations): When restoring bp enable status, don't
	compare function names if any functions have same name.

	* gdb.cp/mb-inline.exp: New.
	* gdb.cp/mb-inline.h: New.
	* gdb.cp/mb-inline1.cc: New.
	* gdb.cp/mb-inline2.cc: New.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.284
diff -u -p -u -p -r1.284 breakpoint.c
--- breakpoint.c	23 Nov 2007 16:54:34 -0000	1.284
+++ breakpoint.c	26 Nov 2007 23:10:19 -0000
@@ -21,6 +21,7 @@
 
 #include "defs.h"
 #include <ctype.h>
+#include "hashtab.h"
 #include "symtab.h"
 #include "frame.h"
 #include "breakpoint.h"
@@ -7337,6 +7338,44 @@ all_locations_are_pending (struct bp_loc
   return 1;
 }
 
+/* Subroutine of update_breakpoint_locations to simplify it.
+   Return non-zero if multiple fns in list LOC have the same name.
+   Null names are ignored.  */
+
+static int
+ambiguous_names_p (struct bp_location *loc)
+{
+  struct bp_location *l;
+  htab_t htab = htab_create_alloc (13, htab_hash_string,
+				   (int (*) (const void *, const void *)) streq,
+				   NULL, xcalloc, xfree);
+
+  for (l = loc; l != NULL; l = l->next)
+    {
+      hashval_t hashval;
+      const char **slot;
+      const char *name = l->function_name;
+
+      /* Allow for some names to be NULL, ignore them.  */
+      if (name == NULL)
+	continue;
+
+      slot = (const char **) htab_find_slot (htab, (const void *) name,
+					     INSERT);
+      /* NOTE: We can assume slot != NULL here because xcalloc never returns
+	 NULL.  */
+      if (*slot != NULL)
+	{
+	  htab_delete (htab);
+	  return 1;
+	}
+      *slot = name;
+    }
+
+  htab_delete (htab);
+  return 0;
+}
+
 static void
 update_breakpoint_locations (struct breakpoint *b,
 			     struct symtabs_and_lines sals)
@@ -7399,18 +7438,37 @@ update_breakpoint_locations (struct brea
   /* If possible, carry over 'disable' status from existing breakpoints.  */
   {
     struct bp_location *e = existing_locations;
+    /* If there are multiple breakpoints with the same function name,
+       e.g. for inline functions, comparing function names won't work.
+       Instead compare pc addresses; this is just a heuristic as things
+       may have moved, but in practice it gives the correct answer
+       often enough until a better solution is found.  */
+    int have_ambiguous_names = ambiguous_names_p (b->loc);
+
     for (; e; e = e->next)
       {
 	if (!e->enabled && e->function_name)
 	  {
 	    struct bp_location *l = b->loc;
-	    for (; l; l = l->next)
-	      if (l->function_name 
-		  && strcmp (e->function_name, l->function_name) == 0)
-		{
-		  l->enabled = 0;
-		  break;
-		}
+	    if (have_ambiguous_names)
+	      {
+		for (; l; l = l->next)
+		  if (e->address == l->address)
+		    {
+		      l->enabled = 0;
+		      break;
+		    }
+	      }
+	    else
+	      {
+		for (; l; l = l->next)
+		  if (l->function_name
+		      && strcmp (e->function_name, l->function_name) == 0)
+		    {
+		      l->enabled = 0;
+		      break;
+		    }
+	      }
 	  }
       }
   }
Index: testsuite/gdb.cp/mb-inline.exp
===================================================================
RCS file: testsuite/gdb.cp/mb-inline.exp
diff -N testsuite/gdb.cp/mb-inline.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/mb-inline.exp	26 Nov 2007 23:10:19 -0000
@@ -0,0 +1,108 @@
+# Copyright 2007 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/>.
+
+# This file is part of the gdb testsuite.
+
+# This test verifies that setting breakpoint on line in inline
+# function will fire in all instantiations of that function.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+if { [skip_cplus_tests] } { continue }
+
+set prms_id 0
+set bug_id 0
+
+set testfile "mb-inline"
+set hdrfile "${testfile}.h"
+set srcfile1 "${testfile}1.cc"
+set objfile1 "${testfile}1.o"
+set srcfile2 "${testfile}2.cc"
+set objfile2 "${testfile}2.o"
+set binfile  "${objdir}/${subdir}/${testfile}"
+
+if  { [gdb_compile "$srcdir/$subdir/$srcfile1" "$objdir/$subdir/$objfile1" object {debug c++}] != "" } {
+     untested rtti.exp
+     return -1
+}
+
+if  { [gdb_compile "$srcdir/$subdir/$srcfile2" "$objdir/$subdir/$objfile2" object {debug c++}] != "" } {
+     untested rtti.exp
+     return -1
+}
+
+if  { [gdb_compile "$objdir/$subdir/$objfile1 $objdir/$subdir/$objfile2" "${binfile}" executable {debug c++}] != "" } {
+     untested rtti.exp
+     return -1
+}
+
+if [get_compiler_info ${binfile} "c++"] {
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+set bp_location [gdb_get_line_number "set breakpoint here" $hdrfile]
+
+# Set a breakpoint with multiple locations.
+
+gdb_test "break $hdrfile:$bp_location" \
+    "Breakpoint.*at.* file .*$hdrfile, line.*\\(2 locations\\).*" \
+    "set breakpoint"
+
+gdb_run_cmd
+gdb_expect {
+    -re "Breakpoint \[0-9\]+,.*foo \\(i=0\\).*$gdb_prompt $" {
+	pass "run to breakpoint"
+    }
+    -re "$gdb_prompt $" {
+	fail "run to breakpoint"
+    }
+    timeout {
+	fail "run to breakpoint (timeout)"
+    }
+}
+
+gdb_test "continue" \
+    ".*Breakpoint.*foo \\(i=1\\).*" \
+    "run to breakpoint 2"
+
+# Try disabling a single location. We also test
+# that at least in simple cases, the enable/disable
+# state of locations survive "run".
+# Early bug would disable 1.1 and enable 1.2 when program is run.
+gdb_test "disable 1.2" "" "disabling location: disable"
+
+gdb_run_cmd
+gdb_expect {
+    -re "Breakpoint \[0-9\]+,.*foo \\(i=0\\).*$gdb_prompt $" {
+	pass "disabling location: run to breakpoint"
+    }
+    -re "$gdb_prompt $" {
+	fail "disabling location: run to breakpoint"
+    }
+    timeout {
+	fail "disabling location: run to breakpoint (timeout)"
+    }
+}
+
+gdb_test "continue" \
+    ".*Program exited normally.*" \
+    "continue with disabled breakpoint 1.2"
Index: testsuite/gdb.cp/mb-inline.h
===================================================================
RCS file: testsuite/gdb.cp/mb-inline.h
diff -N testsuite/gdb.cp/mb-inline.h
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/mb-inline.h	26 Nov 2007 23:10:19 -0000
@@ -0,0 +1,12 @@
+// Test gdb support for setting multiple file:line breakpoints on static
+// functions.  In practice the functions may be inline fns compiled with -O0.
+// We avoid using inline here for simplicity's sake.
+
+static int
+foo (int i)
+{
+  return i; // set breakpoint here
+}
+
+extern int afn ();
+extern int bfn ();
Index: testsuite/gdb.cp/mb-inline1.cc
===================================================================
RCS file: testsuite/gdb.cp/mb-inline1.cc
diff -N testsuite/gdb.cp/mb-inline1.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/mb-inline1.cc	26 Nov 2007 23:10:19 -0000
@@ -0,0 +1,17 @@
+// Test gdb support for setting file:line breakpoints on inline fns.
+
+#include "mb-inline.h"
+
+int
+afn ()
+{
+  return foo (0);
+}
+
+int
+main ()
+{
+  int a = afn ();
+  int b = bfn ();
+  return a * b;
+}
Index: testsuite/gdb.cp/mb-inline2.cc
===================================================================
RCS file: testsuite/gdb.cp/mb-inline2.cc
diff -N testsuite/gdb.cp/mb-inline2.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/mb-inline2.cc	26 Nov 2007 23:10:19 -0000
@@ -0,0 +1,7 @@
+#include "mb-inline.h"
+
+int
+bfn ()
+{
+  return foo (1);
+}