gdb: reduce breakpoint-modified events for dprintf b/p

Consider this backtrace within GDB: #0 notify_breakpoint_modified (b=0x57d31d0) at ../../src/gdb/breakpoint.c:1083 #1 0x00000000005b6406 in breakpoint_set_commands (b=0x57d31d0, commands=...) at ../../src/gdb/breakpoint.c:1523 #2 0x00000000005c8c63 in update_dprintf_command_list (b=0x57d31d0) at ../../src/gdb/breakpoint.c:8641 #3 0x00000000005d3c4e in dprintf_breakpoint::re_set (this=0x57d31d0) at ../../src/gdb/breakpoint.c:12476 #4 0x00000000005d6347 in breakpoint_re_set () at ../../src/gdb/breakpoint.c:13298 Whenever breakpoint_re_set is called we re-build the commands that the dprintf b/p will execute and store these into the breakpoint. The commands are re-built in update_dprintf_command_list and stored into the breakpoint object in breakpoint_set_commands. Now sometimes these commands can change, dprintf_breakpoint::re_set explains one case where this can occur, and I'm sure there must be others. But in most cases the commands we recalculate will not change. This means that the breakpoint modified event which is emitted from breakpoint_set_commands is redundant. This commit aims to eliminate the redundant breakpoint modified events for dprintf breakpoints. This is done by adding a commands_equal call to the start of breakpoint_set_commands. The commands_equal function is a new function which compares two command_line objects and returns true if they are identical. Using this function we can check if the new commands passed to breakpoint_set_commands are identical to the breakpoint's existing commands. If the new commands are equal then we don't need to change anything on the new breakpoint, and the breakpoint modified event can be skipped. The test for this commit stops at a dlopen() call in the inferior, sets up a dprintf breakpoint, then uses 'next' to step over the dlopen() call. When the library loads GDB call breakpoint_re_set, which calls dprintf_breakpoint::re_set. But in this case we don't expect the calculated command string to change, so we don't expect to see the breakpoint modified event.
author: Andrew Burgess <aburgess@redhat.com> 2024-09-07 13:45:40 +0100
committer: Andrew Burgess <aburgess@redhat.com> 2025-03-28 23:39:07 +0000
commit: e83f6121679065bc79aa0cd41225fde48b8b25d4 (patch)
tree: 686369ae72659760f42de6aaf4af858cd9758bc7
parent: 43ac3df61492b94bf13c11bd98c9f9541e92d3b0 (diff)
download: binutils-e83f6121679065bc79aa0cd41225fde48b8b25d4.zip
binutils-e83f6121679065bc79aa0cd41225fde48b8b25d4.tar.gz
binutils-e83f6121679065bc79aa0cd41225fde48b8b25d4.tar.bz2
6 files changed, 270 insertions, 0 deletions
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3085ca1..189d7b8 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1535,6 +1535,11 @@ void
 breakpoint_set_commands (struct breakpoint *b, 
 			 counted_command_line &&commands)
 {
+  /* If the commands have not changed then there's no need to update
+     anything, and no need to emit a breakpoint modified event.  */
+  if (commands_equal (b->commands.get (), commands.get ()))
+    return;
+
   validate_commands_for_breakpoint (b, commands.get ());
 
   b->commands = std::move (commands);
diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index c14480f..0337d01 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -1625,6 +1625,65 @@ define_prefix_command (const char *comname, int from_tty)
   c->allow_unknown = c->user_commands.get () != nullptr;
 }
 
+/* See cli/cli-script.h.  */
+
+bool
+commands_equal (const command_line *a, const command_line *b)
+{
+  if ((a == nullptr) != (b == nullptr))
+    return false;
+
+  while (a != nullptr)
+    {
+      /* We are either at the end of both command lists, or there's
+	 another command in both lists.  */
+      if ((a->next == nullptr) != (b->next == nullptr))
+	return false;
+
+      /* There's a command line for both, or neither.  */
+      if ((a->line == nullptr) != (b->line == nullptr))
+	return false;
+
+      /* Check control_type matches.  */
+      if (a->control_type != b->control_type)
+	return false;
+
+      if (a->control_type == compile_control)
+	{
+	  if (a->control_u.compile.scope != b->control_u.compile.scope)
+	    return false;
+
+	  /* This is where we "fail safe".  The scope_data is a 'void *'
+	     pointer which changes in meaning based on the value of
+	     'scope'.  It is possible that two different 'void *' pointers
+	     could point to the equal scope data, however, we just assume
+	     that if the pointers are different, then the scope_data is
+	     different.  This could be improved in the future.  */
+	  if (a->control_u.compile.scope_data
+	      != b->control_u.compile.scope_data)
+	    return false;
+	}
+
+      /* Check lines are identical.  */
+      if (a->line != nullptr && strcmp (a->line, b->line) != 0)
+	return false;
+
+      /* Check body_list_0.  */
+      if (!commands_equal (a->body_list_0.get (), b->body_list_0.get ()))
+	return false;
+
+      /* Check body_list_1.  */
+      if (!commands_equal (a->body_list_1.get (), b->body_list_1.get ()))
+	return false;
+
+      /* Move to the next element in each chain.  */
+      a = a->next;
+      b = b->next;
+    }
+
+  return true;
+}
+
 
 /* Used to implement source_command.  */
 
diff --git a/gdb/cli/cli-script.h b/gdb/cli/cli-script.h
index df7316e..23a1e1f 100644
--- a/gdb/cli/cli-script.h
+++ b/gdb/cli/cli-script.h
@@ -184,4 +184,14 @@ extern void print_command_trace (const char *cmd, ...)
 
 extern void reset_command_nest_depth (void);
 
+/* Return true if A and B are identical.  Some commands carry around a
+   'void *' compilation context, in this case this function doesn't try to
+   validate if the context is actually the same or not, and will just
+   return false indicating the commands have changed.  That is, a return
+   value of true is a guarantee that the commands are equal, a return
+   value of false means the commands are possibly different (and in most
+   cases are different).  */
+
+extern bool commands_equal (const command_line *a, const command_line *b);
+
 #endif /* GDB_CLI_CLI_SCRIPT_H */
diff --git a/gdb/testsuite/gdb.mi/mi-dprintf-modified-lib.c b/gdb/testsuite/gdb.mi/mi-dprintf-modified-lib.c
new file mode 100644
index 0000000..70fc328
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-dprintf-modified-lib.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2025 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/>.  */
+
+int
+foo (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.mi/mi-dprintf-modified.c b/gdb/testsuite/gdb.mi/mi-dprintf-modified.c
new file mode 100644
index 0000000..7a41adbac
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-dprintf-modified.c
@@ -0,0 +1,55 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2025 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 <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
+
+#include <assert.h>
+
+int
+main (void)
+{
+  int res;
+  void *handle;
+  int (*func) (void);
+  int val = 0;
+
+  handle = dlopen (SHLIB_NAME, RTLD_LAZY);		/* Break here.  */
+  assert (handle != NULL);
+
+  func = (int (*)(void)) dlsym (handle, "foo");
+  assert (func != NULL);
+
+  val += func ();
+
+  res = dlclose (handle);
+  assert (res == 0);
+
+  return val;
+}
diff --git a/gdb/testsuite/gdb.mi/mi-dprintf-modified.exp b/gdb/testsuite/gdb.mi/mi-dprintf-modified.exp
new file mode 100644
index 0000000..c3e1bdf
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-dprintf-modified.exp
@@ -0,0 +1,119 @@
+# Copyright 2025 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/>.
+
+# Check that GDB doesn't emit a 'breakpoint-modified' notification for
+# dprintf breakpoints when the dprintf commands haven't changed.
+#
+# GDB use to emit a 'breakpoint-modified' dprintf breakpoints each
+# time the dprintf_breakpoint::re_set function was called as this
+# would re-cacluate the dprintf command string, even though in most
+# cases the calculated string was no different from the previous
+# value.
+#
+# Then GDB got smarter and could recognise that the string had not
+# changed, and so would skip the 'breakpoint-modified' notification.
+#
+# This test stops at a dlopen() call in the inferior and creates a
+# dprintf breakpoint.  Then we 'next' over the dlopen() which triggers
+# a call to the ::re_set() functions.  We check that there is no
+# 'breakpoint-modified' event emitted for the dprintf breakpoint.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+standard_testfile .c -lib.c
+
+# Build the library.
+set libname ${testfile}-lib
+set libfile [standard_output_file $libname]
+if { [build_executable "build shlib" $libfile $srcfile2 {debug shlib}] == -1} {
+    return
+}
+
+# Build the executable.
+set opts [list debug shlib_load additional_flags=-DSHLIB_NAME=\"${libname}\"]
+if { [build_executable "build exec" $binfile $srcfile $opts] == -1} {
+    return
+}
+
+# The line number of the dlopen() call.
+set bp_line [gdb_get_line_number "Break here" $srcfile]
+
+# Start the inferior.
+mi_clean_restart $binfile
+mi_runto_main
+
+# Place a breakpoint at the dlopen() line.
+mi_create_breakpoint $srcfile:$bp_line "set breakpoint at dlopen call" \
+    -disp keep -func main -file "\[^\r\n\]+/$srcfile" -line $bp_line
+
+# And run to the breakpoint.
+mi_execute_to "exec-continue" "breakpoint-hit" main "" ".*/$srcfile" \
+    $bp_line { "" "disp=\"keep\"" } "run to breakpoint"
+
+# Cleanup breakpoints.
+mi_delete_breakpoints
+
+# Setup a dprintf breakpoint.
+mi_gdb_test "-dprintf-insert --function main \"in main\"" \
+    "\\^done,bkpt={.*}" "dprintf at main"
+
+set bpnum [mi_get_valueof "/d" "\$bpnum" "INVALID" \
+	       "get number for dprintf breakpoint"]
+
+# Use 'next' to step over loading the shared library.
+mi_gdb_test "220-exec-next" ".*" "next over dlopen"
+
+# Now wait for the 'stopped' notification.  While we wait we should
+# see a 'library-loaded' notification for the loading of the shared
+# library.
+#
+# In older versions of GDB we would also see a 'breakpoint-modified'
+# notification for the dprintf breakpoint, but newer versions of GDB
+# are smart enough to not emit this unnecessary notification.
+set bp_re [mi_make_breakpoint -number $bpnum \
+	       -type dprintf -disp keep -enabled y -func main]
+set saw_bp_modified false
+set saw_lib_load false
+set saw_stopped false
+gdb_test_multiple "" "wait for 'next' to complete" {
+    -re "^=library-loaded,id=\[^\r\n\]+\r\n" {
+	set saw_lib_load true
+	exp_continue
+    }
+
+    -re "^=breakpoint-modified,$bp_re\r\n" {
+	set saw_bp_modified true
+	exp_continue
+    }
+
+    -re "^\\*stopped,reason=\"end-stepping-range\",\[^\r\n\]+\r\n" {
+	set saw_stopped true
+	exp_continue
+    }
+
+    -re "^$mi_gdb_prompt$" {
+	gdb_assert { $saw_lib_load } \
+	    "$gdb_test_name, library was loaded"
+	gdb_assert { $saw_stopped } \
+	    "$gdb_test_name, saw stopped message"
+	gdb_assert { !$saw_bp_modified } \
+	    "$gdb_test_name, no breakpoint-modified"
+    }
+
+    -re "^\[^\r\n\]+\r\n" {
+	exp_continue
+    }
+}
author	Andrew Burgess <aburgess@redhat.com>	2024-09-07 13:45:40 +0100
committer	Andrew Burgess <aburgess@redhat.com>	2025-03-28 23:39:07 +0000
commit	e83f6121679065bc79aa0cd41225fde48b8b25d4 (patch)
tree	686369ae72659760f42de6aaf4af858cd9758bc7
parent	43ac3df61492b94bf13c11bd98c9f9541e92d3b0 (diff)
download	binutils-e83f6121679065bc79aa0cd41225fde48b8b25d4.zip binutils-e83f6121679065bc79aa0cd41225fde48b8b25d4.tar.gz binutils-e83f6121679065bc79aa0cd41225fde48b8b25d4.tar.bz2