gdb: remove disable_breakpoints_in_shlibs function

I think there is a problem with the disable_breakpoints_in_shlibs
function: it can disable breakpoint locations without calling
notify_breakpoint_modified.  This means that the Python API's
breakpoint_modified event will not trigger, nor will the MI send a
breakpoint modified event.

I started looking at disable_breakpoints_in_shlibs because of an
earlier commit:

  commit 8c48ec7a6160aed0d1126c623443935e4435cd41
  Date:   Thu Aug 29 12:34:15 2024 +0100

      gdb: handle dprintf breakpoints when unloading a shared library

Currently disable_breakpoints_in_shlibs is only called from one
location, clear_solib in solib.c.  clear_solib also calls
notify_solib_unloaded for every solib in the program_space of
interest, and notify_solib_unloaded will call
disable_breakpoints_in_unloaded_shlib via the solib_unloaded
observer.  These two function, disable_breakpoints_in_shlibs and
disable_breakpoints_in_unloaded_shlib are very similar in what they
do.

I think that we can remove the disable_breakpoints_in_shlibs call, and
instead, tweak how we call disable_breakpoints_in_unloaded_shlib in
order to get the same end result, except that, after this change, we
will call notify_breakpoint_modified, which means the Python API event
will trigger, and the MI events will be emitted.

All that disable_breakpoints_in_shlibs does is disable some
breakpoints.

Meanwhile, disable_breakpoints_in_unloaded_shlib, will disable the
same set of breakpoints, call notify_breakpoint_modified, and
then (for some breakpoint types) print a message telling the user that
the breakpoint has been disabled.  However, this function will ignore
any breakpoints that are already disabled.

As disable_breakpoints_in_shlibs disables the same set of breakpoints,
the result of the current code is that disable_breakpoints_in_shlibs
serves only to prevent the notify_breakpoint_modified call, which I
think is wrong, and to prevent the user message being printed, which I
think is reasonable.

If we remove the disable_breakpoints_in_shlibs call without making any
additional changes, then we start to see some message printed in cases
like this:

  (gdb) start
  The program being debugged has been started already.
  Start it from the beginning? (y or n) y
  warning: Temporarily disabling breakpoints for unloaded shared library "/tmp/shared-lib-test/libfoo.so"
  Temporary breakpoint 3 at 0x40113e: file test.c, line 9.
  Starting program: /tmp/shared-lib-test/test.x

Notice the 'warning:' line, which is new.  I think this is confusing
because, in most cases the breakpoint will be enabled again by the
time the inferior reaches `main` and stops.

In the future I'm interested in exploring if GDB could be smarter
about when to print these 'Temporarily disabling breakpoints ...'
messages so that if the 'start' command does mean a breakpoint is left
disabled, then the user would be informed.  However, I don't propose
doing that work immediately, and certainly not in this commit.  For
now, my intention is to leave things as they are right now, GDB
doesn't warn about disabling breakpoints during an inferior re-start.

To achieve this I think we need to pass a new argument to
disable_breakpoints_in_unloaded_shlib which controls whether we should
print a message about the breakpoint being disabled or not.  With this
added we can now silence the warning when the inferior is
restarted (i.e. when disable_breakpoints_in_unloaded_shlib is called
from clear_solib), but keep the warning for cases like stepping over a
dlclose() call in the inferior.

After this commit, GDB now emits breakpoint modified events (in Python
and/or MI) when a breakpoint is disabled as a result of all shared
libraries being unloaded.  This will be visible in two places that I
can thing of, the 'nosharedlibrary' command, and when an inferior is
restarted.

5 files changed, 89 insertions, 41 deletions

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 189d7b8..0fb6fd9 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3104,7 +3104,6 @@ insert_bp_location (struct bp_location *bl,
 		  || shared_objfile_contains_address_p (bl->pspace,
 							bl->address)))
 	    {
-	      /* See also: disable_breakpoints_in_shlibs.  */
 	      bl->shlib_disabled = 1;
 	      notify_breakpoint_modified (bl->owner);
 	      if (!*disabled_breaks)
@@ -8084,44 +8083,19 @@ create_and_insert_solib_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR add
   return b;
 }
 
-/* See breakpoint.h.  */
-
-void
-disable_breakpoints_in_shlibs (program_space *pspace)
-{
-  for (bp_location *loc : all_bp_locations ())
-    {
-      /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL.  */
-      struct breakpoint *b = loc->owner;
-
-      /* We apply the check to all breakpoints, including disabled for
-	 those with loc->duplicate set.  This is so that when breakpoint
-	 becomes enabled, or the duplicate is removed, gdb will try to
-	 insert all breakpoints.  If we don't set shlib_disabled here,
-	 we'll try to insert those breakpoints and fail.  */
-      if (((b->type == bp_jit_event)
-	   || is_breakpoint (b)
-	   || is_tracepoint (b))
-	  && loc->pspace == pspace
-	  && !loc->shlib_disabled
-	  && solib_name_from_address (loc->pspace, loc->address)
-	  )
-	{
-	  loc->shlib_disabled = 1;
-	}
-    }
-}
-
 /* Disable any breakpoints and tracepoints that are in SOLIB upon
    notification of unloaded_shlib.  Only apply to enabled breakpoints,
    disabled ones can just stay disabled.
 
    When STILL_IN_USE is true, SOLIB hasn't really been unmapped from
-   the inferior.  In this case, don't disable anything.  */
+   the inferior.  In this case, don't disable anything.
+
+   When SILENT is false notify the user if any breakpoints are disabled,
+   otherwise, still disable the breakpoints, but don't tell the user.  */
 
 static void
 disable_breakpoints_in_unloaded_shlib (program_space *pspace, const solib &solib,
-				       bool still_in_use)
+				       bool still_in_use, bool silent)
 {
   if (still_in_use)
     return;
@@ -8165,7 +8139,7 @@ disable_breakpoints_in_unloaded_shlib (program_space *pspace, const solib &solib
 
 	  bp_modified = true;
 
-	  if (!disabled_shlib_breaks && user_breakpoint_p (&b))
+	  if (!disabled_shlib_breaks && !silent && user_breakpoint_p (&b))
 	    {
 	      target_terminal::ours_for_output ();
 	      warning (_("Temporarily disabling breakpoints "
diff --git a/gdb/bsd-uthread.c b/gdb/bsd-uthread.c
index 67db0ca..129e7a6 100644
--- a/gdb/bsd-uthread.c
+++ b/gdb/bsd-uthread.c
@@ -295,7 +295,7 @@ bsd_uthread_solib_loaded (solib &so)
 
 static void
 bsd_uthread_solib_unloaded (program_space *pspace, const solib &so,
-			    bool still_in_use)
+			    bool still_in_use, bool /* silent */)
 {
   if (bsd_uthread_solib_name.empty () || still_in_use)
     return;
diff --git a/gdb/observable.h b/gdb/observable.h
index deea1ff..c50891e 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -102,10 +102,14 @@ extern observable<inferior */* parent_inf */, inferior */* child_inf */,
 extern observable<solib &/* solib */> solib_loaded;
 
 /* The shared library SOLIB has been unloaded from program space PSPACE.
+   The SILENT argument indicates that GDB doesn't wish to notify the CLI
+   about any non-error consequences of unloading the solib, e.g. when
+   breakpoints are disabled.
+
    Note  when gdb calls this observer, the library's symbols have not
    been unloaded yet, and thus are still available.  */
 extern observable<program_space *, const solib &/* solib */,
-		  bool /* still_in_use */> solib_unloaded;
+		  bool /* still_in_use */, bool /* silent */> solib_unloaded;
 
 /* The symbol file specified by OBJFILE has been loaded.  */
 extern observable<struct objfile */* objfile */> new_objfile;
diff --git a/gdb/solib.c b/gdb/solib.c
index b1fdea9..0bbcb02 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -694,14 +694,17 @@ notify_solib_loaded (solib &so)
 /* Notify interpreters and observers that solib SO has been unloaded.
    When STILL_IN_USE is true, the objfile backing SO is still in use,
    this indicates that SO was loaded multiple times, but only mapped
-   in once (the mapping was reused).  */
+   in once (the mapping was reused).
+
+   When SILENT is true, don't announce to the user if any breakpoints are
+   disabled as a result of unloading SO.  */
 
 static void
 notify_solib_unloaded (program_space *pspace, const solib &so,
-		       bool still_in_use)
+		       bool still_in_use, bool silent)
 {
   interps_notify_solib_unloaded (so, still_in_use);
-  gdb::observers::solib_unloaded.notify (pspace, so, still_in_use);
+  gdb::observers::solib_unloaded.notify (pspace, so, still_in_use, silent);
 }
 
 /* See solib.h.  */
@@ -803,7 +806,7 @@ update_solib_list (int from_tty)
 	  /* Notify any observer that the shared object has been
 	     unloaded before we remove it from GDB's tables.  */
 	  notify_solib_unloaded (current_program_space, *gdb_iter,
-				 still_in_use);
+				 still_in_use, false);
 
 	  /* Unless the user loaded it explicitly, free SO's objfile.  */
 	  if (gdb_iter->objfile != nullptr
@@ -1163,14 +1166,12 @@ clear_solib (program_space *pspace)
 {
   const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
 
-  disable_breakpoints_in_shlibs (pspace);
-
   for (solib &so : pspace->so_list)
     {
       bool still_in_use
 	= (so.objfile != nullptr && solib_used (pspace, so));
 
-      notify_solib_unloaded (pspace, so, still_in_use);
+      notify_solib_unloaded (pspace, so, still_in_use, true);
       pspace->remove_target_sections (&so);
     };
 
diff --git a/gdb/testsuite/gdb.base/shlib-unload.exp b/gdb/testsuite/gdb.base/shlib-unload.exp
index f3e8cce..9d47416 100644
--- a/gdb/testsuite/gdb.base/shlib-unload.exp
+++ b/gdb/testsuite/gdb.base/shlib-unload.exp
@@ -225,6 +225,75 @@ proc_with_prefix test_dprintf_with_rerun {} {
 	"dprintf is non-pending after restart"
 }
 
+# Check that we see breakpoint modified events (where appropriate)
+# when the 'nosharedlibrary' command is used to unload all shared
+# libraries.
+#
+# Also check that the 'nosharedlibrary' doesn't trigger a warning
+# about shared library breakpoints being disabled.
+proc_with_prefix test_silent_nosharedlib {} {
+    if { ![allow_python_tests] } {
+	unsupported "python support needed"
+	return
+    }
+
+    foreach_with_prefix type { breakpoint dprintf } {
+	clean_restart $::binfile
+
+	if {![runto_main]} {
+	    return
+	}
+
+	gdb_breakpoint $::srcfile:$::bp_line
+	gdb_continue_to_breakpoint "stop before dlclose"
+
+	# Setup a dprintf or breakpoint in the shared library.
+	if { $type eq "breakpoint" } {
+	    gdb_test "break foo"
+	} else {
+	    gdb_test "dprintf foo,\"In foo\""
+	}
+
+	# Record the number of the b/p (or dprintf) we just inserted.
+	set bp_num [get_integer_valueof "\$bpnum" "*UNKNOWN*" \
+			"get b/p number"]
+
+	# Load Python library to track b/p modifications.
+	gdb_test_no_output "source $::pyfile" "import python scripts"
+
+	# Initialise the b/p modified hash.  Currently dprintf style
+	# breakpoints are not visible from Python, so the modification
+	# count will remain unchanged in that case.
+	gdb_test_no_output "python bp_modified_counts\[$bp_num\] = 0"
+
+	# Discard symbols from all loaded shared libraries.
+	gdb_test_no_output "nosharedlibrary"
+
+	# Check that our b/p is now showing as disabled.
+	if { $type eq "breakpoint" } {
+	    set re \
+		[list "$bp_num\\s+breakpoint\\s+keep\\s+y\\s+<PENDING>\\s+foo"]
+	    set count 1
+	} else {
+	    set re \
+		[list \
+		     "$bp_num\\s+dprintf\\s+keep\\s+y\\s+<PENDING>\\s+foo" \
+		     "\\s+printf \"In foo\""]
+	    set count 0
+	}
+
+	gdb_test "info breakpoints $bp_num" \
+	    [multi_line "^Num\\s+Type\\s+Disp\\s+Enb\\s+Address\\s+What" \
+		 {*}$re]
+
+	# Check we've seen the expected number of breakpoint modified
+	# events.  Currently dprintf breakpoints are not visible from
+	# Python, so we will not see an event in that case.
+	gdb_test "python print(bp_modified_counts\[$bp_num\])" "^$count"
+    }
+}
+
 test_bp_modified_events
 test_dprintf_after_unload
 test_dprintf_with_rerun
+test_silent_nosharedlib