[PATCHv2 2/6] gdb: fixes for code_breakpoint::disabled_by_cond logic

[Andrew Burgess <aburgess@redhat.com>]

I spotted that the code_breakpoint::disabled_by_cond flag doesn't work
how I'd expect it too.  The flag appears to be "sticky" in some
situations; once a code_breakpoint::disabled_by_cond flag is marked
true, then, in some cases the flag wont automatically become false
again, even when you'd think it should.

The problem is in update_breakpoint_locations.  In this function which
is called as a worker of code_breakpoint::re_set, GDB computes a new
set of locations for a breakpoint, the new locations are then
installed into the breakpoint.

However, before installing the new locations GDB attempts to copy the
bp_location::enabled and bp_location::disabled_by_cond flag from the
old locations into the new locations.

The reason for copying the ::enabled flag makes sense.  This flag is
controlled by the user.  When we create a new location set if we can
see that a new location is equivalent to one of the old locations, and
if the old location was disabled by the user, then the new location
should also be disabled.

However, I think the logic behind copying the ::disabled_by_cond flag
is wrong.  The disabled_by_cond flag is controlled by GDB and should
toggle automatically.  If the condition string can be parsed then the
flag should be true, if the condition string can't be parsed then the
flag should be false.

As we always parse the condition string in update_breakpoint_locations
before we try to copy the old enabled flag value then the
disabled_by_cond flag should already be correct, there's no need to
copy over the disabled_by_cond value from the old location.

As a concrete example, consider a b/p placed within the main
executable, but with a condition that depends on a variable within a
shared library.

When the b/p is initially created the b/p will be disabled as the
condition string will be invalid (the shared library variable isn't
known yet).

Now, when the inferior starts the shared library is loaded and the
condition variable becomes known to GDB.  When the shared library is
loaded breakpoint_re_set is called which (eventually) calls
update_breakpoint_locations.

A new location is computed for the breakpoint and the condition string
is parsed.  As the shared library variable is now know the expression
parses correctly and disabled_by_cond is left false for the new
location.

But then GDB spots that the new location is at the same address as the
old location and copies disabled_by_cond over from the old location.

The solution is simple, don't copy over disabled_by_cond.

While writing a test I found another problem though.  The
disabled_by_cond flag doesn't get set true when it should!  This is
the exact opposite of the above.

The problem here is in solib_add which is (despite the name) called
whenever the shared library set changes, including when a shared
library is unloaded.

Imagine an executable that uses dlopen/dlclose to load a shared
library.  Given an example of a b/p in the main executable that has a
condition that uses a variable from our shared library, a library
which might be unloaded with dlclose.

My expectation is that, when the library is unloaded, GDB will
automatically mark the breakpoint as disabled_by_cond, however, this
was not happening.

The problem is that in solib_add we only call breakpoint_re_set when
shared libraries are added, not when shared libraries are removed.

The solution I think is to just call breakpoint_re_set in both cases,
now the disabled_by_cond flag is updated as I'd expect.

There are, of course, tests to cover all these cases.

During review I was pointed at bug PR gdb/32079 as something that this
commit might fix, or help in fixing.

And this commit is part of the fix for that bug, but is not the
complete solution.  However, the remaining parts of the fix for that
bug are not really related to the content of this commit.

The problem in PR gdb/32079 is that the inferior maps multiple copies
of libc in different linker namespaces using dlmopen (actually libc is
loaded as a consequence of loading some other library into a different
namespace, but that's just a detail).  The user then uses a 'next'
command to move the inferior forward.

GDB sets up internal breakpoints on the longjmp symbols, of which
there are multiple copies in every loaded libc.

However, the 'next' command is, in the problem case, stepping over a
dlclose call which unloads one of the loaded libc libraries.

In current HEAD GDB in solib_add we fail to call breakpoint_re_set()
when the library is loaded, and breakpoint_re_set() will delete and
then recreate the longjmp breakpoints.  As breakpoint_re_set() is not
called GDB thinks that the the longjmp breakpoint in the now unloaded
libc still exists, and is still inserted.

When the inferior stops after the 'next' GDB will try to delete and
remove the longjmp breakpoint which will fail as the libc in which the
breakpoint was inserted is no longer mapped in.

When the user tries to 'next' again GDB tries to re-insert the still
existing longjmp breakpoint which again fails as the memory in which
the b/p should be inserted is no longer part of the inferior memory
space.

This commit helps a little.  Now when the libc library is unmapped we
call breakpoint_re_set().  This deletes the longjmp breakpoints
including the one in the unmapped library, then, when we try to
recreate the longjmp breakpoints (at the end of breakpoint_re_set) we
don't create a b/p in the now unmapped copy of libc.

However GDB does still think that the deleted breakpoint is inserted.
The breakpoint location remains on GDB's data structures until the
next time the inferior stops, at which point GDB tries to remove the
breakpoint .... and fails.

However, as the b/p is now deleted, when the user tries to 'next' GDB
no longer tries to re-insert the b/p, and so one of the problems
reported in PR gdb/32079 is resolved.

I'll fix the remaining issues in PR gdb/32079 in a later commit in
this series.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32079
---
 gdb/breakpoint.c                              |   4 +-
 gdb/solib.c                                   |   2 +-
 .../gdb.base/bp-disabled-by-cond-lib.c        |  24 +++
 gdb/testsuite/gdb.base/bp-disabled-by-cond.c  |  64 ++++++
 .../gdb.base/bp-disabled-by-cond.exp          | 197 ++++++++++++++++++
 gdb/testsuite/gdb.base/bp-disabled-by-cond.py |  21 ++
 6 files changed, 308 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/bp-disabled-by-cond-lib.c
 create mode 100644 gdb/testsuite/gdb.base/bp-disabled-by-cond.c
 create mode 100644 gdb/testsuite/gdb.base/bp-disabled-by-cond.exp
 create mode 100644 gdb/testsuite/gdb.base/bp-disabled-by-cond.py

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 17bd627f867..01937beb5fe 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -13036,7 +13036,7 @@ update_breakpoint_locations (code_breakpoint *b,
 
     for (const bp_location &e : existing_locations)
       {
-	if ((!e.enabled || e.disabled_by_cond) && e.function_name)
+	if (!e.enabled && e.function_name)
 	  {
 	    if (have_ambiguous_names)
 	      {
@@ -13052,7 +13052,6 @@ update_breakpoint_locations (code_breakpoint *b,
 		    if (breakpoint_locations_match (&e, &l, true))
 		      {
 			l.enabled = e.enabled;
-			l.disabled_by_cond = e.disabled_by_cond;
 			break;
 		      }
 		  }
@@ -13065,7 +13064,6 @@ update_breakpoint_locations (code_breakpoint *b,
 				 l.function_name.get ()) == 0)
 		    {
 		      l.enabled = e.enabled;
-		      l.disabled_by_cond = e.disabled_by_cond;
 		      break;
 		    }
 	      }
diff --git a/gdb/solib.c b/gdb/solib.c
index 49f607514dc..617f32a2161 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -1012,7 +1012,7 @@ solib_add (const char *pattern, int from_tty, int readsyms)
 	    }
 	}
 
-    if (loaded_any_symbols)
+    if (loaded_any_symbols || !current_program_space->deleted_solibs.empty ())
       breakpoint_re_set ();
 
     if (from_tty && pattern && !any_matches)
diff --git a/gdb/testsuite/gdb.base/bp-disabled-by-cond-lib.c b/gdb/testsuite/gdb.base/bp-disabled-by-cond-lib.c
new file mode 100644
index 00000000000..ab0cb9dcb16
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bp-disabled-by-cond-lib.c
@@ -0,0 +1,24 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>.  */
+
+volatile int lib_global = 0;
+
+void
+lib_func (void)
+{
+  /* Nothing.  */
+}
diff --git a/gdb/testsuite/gdb.base/bp-disabled-by-cond.c b/gdb/testsuite/gdb.base/bp-disabled-by-cond.c
new file mode 100644
index 00000000000..cf61c2821df
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bp-disabled-by-cond.c
@@ -0,0 +1,64 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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 <assert.h>
+#include <stdlib.h>
+
+#ifdef __WIN32__
+#include <windows.h>
+#define dlopen(name, mode) LoadLibrary (TEXT (name))
+#ifdef _WIN32_WCE
+# define dlsym(handle, func) GetProcAddress (handle, TEXT (func))
+#else
+# define dlsym(handle, func) GetProcAddress (handle, func)
+#endif
+#define dlclose(handle) FreeLibrary (handle)
+#else
+#include <dlfcn.h>
+#endif
+
+void
+breakpt ()
+{
+  /* Nothing.  */
+}
+
+int
+main (void)
+{
+  int res;
+  void *handle;
+  void (*func) (void);
+
+  handle = dlopen (SHLIB_NAME, RTLD_LAZY);
+  assert (handle != NULL);
+
+  func = (void (*)(void)) dlsym (handle, "lib_func");
+  assert (func != NULL);
+
+  breakpt ();	/* Conditional BP location.  */
+  breakpt ();	/* Other BP location.  */
+
+  func ();
+
+  res = dlclose (handle);
+  assert (res == 0);
+
+  breakpt ();	/* BP before exit.  */
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/bp-disabled-by-cond.exp b/gdb/testsuite/gdb.base/bp-disabled-by-cond.exp
new file mode 100644
index 00000000000..bb9c313799d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bp-disabled-by-cond.exp
@@ -0,0 +1,197 @@
+# 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/>.
+
+# Breakpoint locations can be marked as "disabled due to their
+# condition".  This test sets up a breakpoint which depends on reading
+# a variable in a shared library.  We then check that the b/p is
+# correctly disabled before the shared library is loaded, becomes
+# enabled once the shared library is loaded.  And becomes disabled
+# again when the shared libraries are unloaded.
+
+standard_testfile .c -lib.c
+
+set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
+
+set libname ${testfile}-lib
+set libfile [standard_output_file $libname]
+if { [build_executable "build shlib" $libfile $srcfile2 {debug shlib}] == -1} {
+    return
+}
+
+set opts [list debug shlib_load additional_flags=-DSHLIB_NAME=\"${libname}\"]
+if { [build_executable "build exec" $binfile $srcfile $opts] == -1} {
+    return
+}
+
+set other_bp_line [gdb_get_line_number "Other BP location" $srcfile]
+set cond_bp_line [gdb_get_line_number "Conditional BP location" $srcfile]
+set exit_bp_line [gdb_get_line_number "BP before exit" $srcfile]
+
+# Setup a conditional b/p, the condition of which depends on reading a
+# variable from a shared library.  The b/p will initially be created
+# disabled (due to the condition).
+#
+# Continue the inferior, when the shared library is loaded GDB should
+# make the b/p enabled.
+#
+# Restart the inferior, which should unload the shared library, GDB
+# should mark the b/p as disabled due to its condition again.
+proc run_test { hit_cond } {
+    clean_restart $::binfile
+
+    if {![runto_main]} {
+	return
+    }
+
+    # Setup breakpoints.
+    gdb_breakpoint $::srcfile:$::other_bp_line
+    set other_bp_num [get_integer_valueof "\$bpnum" "*UNKNOWN*" \
+			  "get number of other b/p"]
+
+    gdb_breakpoint $::srcfile:$::exit_bp_line
+    set exit_bp_num [get_integer_valueof "\$bpnum" "*UNKNOWN*" \
+			 "get number of exit b/p"]
+
+    gdb_breakpoint $::srcfile:$::cond_bp_line
+    set cond_bp_num [get_integer_valueof "\$bpnum" "*UNKNOWN*" \
+			"get number of conditional b/p"]
+
+    if { $hit_cond } {
+	set lib_global_val 0
+    } else {
+	set lib_global_val 1
+    }
+
+    # Set the condition.  Use 'force' as we're referencing a variable in
+    # the shared library, which hasn't been loaded yet.  The breakpoint
+    # will immediately be marked as disabled_by_cond.
+    gdb_test "condition -force $cond_bp_num lib_global == $lib_global_val" \
+	[multi_line \
+	     "warning: failed to validate condition at location $cond_bp_num\\.1, disabling:" \
+	     "  No symbol \"lib_global\" in current context\\."] \
+	"set b/p condition, it will be disabled"
+
+    # Source Python script if supported.
+    if { [allow_python_tests] } {
+	gdb_test_no_output "source $::pyfile" "import python scripts"
+	gdb_test "python print(bp_modified_list)" "\\\[\\\]" \
+	    "check b/p modified observer has not yet triggered"
+    }
+
+    # Check the b/p is indeed marked as disabled (based on its condition).
+    gdb_test "info breakpoint $cond_bp_num" \
+	[multi_line \
+	     "\r\n$cond_bp_num\\.1\\s+N\\*\\s+$::hex in main at \[^\r\n\]+" \
+	     "\\(\\*\\): Breakpoint condition is invalid at this location\\."] \
+	"conditional breakpoint is disabled based on condition"
+
+    if { $hit_cond } {
+	# Continue the inferior.  The shared library is loaded and the
+	# breakpoint condition should become valid.  We should then stop at
+	# the conditional breakpoint.
+	gdb_test "continue" \
+	    [multi_line \
+		 "Breakpoint $cond_bp_num, main \\(\\) at \[^\r\n\]+:$::cond_bp_line" \
+		 "$::cond_bp_line\\s+breakpt \\(\\);\\s+/\\* Conditional BP location\\.  \\*/"] \
+	    "continue until conditional b/p is hit"
+    } else {
+	# Continue the inferior.  The shared library is loaded and the
+	# breakpoint condition should become valid.  As the condition
+	# is going to be false GDB will stop at the other line.
+	gdb_test "continue" \
+	    [multi_line \
+		 "Breakpoint $other_bp_num, main \\(\\) at \[^\r\n\]+:$::other_bp_line" \
+		 "$::other_bp_line\\s+breakpt \\(\\);\\s+/\\* Other BP location\\.  \\*/"] \
+	    "continue until conditional b/p is hit"
+    }
+
+    if { [allow_python_tests] } {
+	# We're going to look at the list of b/p that have been
+	# modified since we loaded the Python script.  The first b/p
+	# modified will be the conditional b/p, this occurs when the
+	# b/p condition became valid.
+	#
+	# The second b/p will be whichever b/p we hit (the hit count
+	# increased).  So figure out which b/p we are going to hit.
+	if { $hit_cond } {
+	    set second_bp_num $cond_bp_num
+	} else {
+	    set second_bp_num $other_bp_num
+	}
+
+	# Now check the list of modified b/p.
+	gdb_test "python print(bp_modified_list)" \
+	    "\\\[$cond_bp_num, $second_bp_num\\\]" \
+	    "check b/p modified observer was triggered"
+    }
+
+    # Check the b/p is no longer marked as disabled.  The output is
+    # basically the same here whether the b/p was hit or not.  It's
+    # just the hit counter line that we need to append or not.
+    set re_list \
+	[list \
+	     "$cond_bp_num\\s+breakpoint\\s+keep\\s+y\\s+$::hex in main at \[^\r\n\]+:$::cond_bp_line" \
+	     "\\s+stop only if lib_global == $lib_global_val"]
+    if { $hit_cond } {
+	lappend re_list "\\s+breakpoint already hit 1 time"
+    }
+    set re [multi_line {*}$re_list]
+    gdb_test "info breakpoint $cond_bp_num" $re \
+	"conditional breakpoint is now enabled"
+
+    if { $hit_cond } {
+	gdb_test "continue" \
+	    [multi_line \
+		 "Breakpoint $other_bp_num, main \\(\\) at \[^\r\n\]+:$::other_bp_line" \
+		 "$::other_bp_line\\s+breakpt \\(\\);\\s+/\\* Other BP location\\.  \\*/"] \
+	    "continue to other b/p"
+    }
+
+    if {[allow_python_tests]} {
+	# Clear out the list of modified b/p.  This makes the results
+	# (see below) clearer.
+	gdb_test_no_output "python bp_modified_list=\[\]" \
+	    "clear bp_modified_list"
+    }
+
+    gdb_test "continue" \
+	[multi_line \
+	     "Breakpoint $exit_bp_num, main \\(\\) at \[^\r\n\]+:$::exit_bp_line" \
+	     "$::exit_bp_line\\s+breakpt \\(\\);\\s+/\\* BP before exit\\.  \\*/"] \
+	"continue b/p before exit"
+
+    # Check the b/p is once again marked as disabled based on its
+    # condition.
+    gdb_test "info breakpoint $cond_bp_num" \
+	[multi_line \
+	     "\r\n$cond_bp_num\\.1\\s+N\\*\\s+$::hex in main at \[^\r\n\]+" \
+	     "\\(\\*\\): Breakpoint condition is invalid at this location\\."] \
+	"conditional breakpoint is again disabled based on condition"
+
+    if { [allow_python_tests] } {
+	# The condition breakpoint will have been modified (moved to
+	# the disabled state) when GDB unloaded the shared libraries.
+	# And the b/p in main will have been modified in that it's hit
+	# count will have gone up.
+	gdb_test "python print(bp_modified_list)" \
+	    "\\\[$cond_bp_num, $exit_bp_num\\\]" \
+	    "check b/p modified observer was triggered during restart"
+    }
+}
+
+# The tests.
+foreach_with_prefix hit_cond { true false } {
+    run_test $hit_cond
+}
diff --git a/gdb/testsuite/gdb.base/bp-disabled-by-cond.py b/gdb/testsuite/gdb.base/bp-disabled-by-cond.py
new file mode 100644
index 00000000000..87b374c201e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bp-disabled-by-cond.py
@@ -0,0 +1,21 @@
+# 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):
+    bp_modified_list.append (bp.number)
+
+gdb.events.breakpoint_modified.connect(bp_modified)
-- 
2.25.4