gdb: avoid breakpoint::clear_locations calls in update_breakpoint_locations

The commit:

  commit 6cce025114ccd0f53cc552fde12b6329596c6c65
  Date:   Fri Mar 3 19:03:15 2023 +0000

      gdb: only insert thread-specific breakpoints in the relevant inferior

added a couple of calls to breakpoint::clear_locations() inside
update_breakpoint_locations().

The intention of these calls was to avoid leaving redundant locations
around when a thread- or inferior-specific breakpoint was switched
from one thread or inferior to another.

Without the clear_locations() calls the tests gdb.multi/tids.exp and
gdb.multi/pending-bp.exp have some failures.  A b/p is changed such
that the program space it is associated with changes.  This triggers a
call to breakpoint_re_set_one() but the FILTER_PSPACE argument will be
the new program space.  As a result GDB correctly calculates the new
locations and adds these to the breakpoint, but the old locations, in
the old program space, are incorrectly retained.  The call to
clear_locations() solves this by deleting the old locations.

However, while working on another patch I realised that the approach
taken here is not correct.  The FILTER_PSPACE argument passed to
breakpoint_re_set_one() and then on to update_breakpoint_locations()
might not be the program space to which the breakpoint is associated.
Consider this example:

  (gdb) file /tmp/hello.x
  Reading symbols from /tmp/hello.x...
  (gdb) start
  Temporary breakpoint 1 at 0x401198: file hello.c, line 18.
  Starting program: /tmp/hello.x

  Temporary breakpoint 1, main () at hello.c:18
  18	  printf ("Hello World\n");
  (gdb) break main thread 1
  Breakpoint 2 at 0x401198: file hello.c, line 18.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  2       breakpoint     keep y   0x0000000000401198 in main at hello.c:18
  	stop only in thread 1
  (gdb) add-inferior -exec /tmp/hello.x
  [New inferior 2]
  Added inferior 2 on connection 1 (native)
  Reading symbols from /tmp/hello.x...
  (gdb) info breakpoints
  Num     Type           Disp Enb Address    What
  2       breakpoint     keep y   <PENDING>  main
  	stop only in thread 1.1

Notice that after creating the second inferior and loading a file the
thread-specific breakpoint was incorrectly made pending.  Loading the
exec file in the second inferior triggered a call to
breakpoint_re_set() with the new, second, program space as the
current_program_space.

This program space ends up being passed to
update_breakpoint_locations().

In update_breakpoint_locations this condition is true:

  if (all_locations_are_pending (b, filter_pspace) && sals.empty ())

and so we end up discarding all of the locations for this breakpoint,
making the breakpoint pending.

What we really want to do in update_breakpoint_locations() is, for
thread- or inferior- specific breakpoints, delete any locations which
are associated with a program space that this breakpoint is NOT
associated with.

But then I realised the answer was easier than that.

The ONLY time that a b/p can have locations associated with the
"wrong" program space like this is at the moment we change the thread
or inferior the b/p is associated with by calling
breakpoint_set_thread() or breakpoint_set_inferior().

And so, I think the correct solution is to hoist the call to
clear_locations() out of update_breakpoint_locations() and place a
call in each of the breakpoint_set_{thread,inferior} functions.

I've done this, and added a couple of new tests.  All of which are
now passing.

Approved-By: Tom Tromey <tom@tromey.com>

4 files changed, 93 insertions, 2 deletions

diff --git a/gdb/testsuite/gdb.multi/bp-thread-specific.exp b/gdb/testsuite/gdb.multi/bp-thread-specific.exp
index c1d8752..11dc248 100644
--- a/gdb/testsuite/gdb.multi/bp-thread-specific.exp
+++ b/gdb/testsuite/gdb.multi/bp-thread-specific.exp
@@ -32,9 +32,33 @@ if {![runto_main]} {
     return -1
 }
 
+delete_breakpoints
+
+# Create a thread-specific b/p on main.
+gdb_breakpoint "main thread 1"
+set bpnum [get_integer_valueof "\$bpnum" "INVALID" \
+	      "get number for thread specific b/p on main"]
+
+# Check the b/p has a location and is displayed correctly.
+gdb_test "info breakpoints" \
+    [multi_line \
+	 "" \
+	 "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+$hex\\s+in main at \[^\r\n\]+/$srcfile:$decimal"\
+	 "\\s+stop only in thread 1"] \
+    "check thread b/p on main has a location"
+
 gdb_test "add-inferior -exec ${binfile}" "Added inferior 2.*" "add inferior 2"
 gdb_test "inferior 2"
 
+# The breakpoint should still have a location, but should now display
+# information indicating this breakpoint is only in inferior 1.
+gdb_test "info breakpoints" \
+    [multi_line \
+	 "" \
+	 "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+$hex\\s+in main at \[^\r\n\]+/$srcfile:$decimal inf 1"\
+	 "\\s+stop only in thread 1\\.1"] \
+    "check thread b/p on main still has updated correctly"
+
 if {![runto_main]} {
     return -1
 }
diff --git a/gdb/testsuite/gdb.multi/inferior-specific-bp-1.c b/gdb/testsuite/gdb.multi/inferior-specific-bp-1.c
index 59a6e32..16db062 100644
--- a/gdb/testsuite/gdb.multi/inferior-specific-bp-1.c
+++ b/gdb/testsuite/gdb.multi/inferior-specific-bp-1.c
@@ -35,7 +35,7 @@ foo (void)
 static void
 bar (void)
 {
-  global_var = 0;
+  global_var = 0;	/* First location of bar.  */
 
   foo ();
 }
diff --git a/gdb/testsuite/gdb.multi/inferior-specific-bp-2.c b/gdb/testsuite/gdb.multi/inferior-specific-bp-2.c
index cbae745..bde6fbf 100644
--- a/gdb/testsuite/gdb.multi/inferior-specific-bp-2.c
+++ b/gdb/testsuite/gdb.multi/inferior-specific-bp-2.c
@@ -36,7 +36,7 @@ main (void)
 static int
 bar (void)
 {
-  return baz ();
+  return baz ();		/* Second location of bar.  */
 }
 
 static int
diff --git a/gdb/testsuite/gdb.multi/inferior-specific-bp.exp b/gdb/testsuite/gdb.multi/inferior-specific-bp.exp
index 52f8418..82cc924 100644
--- a/gdb/testsuite/gdb.multi/inferior-specific-bp.exp
+++ b/gdb/testsuite/gdb.multi/inferior-specific-bp.exp
@@ -62,6 +62,73 @@ gdb_test "break foo inferior 1 inferior 2" \
 # Clear out any other breakpoints.
 delete_breakpoints
 
+# Create an inferior specific breakpoint and then change the inferior
+# using the Python API.  Use 'info breakpoint' to check that the
+# breakpoint was updated as we expect.
+if { [allow_python_tests] } {
+    with_test_prefix "update breakpoint inferior" {
+	# Create the b/p and grab its number.
+	gdb_breakpoint "bar inferior 1"
+	set bpnum [get_integer_valueof "\$bpnum" "INVALID" \
+		       "get b/p number for breakpoint on bar"]
+
+	# Get the line number for the two locations, the first in
+	# inferior 1, the second in inferior 2.
+	set bar_lineno_1 \
+	    [gdb_get_line_number "First location of bar" $srcfile]
+	set bar_lineno_2 \
+	    [gdb_get_line_number "Second location of bar" $srcfile2]
+
+	# Check the b/p was created with a single location where we
+	# expect it.
+	gdb_test "info breakpoint $bpnum" \
+	    [multi_line \
+		 "" \
+		 "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+$hex\\s+in bar at \[^\r\n\]+/$srcfile:$bar_lineno_1 inf 1" \
+		 "\\s+stop only in inferior 1"] \
+	    "check original details for breakpoint on bar"
+
+	# Use the Python API to update the b/p's inferior.
+	gdb_test_no_output "python bp = gdb.breakpoints()\[0\]"
+	gdb_test_no_output "python bp.inferior = 2"
+
+	# We should still only have a single location, but now in
+	# inferior 2.
+	gdb_test "info breakpoint $bpnum" \
+	    [multi_line \
+		 "" \
+		 "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+$hex\\s+in bar at \[^\r\n\]+/$srcfile2:$bar_lineno_2 inf 2" \
+		 "\\s+stop only in inferior 2"] \
+	    "check updated details for breakpoint on bar"
+
+	# Use the Python API to remove the inferior restriction on the
+	# breakpoint.
+	gdb_test_no_output "python bp.inferior = None"
+
+	# The breakpoint should now have multiple locations.
+	gdb_test "info breakpoint $bpnum" \
+	    [multi_line \
+		 "" \
+		 "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+<MULTIPLE>\\s*" \
+		 "$bpnum.1\\s+y\\s+$hex\\s+in bar at\[^\r\n\]+$srcfile:$bar_lineno_1 inf 1" \
+		 "$bpnum.2\\s+y\\s+$hex\\s+in bar at\[^\r\n\]+$srcfile2:$bar_lineno_2 inf 2"] \
+	    "check breakpoint bar now inferior requirement is gone"
+
+	# Finally, add the inferior requirement back.
+	gdb_test_no_output "python bp.inferior = 1"
+
+	# Check the original location and restriction is restored.
+	gdb_test "info breakpoint $bpnum" \
+	    [multi_line \
+		 "" \
+		 "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+$hex\\s+in bar at \[^\r\n\]+/$srcfile:$bar_lineno_1 inf 1" \
+		 "\\s+stop only in inferior 1"] \
+	    "check original details for breakpoint on bar are back"
+
+	delete_breakpoints
+    }
+}
+
 # Use 'info breakpoint' to check that the inferior specific breakpoint is
 # present in the breakpoint list.  TESTNAME is the name used for this test,
 # BP_NUMBER is the number for the breakpoint, and EXPECTED_LOC_COUNT is the