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

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

[Doug Evans]

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 (ambiguous_names_p): New fn.
	(update_breakpoint_locations): When restoring bp enable status, don't
	compare function names if all functions have same name.

	* gdb.cp/mb-ctor.exp: Check skip_cplus_tests.
	* gdb.cp/mb-templates.exp: Check skip_cplus_tests.
	* 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.274
diff -u -u -p -r1.274 breakpoint.c
--- breakpoint.c	12 Oct 2007 16:11:11 -0000	1.274
+++ breakpoint.c	15 Oct 2007 23:47:54 -0000
@@ -7496,6 +7496,37 @@ all_locations_are_pending (struct bp_loc
   return 1;
 }
 
+/* Return non-zero if multiple fns in list LOC have the same name.
+   Null names are ignored.
+   This returns zero if there's <= one named function, there's no ambiguity.
+   ??? This tests for ambiguity with the first named function, it doesn't
+   handle the case of multiple ambiguities.  This can be fixed at the cost of
+   some complexity in the caller, but it's unknown if this is a practical
+   issue.  */
+
+static int
+ambiguous_names_p (struct bp_location *loc)
+{
+  struct bp_location *l;
+  const char *name = NULL;
+
+  for (l = loc; l != NULL; l = l->next)
+    {
+      /* Allow for some names to be NULL, ignore them.  */
+      if (l->function_name == NULL)
+	continue;
+      if (name == NULL)
+	{
+	  name = l->function_name;
+	  continue;
+	}
+      if (strcmp (name, l->function_name) == 0)
+	return 1;
+    }
+
+  return 0;
+}
+
 static void
 update_breakpoint_locations (struct breakpoint *b,
 			     struct symtabs_and_lines sals)
@@ -7558,18 +7589,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-ctor.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/mb-ctor.exp,v
retrieving revision 1.1
diff -u -u -p -r1.1 mb-ctor.exp
--- testsuite/gdb.cp/mb-ctor.exp	24 Sep 2007 07:40:32 -0000	1.1
+++ testsuite/gdb.cp/mb-ctor.exp	15 Oct 2007 23:47:55 -0000
@@ -21,6 +21,8 @@ if $tracelevel then {
     strace $tracelevel
 }
 
+if { [skip_cplus_tests] } { continue }
+
 set prms_id 0
 set bug_id 0
 
Index: testsuite/gdb.cp/mb-templates.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/mb-templates.exp,v
retrieving revision 1.1
diff -u -u -p -r1.1 mb-templates.exp
--- testsuite/gdb.cp/mb-templates.exp	24 Sep 2007 07:40:32 -0000	1.1
+++ testsuite/gdb.cp/mb-templates.exp	15 Oct 2007 23:47:55 -0000
@@ -21,6 +21,8 @@ if $tracelevel then {
     strace $tracelevel
 }
 
+if { [skip_cplus_tests] } { continue }
+
 set prms_id 0
 set bug_id 0
 
--- testsuite/gdb.cp/mb-inline.exp=	1969-12-31 16:00:00.000000000 -0800
+++ testsuite/gdb.cp/mb-inline.exp	2007-10-15 16:44:36.858766000 -0700
@@ -0,0 +1,105 @@
+# 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 template
+# function will fire in all instantiations of that template.
+
+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".
+# Testing 1.2 here is important - early bug would disable 1.1 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)"
+    }
+}
--- testsuite/gdb.cp/mb-inline.h=	1969-12-31 16:00:00.000000000 -0800
+++ testsuite/gdb.cp/mb-inline.h	2007-10-15 16:40:42.908114000 -0700
@@ -0,0 +1,10 @@
+// Test gdb support for setting file:line breakpoints on inline fns.
+
+static inline int
+foo (int i)
+{
+  return i; // set breakpoint here
+}
+
+extern int afn ();
+extern int bfn ();
--- testsuite/gdb.cp/mb-inline1.cc=	1969-12-31 16:00:00.000000000 -0800
+++ testsuite/gdb.cp/mb-inline1.cc	2007-10-15 16:42:02.644024000 -0700
@@ -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;
+}
--- testsuite/gdb.cp/mb-inline2.cc=	1969-12-31 16:00:00.000000000 -0800
+++ testsuite/gdb.cp/mb-inline2.cc	2007-10-15 16:42:06.315615000 -0700
@@ -0,0 +1,7 @@
+#include "mb-inline.h"
+
+int
+bfn ()
+{
+  return foo (1);
+}

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

[Douglas Evans]

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

Ping.

I've split the patch into two parts, separating out the tweaks to
mb-templates.exp,mb-ctor.exp (I'll send this in a separate message).
I also fixed a few typos.  The revised patch is attached.

Ok to check in?

On 10/15/07, Doug Evans <dje@google.com> wrote:
> 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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gdb-071022-mb-inline-2a.patch --]
[-- Type: text/x-patch; name=gdb-071022-mb-inline-2a.patch, Size: 10156 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 (ambiguous_names_p): New fn.
	(update_breakpoint_locations): When restoring bp enable status, don't
	compare function names if all 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.274
diff -u -u -p -r1.274 breakpoint.c
--- ./breakpoint.c	12 Oct 2007 16:11:11 -0000	1.274
+++ ./breakpoint.c	15 Oct 2007 23:47:54 -0000
@@ -7496,6 +7496,37 @@ all_locations_are_pending (struct bp_loc
   return 1;
 }
 
+/* Return non-zero if multiple fns in list LOC have the same name.
+   Null names are ignored.
+   This returns zero if there's <= one named function, there's no ambiguity.
+   ??? This tests for ambiguity with the first named function, it doesn't
+   handle the case of multiple ambiguities.  This can be fixed at the cost of
+   some complexity in the caller, but it's unknown if this is a practical
+   issue.  */
+
+static int
+ambiguous_names_p (struct bp_location *loc)
+{
+  struct bp_location *l;
+  const char *name = NULL;
+
+  for (l = loc; l != NULL; l = l->next)
+    {
+      /* Allow for some names to be NULL, ignore them.  */
+      if (l->function_name == NULL)
+	continue;
+      if (name == NULL)
+	{
+	  name = l->function_name;
+	  continue;
+	}
+      if (strcmp (name, l->function_name) == 0)
+	return 1;
+    }
+
+  return 0;
+}
+
 static void
 update_breakpoint_locations (struct breakpoint *b,
 			     struct symtabs_and_lines sals)
@@ -7558,18 +7589,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: /cvs/src/src/gdb/testsuite/gdb.cp/mb-inline.exp,v
diff -u -p -N mb-inline.exp
--- .//dev/null	1969-12-31 16:00:00.000000000 -0800
+++ ./testsuite/gdb.cp/mb-inline.exp	2007-10-15 16:44:36.858766000 -0700
@@ -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: /cvs/src/src/gdb/testsuite/gdb.cp/mb-inline.h,v
diff -u -p -N mb-inline.h
--- .//dev/null	1969-12-31 16:00:00.000000000 -0800
+++ ./testsuite/gdb.cp/mb-inline.h	2007-10-15 16:40:42.908114000 -0700
@@ -0,0 +1,10 @@
+// Test gdb support for setting file:line breakpoints on inline fns.
+
+static inline int
+foo (int i)
+{
+  return i; // set breakpoint here
+}
+
+extern int afn ();
+extern int bfn ();
Index: testsuite/gdb.cp/mb-inline1.cc
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/mb-inline1.cc,v
diff -u -p -N mb-inline1.cc
--- .//dev/null	1969-12-31 16:00:00.000000000 -0800
+++ ./testsuite/gdb.cp/mb-inline1.cc	2007-10-15 16:42:02.644024000 -0700
@@ -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: /cvs/src/src/gdb/testsuite/gdb.cp/mb-inline2.cc,v
diff -u -p -N mb-inline2.cc
--- .//dev/null	1969-12-31 16:00:00.000000000 -0800
+++ ./testsuite/gdb.cp/mb-inline2.cc	2007-10-15 16:42:06.315615000 -0700
@@ -0,0 +1,7 @@
+#include "mb-inline.h"
+
+int
+bfn ()
+{
+  return foo (1);
+}

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

[Daniel Jacobowitz]

On Mon, Oct 22, 2007 at 02:22:31PM -0700, Douglas Evans wrote:
> Ping.
> 
> I've split the patch into two parts, separating out the tweaks to
> mb-templates.exp,mb-ctor.exp (I'll send this in a separate message).
> I also fixed a few typos.  The revised patch is attached.
> 
> Ok to check in?

I've been hoping Vladimir would look at this - Vladimir?

If he doesn't have time I'll try to figure it out.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Douglas Evans]

On Oct 22, 2007 2:25 PM, Daniel Jacobowitz <drow@false.org> wrote:
> On Mon, Oct 22, 2007 at 02:22:31PM -0700, Douglas Evans wrote:
> > Ping.
> >
> > I've split the patch into two parts, separating out the tweaks to
> > mb-templates.exp,mb-ctor.exp (I'll send this in a separate message).
> > I also fixed a few typos.  The revised patch is attached.
> >
> > Ok to check in?
>
> I've been hoping Vladimir would look at this - Vladimir?
>
> If he doesn't have time I'll try to figure it out.

The bug is that transferring enable/disable status from the old
breakpoint list to the new one (which is done each time the program is
run) gets the wrong results for static inline functions with
multi-breakpoints.  In my testcase 1.1 is enabled and 1.2 is disabled,
but when the program is run 1.1 becomes disabled and 1.2 becomes
enabled.  The code uses strcmp to distinguish the functions but
clearly that can't work in this case.  The patch adds code to
recognize this case of ambiguous function names and when detected uses
the pc address instead.  Using the pc address is less preferable to
strcmp because if the user changes the program, functions can move,
but in this case strcmp doesn't work at all.  OTOH the patch is simple
and gets the right answer most of the time.  More complex solutions
are possible - I'd still add this patch until then, but that's just my
$0.02.

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

[Vladimir Prus]

Douglas Evans wrote:

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

Thanks for the patch! I generally find it to be good incremental
improvement.

> 2007-10-15  Doug Evans  <dje@google.com>
> 
> * breakpoint.c (ambiguous_names_p): New fn.
> (update_breakpoint_locations): When restoring bp enable status, don't
> compare function names if all functions have same name.

IIUC, your patch avoids comparing function names if it finds
two locations having the same name, not necessary that *all* locations
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.274
> diff -u -u -p -r1.274 breakpoint.c
> --- ./breakpoint.c      12 Oct 2007 16:11:11 -0000      1.274
> +++ ./breakpoint.c      15 Oct 2007 23:47:54 -0000
> @@ -7496,6 +7496,37 @@ all_locations_are_pending (struct bp_loc
> return 1;
> }
> 
> +/* Return non-zero if multiple fns in list LOC have the same name.
> +   Null names are ignored.
> +   This returns zero if there's <= one named function, there's no
> ambiguity. +   ??? This tests for ambiguity with the first named function,
> it doesn't +   handle the case of multiple ambiguities.  This can be fixed
> at the cost of +   some complexity in the caller, but it's unknown if this
> is a practical +   issue.  */

I find the comment a bit confusing. How about this:

/* Return non-zero if any two locations in LOC have the 
   function_name field non-NULL and equal. Non-zero return means
   we cannot use function names to uniquely identify locations
   in this list.
   Although it's possible to identify groups of locations with
   the same name, this functions does not try to do that, as the
   code for matching old and new locations does not have use for
   such elaborate functionality.  */

> +
> +static int
> +ambiguous_names_p (struct bp_location *loc)
> +{
> +  struct bp_location *l;
> +  const char *name = NULL;
> +
> +  for (l = loc; l != NULL; l = l->next)
> +    {
> +      /* Allow for some names to be NULL, ignore them.  */
> +      if (l->function_name == NULL)
> +       continue;
> +      if (name == NULL)
> +       {
> +         name = l->function_name;
> +         continue;
> +       }
> +      if (strcmp (name, l->function_name) == 0)
> +       return 1;
> +    }

Assume we have location with function names like "a" "b", "b".
IIUC, this code will compare "a" with "b", and then "a" with "b"
again, and return 0. It will never compare "b" with "b". Am I wrong?
If I'm right we probably should have double loop.

> +
> +  return 0;
> +}
> +
> static void
> update_breakpoint_locations (struct breakpoint *b,
> struct symtabs_and_lines sals)
> @@ -7558,18 +7589,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)
.......
> +           if (have_ambiguous_names)
> +             {

Do we still have to check for e->function name if have_ambiguous_names
is true?


> Index: testsuite/gdb.cp/mb-inline.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/mb-inline.h,v
> diff -u -p -N mb-inline.h
> --- .//dev/null 1969-12-31 16:00:00.000000000 -0800
> +++ ./testsuite/gdb.cp/mb-inline.h      2007-10-15 16:40:42.908114000
> -0700 @@ -0,0 +1,10 @@
> +// Test gdb support for setting file:line breakpoints on inline fns.
> +
> +static inline int
> +foo (int i)

As I've asked on IRC -- is it important that this function is "static"?
Will the test be valid if "static" is removed. If so, can you add a 
comment explaining why?

Thanks,
Volodya

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

[Douglas Evans]

On Nov 13, 2007 1:10 PM, Vladimir Prus <ghost@cs.msu.su> wrote:
> > 2007-10-15 Doug Evans <dje@google.com>
> >
> > * breakpoint.c (ambiguous_names_p): New fn.
> > (update_breakpoint_locations): When restoring bp enable status, don't
> > compare function names if all functions have same name.
>
> IIUC, your patch avoids comparing function names if it finds
> two locations having the same name, not necessary that *all* locations
> have same name.

Correct.  I can fix the wording.

> > +/* Return non-zero if multiple fns in list LOC have the same name.
> > + Null names are ignored.
> > + This returns zero if there's <= one named function, there's no
> > ambiguity. + ??? This tests for ambiguity with the first named function,
> > it doesn't + handle the case of multiple ambiguities. This can be fixed
> > at the cost of + some complexity in the caller, but it's unknown if this
> > is a practical + issue. */
>
> I find the comment a bit confusing. How about this:
>
> /* Return non-zero if any two locations in LOC have the
>    function_name field non-NULL and equal. Non-zero return means
>    we cannot use function names to uniquely identify locations
>    in this list.
>    Although it's possible to identify groups of locations with
>    the same name, this functions does not try to do that, as the
>    code for matching old and new locations does not have use for
>    such elaborate functionality.  */

Well ...
"Non-zero return means we cannot use ..." is an attribute of the
caller.  We can certainly put such a comment at the call site.
There's a comment to that effect at the call site already though.

How about simply

+/* Return non-zero if multiple fns in list LOC have the same name.
+   Null names are ignored.  */

> Assume we have location with function names like "a" "b", "b".
> IIUC, this code will compare "a" with "b", and then "a" with "b"
> again, and return 0. It will never compare "b" with "b". Am I wrong?
> If I'm right we probably should have double loop.

Like the original comment to the function says, it wasn't written to
handle multiple different names.  An alternative to two loops is to
store all the names in a set and then see if the set has more than one
member.

> Do we still have to check for e->function name if have_ambiguous_names
> is true?

I wrote it that way to preserve the basic intent of the loop.
I can rewrite it as one pleases however.

> As I've asked on IRC -- is it important that this function is "static"?
> Will the test be valid if "static" is removed. If so, can you add a
> comment explaining why?

"static inline" is a pretty common gcc idiom.
bash$ find /usr/include -type f  | xargs grep "static inline"

Is this what you're looking for?

+/* This is "static inline" to cause the function to be put
+   in each file that it is compiled in.  */
+static inline int
+foo (int i)
+{
+  return i; // set breakpoint here
+}

Maybe I should remove the "inline" so the test is immune to -O.

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

[Douglas Evans]

On Nov 14, 2007 2:25 PM, Douglas Evans <dje@google.com> wrote:
> Like the original comment to the function says, it wasn't written to
> handle multiple different names.  An alternative to two loops is to
> store all the names in a set and then see if the set has more than one
> member.

Blech.  Bad-Wording-R-Us (TM).  What I really meant of course was to
see if any member had more than one occurrence.  E.g. While adding
names checking if it's already there.

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

[Douglas Evans]

[-- 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);
+}

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

[Doug Evans]

On Nov 26, 2007 3:22 PM, Douglas Evans <dje@google.com> wrote:
> 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.

Ping.

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

[Vladimir Prus]

On Tuesday 27 November 2007 02:22:40 Douglas Evans wrote:
> 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.

Hi Doug,
sorry for slow reply. I think this version of patch is fine, but
I don't have the right to approve it.

Speaking about the constructors issue you bring -- right, for
reasons unknown the function name for constructor does not
include parameter types (either in plain text, or mangled).

Storing mangled name would require two bits:

1. Learning how to get than mangled name.
2. Changing expand_line_sal_maybe to cope with this.
Right now, if you set breakpoint at function name, we first
expand locations, and then check that all newly found locations
belong to a function of the same name. So, if you set a breakpoint
on a specific instantiation of template function that is inline,
you'd have breakpoint on all inlined instances of that instantiations,
but not on other instantiations.

Now if we use mangled name of constructor, then setting breakpoint on
constructor by name won't set breakpoint on the other constructor.
It might require some tweaks to get right

- Volodya

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

[Doug Evans]

Ping ...

On Dec 27, 2007 10:36 AM, Vladimir Prus <vladimir@codesourcery.com> wrote:
>
> On Tuesday 27 November 2007 02:22:40 Douglas Evans wrote:
> > 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.
>
> Hi Doug,
> sorry for slow reply. I think this version of patch is fine, but
> I don't have the right to approve it.
>
> Speaking about the constructors issue you bring -- right, for
> reasons unknown the function name for constructor does not
> include parameter types (either in plain text, or mangled).
>
> Storing mangled name would require two bits:
>
> 1. Learning how to get than mangled name.
> 2. Changing expand_line_sal_maybe to cope with this.
> Right now, if you set breakpoint at function name, we first
> expand locations, and then check that all newly found locations
> belong to a function of the same name. So, if you set a breakpoint
> on a specific instantiation of template function that is inline,
> you'd have breakpoint on all inlined instances of that instantiations,
> but not on other instantiations.
>
> Now if we use mangled name of constructor, then setting breakpoint on
> constructor by name won't set breakpoint on the other constructor.
> It might require some tweaks to get right
>
> - Volodya
>

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

[Doug Evans]

If this is still in someone's queue, let me know.  Vladimir is ok with
this but he can't approve it.  If folks are holding off approving it
because there's something they don't like but don't have time to
research it further, maybe you could tell me what it is and we can
both research it.


On Jan 28, 2008 10:14 AM, Doug Evans <dje@google.com> wrote:
> Ping ...
>
>
> On Dec 27, 2007 10:36 AM, Vladimir Prus <vladimir@codesourcery.com> wrote:
> >
> > On Tuesday 27 November 2007 02:22:40 Douglas Evans wrote:
> > > 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.
> >
> > Hi Doug,
> > sorry for slow reply. I think this version of patch is fine, but
> > I don't have the right to approve it.
> >
> > Speaking about the constructors issue you bring -- right, for
> > reasons unknown the function name for constructor does not
> > include parameter types (either in plain text, or mangled).
> >
> > Storing mangled name would require two bits:
> >
> > 1. Learning how to get than mangled name.
> > 2. Changing expand_line_sal_maybe to cope with this.
> > Right now, if you set breakpoint at function name, we first
> > expand locations, and then check that all newly found locations
> > belong to a function of the same name. So, if you set a breakpoint
> > on a specific instantiation of template function that is inline,
> > you'd have breakpoint on all inlined instances of that instantiations,
> > but not on other instantiations.
> >
> > Now if we use mangled name of constructor, then setting breakpoint on
> > constructor by name won't set breakpoint on the other constructor.
> > It might require some tweaks to get right
> >
> > - Volodya
> >
>

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

[Daniel Jacobowitz]

On Wed, Feb 06, 2008 at 11:08:57AM -0800, Doug Evans wrote:
> If this is still in someone's queue, let me know.

Sorry.  It's still in my queue but not for any good reason; when I
blazed through patches last week, this was left in the little bit I
ran out of time for.  I'll do it now.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Daniel Jacobowitz]

On Mon, Nov 26, 2007 at 03:22:40PM -0800, Douglas Evans wrote:
> How about this version?

This is OK.  Minor corrections: update copyright years because I took
so long, add copyright notices to source files in the testsuite,
update Makefile.in with $(hashtab_h).

-- 
Daniel Jacobowitz
CodeSourcery