[PATCHv2] gdb: do better at retaining shlib_disabled breakpoint locations

[Andrew Burgess <aburgess@redhat.com>]

In v2:

  - Rebased onto current HEAD.

  - Retested.

---

When a breakpoint is placed into a shared library, and the shared
library is subsequently unloaded, the breakpoint locations within the
shared library are marked as shlib_disabled, but are otherwise
retained within the breakpoint's location list.

These shlib_disabled breakpoints are then displayed as <PENDING> in
the 'info breakpoints' output.

In update_breakpoint_locations we even have some code which attempts
to retain these shlib_disabled locations, with these lines:

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

This means that if all locations for a breakpoint are shlib_disabled,
and we're not trying to add additional locations for this breakpoint,
then just leave the breakpoint locations alone (i.e. retain the
shlib_disabled locations).

In another patch I'm working on (not posted yet) I fixed an unrelated
breakpoint bug by adding an extra call to breakpoint_re_set.  This
ultimately ends up calling update_breakpoint_locations.  Unfortunately
this caused a test regression in gdb.trace/change-loc.exp.  The
problem is that a <PENDING> breakpoint was now being removed from the
breakpoints location list.

After diagnosing the issue I realised that the problem is nothing
specific to tracing (which is what the above test is about), but is
instead a general problem with the way that shlib_disabled breakpoints
are preserved.

The lines I quoted above only preserve shlib_disabled locations if we
are not adding new locations.  If we are adding new locations then the
shlib_disabled locations are removed from the breakpoint and will not
be recreated -- remember, shlib_disabled locations are, by their
definition, locations which no longer exist in the inferior (due to
shlib unloading), and so we'll only be recreating those locations if
we happen to be reloading the same shlib.

If instead we are loading a different shlib, or doing any action which
is going to cause the location(s) of a breakpoint to change, then any
shlib_disabled locations will be discarded.  This doesn't seem like a
great user experience to me, shlib_disabled breakpoints are retained
until some arbitrary point in the future when they might suddenly be
removed from the location list.

I think we should commit one way or the other, either just give up on
shlib_disabled locations, and discard them immediately (i.e. drop the
whole idea of shlib_disabled and just remove the locations), or, work
harder to retain the shlib_disabled locations.

Dropping shlib_disabled will have significant impact, every time an
inferior is restarted the user would loose any breakpoint placed
within a shared library ... this feels like a bad thing.  So I think
the right choice is to work harder at preserving shlib_disabled
locations.

And so, in this patch, I've added a new block to the end of
update_breakpoint_locations.  This new block scans the
existing_locations list (that is, the previous breakpoint's
locations), and considers adding back every shlib_disabled location
that the breakpoint used to have.

However, it's not as simple as always adding back the shlib_disabled
locations.  If the update_breakpoint_locations call was triggered
because the inferior did reload the shlib, then we will have created a
location which corresponds to the previously shlib_disabled location.
In this case the shlib_disabled location should not be added back.

And so the actual algorithm is, loop over the existing_locations list,
for every shlib_disabled location, scan the breakpoint's current
locations.  If there is no current location at the same address as the
previously shlib_disabled location then add the shlib_disabled
location back, otherwise, discard the shlib_disabled location.

There's a bit of a problem that we can't just move the shlib_disabled
location from the existing_locations list as we need to preserve the
existing_locations list in order to compare it with the breakpoint's
new locations in these lines:

  if (!locations_are_equal (existing_locations, b->locations ()))
    notify_breakpoint_modified (b);

As a bp_location can only be in one list at a time, either the
existing_locations list or the breakpoint's location list, then what I
ended up doing is creating a copy of the shlib_disabled location,
adding this copy to the existing_locations list, and moving the
original bp_location back to the breakpoint's location list.  The copy
which is placed in the existing_locations list is "just enough" to
fool the locations_are_equal function into thinking the two locations
are identical.

There's a new test which exercises this change.  I also found the
tests gdb.base/shlib-call.exp, gdb.base/unload.exp, and
gdb.trace/change-loc.exp useful for testing this patch.
---
 gdb/breakpoint.c                              |  90 ++++++++
 .../gdb.base/retain-disabled-bp-loc-lib1.c    |  26 +++
 .../gdb.base/retain-disabled-bp-loc-lib2.c    |  26 +++
 .../gdb.base/retain-disabled-bp-loc.c         |  82 ++++++++
 .../gdb.base/retain-disabled-bp-loc.exp       | 192 ++++++++++++++++++
 .../gdb.base/retain-disabled-bp-loc.py        |  36 ++++
 6 files changed, 452 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/retain-disabled-bp-loc-lib1.c
 create mode 100644 gdb/testsuite/gdb.base/retain-disabled-bp-loc-lib2.c
 create mode 100644 gdb/testsuite/gdb.base/retain-disabled-bp-loc.c
 create mode 100644 gdb/testsuite/gdb.base/retain-disabled-bp-loc.exp
 create mode 100644 gdb/testsuite/gdb.base/retain-disabled-bp-loc.py

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index b7e4f5d0a45..b8e0bd6b24a 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -12941,6 +12941,31 @@ breakpoint::steal_locations (program_space *pspace)
   return ret;
 }
 
+/* Create a copy of a shlib_disabled location LOC.  This is used by
+   update_breakpoint_locations when a shlib_disabled location needs to be
+   added back onto a breakpoint, but we need to keep a copy in the
+   existing_locations list.  Enough information is copied from LOC into
+   the newly created location so that locations_are_equal will recognise
+   them as being identical.  */
+
+static bp_location *
+clone_shlib_disabled_loc (const bp_location &loc)
+{
+  gdb_assert (loc.shlib_disabled);
+
+  bp_location *clone = loc.owner->allocate_location ();
+
+  clone->address = loc.address;
+  clone->enabled = loc.enabled;
+  clone->disabled_by_cond = loc.disabled_by_cond;
+  clone->symbol = loc.symbol;
+  clone->msymbol = loc.msymbol;
+  clone->shlib_disabled = loc.shlib_disabled;
+
+  return clone;
+}
+
+
 /* Create new breakpoint locations for B (a hardware or software
    breakpoint) based on SALS and SALS_END.  If SALS_END.NELTS is not
    zero, then B is a ranged breakpoint.  Only recreates locations for
@@ -13009,6 +13034,9 @@ update_breakpoint_locations (code_breakpoint *b,
 
 	  new_loc->length = end - sals[0].pc + 1;
 	}
+
+      /* New locations are never shlib_disabled.  */
+      gdb_assert (!new_loc->shlib_disabled);
     }
 
   /* If possible, carry over 'disable' status from existing
@@ -13060,6 +13088,68 @@ update_breakpoint_locations (code_breakpoint *b,
       }
   }
 
+  /* For every shlib_disabled location in EXISTING_LOCATIONS, if there
+     isn't a non-shlib_disabled location in the breakpoint's location list,
+     then copy the shlib_disabled location back onto the breakpoint.  This
+     allows us to retain shlib_disabled locations for shared libraries that
+     have been unloaded.  */
+  for (auto it = existing_locations.begin (); it != existing_locations.end (); )
+    {
+      bool removed = false;
+
+      if (it->shlib_disabled)
+	{
+	  /* This is a shlib_disabled location, check to see if a
+	     non-shlib_disabled location has been created for this
+	     breakpoint.  If it has then the previously shlib_disabled
+	     location is considered re-activated.  */
+	  bool found_match = false;
+	  for (bp_location &l : b->locations ())
+	    {
+	      /* Breakpoint addresses are stored in ascending address
+		 order.  If L is after IT then no locations after L are
+		 going have a matching address.  */
+	      if (l.address > it->address)
+		break;
+
+	      if (breakpoint_locations_match (&(*it), &l, true))
+		{
+		  found_match = true;
+		  break;
+		}
+	    }
+
+	  /* If no match was found, then this shlib_disabled location
+	     should be preserved, i.e. added back to the breakpoint.  */
+	  if (!found_match)
+	    {
+	      /* Create a new location we can add to EXISTING_LOCATIONS,
+		 this is needed so locations_are_equal has something to
+		 compare with.  */
+	      bp_location *dummy = clone_shlib_disabled_loc (*it);
+
+	      /* Add the new dummy location before IT, this is just before
+		 the location we are about to remove.  */
+	      existing_locations.insert (it, *dummy);
+
+	      /* Now remove IT.  This updates IT to point to the next
+		 element, so we set REMOVED, this prevents the later
+		 increment of the iterator.  */
+	      bp_location &loc = *it;
+	      it = existing_locations.erase (it);
+	      removed = true;
+
+	      /* Add this shlib_disabled location back to the breakpoint.  */
+	      ((breakpoint *) b)->add_location (loc);
+	    }
+	}
+
+      /* If we didn't remove this location from the EXISTING_LOCATIONS list
+	 then the iterator will not have moved on, move it on now.  */
+      if (!removed)
+	++it;
+    }
+
   if (!locations_are_equal (existing_locations, b->locations ()))
     notify_breakpoint_modified (b);
 }
diff --git a/gdb/testsuite/gdb.base/retain-disabled-bp-loc-lib1.c b/gdb/testsuite/gdb.base/retain-disabled-bp-loc-lib1.c
new file mode 100644
index 00000000000..afc5b0f3281
--- /dev/null
+++ b/gdb/testsuite/gdb.base/retain-disabled-bp-loc-lib1.c
@@ -0,0 +1,26 @@
+/* Copyright 2024 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/>.  */
+
+static void
+foo ()
+{
+  /* Nothing.  */
+}
+
+void
+lib1_func ()
+{
+  foo ();
+}
diff --git a/gdb/testsuite/gdb.base/retain-disabled-bp-loc-lib2.c b/gdb/testsuite/gdb.base/retain-disabled-bp-loc-lib2.c
new file mode 100644
index 00000000000..22d5f715f60
--- /dev/null
+++ b/gdb/testsuite/gdb.base/retain-disabled-bp-loc-lib2.c
@@ -0,0 +1,26 @@
+/* Copyright 2024 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/>.  */
+
+static void
+bar ()
+{
+  /* Nothing.  */
+}
+
+void
+lib2_func ()
+{
+  bar ();
+}
diff --git a/gdb/testsuite/gdb.base/retain-disabled-bp-loc.c b/gdb/testsuite/gdb.base/retain-disabled-bp-loc.c
new file mode 100644
index 00000000000..19665a8c49b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/retain-disabled-bp-loc.c
@@ -0,0 +1,82 @@
+/* Copyright 2024 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/>.  */
+
+#include <dlfcn.h>
+#include <stdlib.h>
+
+static void
+foo ()
+{
+  /* Nothing.  */
+}
+
+void
+breakpt ()
+{
+  /* Nothing.  */
+}
+
+int
+main (void)
+{
+  void *handle;
+  void (*func)(int);
+
+  breakpt ();
+  breakpt ();		/* Breakpoint 1.  */
+
+  /* Load the first shared library.  */
+  handle = dlopen (SHLIB_1_NAME, RTLD_LAZY);
+  if (handle == NULL)
+    abort ();
+
+  breakpt ();		/* Breakpoint 2.  */
+
+  /* Unload the shared library.  */
+  if (dlclose (handle) != 0)
+    abort ();
+
+  breakpt ();		/* Breakpoint 3.  */
+
+  /* Load the second shared library.  */
+  handle = dlopen (SHLIB_2_NAME, RTLD_LAZY);
+  if (handle == NULL)
+    abort ();
+
+  breakpt ();		/* Breakpoint 4.  */
+
+  /* Unload the shared library.  */
+  if (dlclose (handle) != 0)
+    abort ();
+
+  breakpt ();		/* Breakpoint 5.  */
+
+  /* Load the first shared library for a second time.  */
+  handle = dlopen (SHLIB_1_NAME, RTLD_LAZY);
+  if (handle == NULL)
+    abort ();
+
+  breakpt ();		/* Breakpoint 6.  */
+
+  /* Unload the shared library.  */
+  if (dlclose (handle) != 0)
+    abort ();
+
+  breakpt ();		/* Breakpoint 7.  */
+
+  foo ();
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/retain-disabled-bp-loc.exp b/gdb/testsuite/gdb.base/retain-disabled-bp-loc.exp
new file mode 100644
index 00000000000..efa1eaaccca
--- /dev/null
+++ b/gdb/testsuite/gdb.base/retain-disabled-bp-loc.exp
@@ -0,0 +1,192 @@
+# Copyright 2024 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/>.
+
+# Test that GDB will retain shlib_disabled breakpoints.  Breakpoints
+# that are shlib_disabled will show up as <PENDING> in the 'info
+# breakpoints' output.  These breakpoints should be retained, and
+# then, reactivated if the same shared library is reloaded later on.
+
+require allow_python_tests
+require allow_shlib_tests
+
+standard_testfile .c -lib1.c -lib2.c
+
+set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
+
+set libfile1 $binfile-lib1.so
+set libfile2 $binfile-lib2.so
+
+if {[build_executable "build first shared library" $libfile1 $srcfile2 \
+	 {debug shlib}] == -1} {
+    return
+}
+
+if {[build_executable "build second shared library" $libfile2 $srcfile3 \
+	 {debug shlib}] == -1} {
+    return
+}
+
+set libfile1_target [gdb_download_shlib $libfile1]
+set libfile2_target [gdb_download_shlib $libfile2]
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
+	  [list debug \
+	       additional_flags=-DSHLIB_1_NAME=\"$libfile1_target\" \
+	       additional_flags=-DSHLIB_2_NAME=\"$libfile2_target\" \
+	       shlib_load]] } {
+    return -1
+}
+
+if {![runto_main]} {
+    return
+}
+
+gdb_breakpoint [gdb_get_line_number "Breakpoint 1"]
+gdb_breakpoint [gdb_get_line_number "Breakpoint 2"]
+gdb_breakpoint [gdb_get_line_number "Breakpoint 3"]
+gdb_breakpoint [gdb_get_line_number "Breakpoint 4"]
+gdb_breakpoint [gdb_get_line_number "Breakpoint 5"]
+gdb_breakpoint [gdb_get_line_number "Breakpoint 6"]
+gdb_breakpoint [gdb_get_line_number "Breakpoint 7"]
+
+gdb_breakpoint "foo"
+set bpnum [get_integer_valueof "\$bpnum" "INVALID" \
+	       "get b/p number for breakpoint on foo"]
+
+gdb_test "source $pyfile" "^OK" \
+    "load python script"
+
+# Check the 'info breakpoints' output of breakpoint BPNUM.  The string
+# STATUS indicates what the expected output should be.  Possible
+# values are:
+#
+# 'single' - breakpoint should have a single location in the main
+#            source file.
+#
+# 'two, both active' - breakpoint should have two locations, one in
+#                      the main source file, and one in the first
+#                      shared library.
+#
+# 'two, one pending' - breakpoint should have two locations, one in
+#                      the main source file, and one pending location.
+proc check_foo_breakpoint { bpnum status } {
+    if { $status eq "single" } {
+	gdb_test "info breakpoints $bpnum" \
+	    "\r\n$bpnum\\s+breakpoint\\s+keep\\s+y\\s+$::hex\\s+in foo at \[^\r\n\]+/[string_to_regexp $::srcfile]:$::decimal" \
+	    "info breakpoint on 'foo' b/p"
+    } elseif { $status eq "two, both active" } {
+	gdb_test "info breakpoints $bpnum" \
+	    [multi_line \
+		 "" \
+		 "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+<MULTIPLE>\\s*" \
+		 "$bpnum\\.1\\s+y\\s+$::hex\\s+in foo at \[^\r\n\]+/[string_to_regexp $::srcfile]:$::decimal" \
+		 "$bpnum\\.2\\s+y\\s+$::hex\\s+in foo at \[^\r\n\]+/[string_to_regexp $::srcfile2]:$::decimal"] \
+	    "info breakpoint on 'foo' b/p"
+    } elseif { $status eq "two, one pending" } {
+	gdb_test "info breakpoints $bpnum" \
+	    [multi_line \
+		 "" \
+		 "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+<MULTIPLE>\\s*" \
+		 "$bpnum\\.1\\s+y\\s+$::hex\\s+in foo at \[^\r\n\]+/[string_to_regexp $::srcfile]:$::decimal" \
+		 "$bpnum\\.2\\s+y\\s+<PENDING>\\s+foo"] \
+	    "info breakpoint on 'foo' b/p"
+    } else {
+	error "unknown status: $status"
+    }
+}
+
+# Check if breakpoint BPNUM was modified or not (depending on
+# EXPECT_MODIFIED).  Resets the list of modified breakpoints to empty
+# (see Python code) ready for the next test.
+proc check_modified_bp { bpnum expect_modified } {
+    if { $expect_modified } {
+	set out "Was modified"
+    } else {
+	set out "Was not modified"
+    }
+    gdb_test "python check_if_modified($bpnum)" "^$out"
+}
+
+gdb_continue_to_breakpoint "continue to b/p location 1" \
+    ".*Breakpoint 1.*"
+
+with_test_prefix "at b/p 1" {
+    # At this point there should be a single location for 'foo' b/p.
+    check_foo_breakpoint $bpnum "single"
+}
+
+gdb_continue_to_breakpoint "continue to b/p location 2" \
+    ".*Breakpoint 2.*"
+
+with_test_prefix "at b/p 2" {
+    # The inferior has loaded a shared library that includes a new
+    # location for the 'foo' breakpoint.  There should now be two
+    # locations.
+    check_modified_bp $bpnum true
+    check_foo_breakpoint $bpnum "two, both active"
+}
+
+gdb_continue_to_breakpoint "continue to b/p location 3" \
+    ".*Breakpoint 3.*"
+
+with_test_prefix "at b/p 3" {
+    # The inferior has unloaded the shared library containing the
+    # second 'foo' breakpoint location.  The location should have been
+    # retained, but marked as pending.
+    check_modified_bp $bpnum true
+    check_foo_breakpoint $bpnum "two, one pending"
+}
+
+gdb_continue_to_breakpoint "continue to b/p location 4" \
+    ".*Breakpoint 4.*"
+
+with_test_prefix "at b/p 4" {
+    # The inferior has loaded a second shared library.  This one
+    # doesn't include a location for the 'foo' breakpoint, but we
+    # expect the pending location to be retained.
+    check_modified_bp $bpnum false
+    check_foo_breakpoint $bpnum "two, one pending"
+}
+
+gdb_continue_to_breakpoint "continue to b/p location 5" \
+    ".*Breakpoint 5.*"
+
+with_test_prefix "at b/p 5" {
+    # The inferior has unloaded the second shared library.  The
+    # pending location for 'foo' should still be around.
+    check_modified_bp $bpnum false
+    check_foo_breakpoint $bpnum "two, one pending"
+}
+
+gdb_continue_to_breakpoint "continue to b/p location 6" \
+    ".*Breakpoint 6.*"
+
+with_test_prefix "at b/p 6" {
+    # The inferior has reloaded the first shared library.  The pending
+    # location should once again be active (i.e. not pending).
+    check_modified_bp $bpnum true
+    check_foo_breakpoint $bpnum "two, both active"
+}
+
+gdb_continue_to_breakpoint "continue to b/p location 7" \
+    ".*Breakpoint 7.*"
+
+with_test_prefix "at b/p 7" {
+    # The inferior has unloaded the shared library containing the
+    # second 'foo' breakpoint location (again).  The location should
+    # have been retained, and once again, be marked as pending.
+    check_modified_bp $bpnum true
+    check_foo_breakpoint $bpnum "two, one pending"
+}
diff --git a/gdb/testsuite/gdb.base/retain-disabled-bp-loc.py b/gdb/testsuite/gdb.base/retain-disabled-bp-loc.py
new file mode 100644
index 00000000000..6e08d8f1e91
--- /dev/null
+++ b/gdb/testsuite/gdb.base/retain-disabled-bp-loc.py
@@ -0,0 +1,36 @@
+# Copyright (C) 2024 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/>.
+
+bp_modified_list = []
+
+
+def bp_modified(bp):
+    global bp_modified_list
+    bp_modified_list.append(bp.number)
+
+
+gdb.events.breakpoint_modified.connect(bp_modified)
+
+
+def check_if_modified(num):
+    global bp_modified_list
+    if num in bp_modified_list:
+        print("Was modified")
+    else:
+        print("Was not modified")
+    bp_modified_list = []
+
+
+print("OK")

base-commit: a723c56efb07c4f8b3f6a3ed4b878a2f8f5572cc
-- 
2.25.4