[pushed 2/3] dprintf-style agent can't explain a trap.

[pushed 2/3] dprintf-style agent can't explain a trap.

[Pedro Alves]

If some event happens to trigger at the same address as a dprintf-style
agent dprintf is installed, GDB will complain, like:

 (gdb) continue
 Continuing.
 May only run agent-printf on the target
 (gdb)

Such dprintfs are completely handled on the target side, so they can't
explain a stop, but GDB is currently putting then on the bpstat chain
anyway, because they currently unconditionally use bkpt_breakpoint_hit
as breakpoint_hit method.

gdb/
2014-06-02  Pedro Alves  <palves@redhat.com>

	* breakpoint.c (dprintf_breakpoint_hit): New function.
	(initialize_breakpoint_ops): Install it as dprintf's
	breakpoint_hit method.
---
 gdb/ChangeLog    |  6 ++++++
 gdb/breakpoint.c | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6faa689..1d37c56 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2014-06-02  Pedro Alves  <palves@redhat.com>
+
+	* breakpoint.c (dprintf_breakpoint_hit): New function.
+	(initialize_breakpoint_ops): Install it as dprintf's
+	breakpoint_hit method.
+
 2014-06-02  Joel Brobecker  <brobecker@adacore.com>
 
 	* source.c (substitute_path_rule_matches): Simplify using
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 676c7b8..a966ba2 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -13142,6 +13142,23 @@ bkpt_breakpoint_hit (const struct bp_location *bl,
 }
 
 static int
+dprintf_breakpoint_hit (const struct bp_location *bl,
+			struct address_space *aspace, CORE_ADDR bp_addr,
+			const struct target_waitstatus *ws)
+{
+  if (dprintf_style == dprintf_style_agent
+      && target_can_run_breakpoint_commands ())
+    {
+      /* An agent-style dprintf never causes a stop.  If we see a trap
+	 for this address it must be for a breakpoint that happens to
+	 be set at the same address.  */
+      return 0;
+    }
+
+  return bkpt_breakpoint_hit (bl, aspace, bp_addr, ws);
+}
+
+static int
 bkpt_resources_needed (const struct bp_location *bl)
 {
   gdb_assert (bl->owner->type == bp_hardware_breakpoint);
@@ -16220,6 +16237,7 @@ initialize_breakpoint_ops (void)
   ops->print_mention = bkpt_print_mention;
   ops->print_recreate = dprintf_print_recreate;
   ops->after_condition_true = dprintf_after_condition_true;
+  ops->breakpoint_hit = dprintf_breakpoint_hit;
 }
 
 /* Chain containing all defined "enable breakpoint" subcommands.  */
-- 
1.9.0

[pushed 3/3] Installing a breakpoint on top of a dprintf makes GDB lose control.

[Pedro Alves]

While the full fix for PR 15180 isn't in, it's best if we at least
make sure that GDB doesn't lose control when a breakpoint is set at
the same address as a dprintf.

gdb/
2014-06-02  Pedro Alves  <palves@redhat.com>

	* breakpoint.c (build_target_command_list): Don't build a command
	list if we have any duplicate location that isn't a dprintf.

gdb/testsuite/
2014-06-02  Pedro Alves  <palves@redhat.com>

	* gdb.base/dprintf-bp-same-addr.c: New file.
	* gdb.base/dprintf-bp-same-addr.exp: New file.
---
 gdb/ChangeLog                                   |  5 ++
 gdb/testsuite/ChangeLog                         |  5 ++
 gdb/breakpoint.c                                | 20 ++++++--
 gdb/testsuite/gdb.base/dprintf-bp-same-addr.c   | 28 +++++++++++
 gdb/testsuite/gdb.base/dprintf-bp-same-addr.exp | 66 +++++++++++++++++++++++++
 5 files changed, 120 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/dprintf-bp-same-addr.c
 create mode 100644 gdb/testsuite/gdb.base/dprintf-bp-same-addr.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1d37c56..d8f1cbe 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2014-06-02  Pedro Alves  <palves@redhat.com>
 
+	* breakpoint.c (build_target_command_list): Don't build a command
+	list if we have any duplicate location that isn't a dprintf.
+
+2014-06-02  Pedro Alves  <palves@redhat.com>
+
 	* breakpoint.c (dprintf_breakpoint_hit): New function.
 	(initialize_breakpoint_ops): Install it as dprintf's
 	breakpoint_hit method.
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index eed85c1..f4d59cf 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-06-02  Pedro Alves  <palves@redhat.com>
+
+	* gdb.base/dprintf-bp-same-addr.c: New file.
+	* gdb.base/dprintf-bp-same-addr.exp: New file.
+
 2014-06-02  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>
 
 	* gdb.arch/powerpc-power.exp: Add power8 instructions to the testcase.
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index a966ba2..4e6c627 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2351,14 +2351,26 @@ build_target_command_list (struct bp_location *bl)
   /* Release commands left over from a previous insert.  */
   VEC_free (agent_expr_p, bl->target_info.tcommands);
 
-  /* For now, limit to agent-style dprintf breakpoints.  */
-  if (bl->owner->type != bp_dprintf
-      || strcmp (dprintf_style, dprintf_style_agent) != 0)
+  if (!target_can_run_breakpoint_commands ())
     return;
 
-  if (!target_can_run_breakpoint_commands ())
+  /* For now, limit to agent-style dprintf breakpoints.  */
+  if (dprintf_style != dprintf_style_agent)
     return;
 
+  /* For now, if we have any duplicate location that isn't a dprintf,
+     don't install the target-side commands, as that would make the
+     breakpoint not be reported to the core, and we'd lose
+     control.  */
+  ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
+    {
+      loc = (*loc2p);
+      if (is_breakpoint (loc->owner)
+	  && loc->pspace->num == bl->pspace->num
+	  && loc->owner->type != bp_dprintf)
+	return;
+    }
+
   /* Do a first pass to check for locations with no assigned
      conditions or conditions that fail to parse to a valid agent expression
      bytecode.  If any of these happen, then it's no use to send conditions
diff --git a/gdb/testsuite/gdb.base/dprintf-bp-same-addr.c b/gdb/testsuite/gdb.base/dprintf-bp-same-addr.c
new file mode 100644
index 0000000..b28e102
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-bp-same-addr.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright (C) 2014 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
+main (void)
+{
+  int x = 1, y = 1, z = 1;
+
+  ++x; /* before dprintf */
+  ++y; /* set dprintf here */
+  ++z; /* after dprintf */
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/dprintf-bp-same-addr.exp b/gdb/testsuite/gdb.base/dprintf-bp-same-addr.exp
new file mode 100644
index 0000000..b0c6d7b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-bp-same-addr.exp
@@ -0,0 +1,66 @@
+# Copyright 2014 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 doesn't lose control when a breakpoint is set at the
+# same address as a dprintf.
+
+standard_testfile
+
+if [prepare_for_testing "failed to prepare" \
+    ${testfile} ${srcfile} {debug}] {
+    return -1
+}
+
+set dp_location [gdb_get_line_number "set dprintf here"]
+
+proc test { style } {
+    global gdb_prompt binfile dp_location
+
+    with_test_prefix "$style" {
+	clean_restart $binfile
+
+	if ![runto_main] {
+	    fail "Can't run to main"
+	    return -1
+	}
+
+	delete_breakpoints
+
+	gdb_test_no_output "set dprintf-style $style"
+
+	# Enable always-inserted so we can control the breakpoint
+	# insertion order.
+	gdb_test_no_output "set breakpoint always-inserted on"
+
+	set test "set dprintf"
+	gdb_test_multiple "dprintf $dp_location, \"y=%d\\n\", y" $test {
+	    -re "cannot run dprintf commands.*$gdb_prompt $" {
+		unsupported $test
+	    }
+	    -re "Dprintf .* at .*$gdb_prompt $" {
+		pass $test
+	    }
+	}
+
+	# In case of agent style, this should force the target to
+	# report the trap to GDB.  IOW, GDB should remove the commands
+	# from the target-side breakpoint.
+	gdb_test "break $dp_location" ".*" "set breakpoint"
+
+	gdb_test "continue" "set dprintf here.*"
+    }
+}
+
+test "gdb"
+test "agent"
-- 
1.9.0

[pushed 1/3] gdbserver: on GDB breakpoint reinsertion, also delete the breakpoint's commands.

[Pedro Alves]

If GDB decides to change the breakpoint's conditions or commands,
it'll reinsert the same breakpoint again, with the new options
attached, without deleting the previous breakpoint.  E.g.,

 (gdb) set breakpoint always-inserted on
 (gdb) b main if 0
 Breakpoint 1 at 0x400594: file foo.c, line 21.
 Sending packet: $Z0,400594,1;X3,220027#68...Packet received: OK
 (gdb) b main
 Breakpoint 15 at 0x400594: file foo.c, line 21.
 Sending packet: $Z0,400594,1#49...Packet received: OK

GDBserver understands this and deletes the breakpoint's previous
conditions.  But, it forgets to delete the previous commands.

gdb/gdbserver/
2014-06-02  Pedro Alves  <palves@redhat.com>

	* ax.c (gdb_free_agent_expr): New function.
	* ax.h (gdb_free_agent_expr): New declaration.
	* mem-break.c (delete_gdb_breakpoint_1): Also clear the commands
	list.
	(clear_breakpoint_conditions, clear_breakpoint_commands): Make
	static.
	(clear_breakpoint_conditions_and_commands): New function.
	* mem-break.h (clear_breakpoint_conditions): Delete declaration.
	(clear_breakpoint_conditions_and_commands): New declaration.
---
 gdb/gdbserver/ChangeLog   | 12 ++++++++++++
 gdb/gdbserver/ax.c        | 10 ++++++++++
 gdb/gdbserver/ax.h        |  3 +++
 gdb/gdbserver/mem-break.c | 43 +++++++++++++++++++++++++++++++++++++------
 gdb/gdbserver/mem-break.h |  5 +++--
 gdb/gdbserver/server.c    |  2 +-
 6 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index e591108..0210536 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,15 @@
+2014-06-02  Pedro Alves  <palves@redhat.com>
+
+	* ax.c (gdb_free_agent_expr): New function.
+	* ax.h (gdb_free_agent_expr): New declaration.
+	* mem-break.c (delete_gdb_breakpoint_1): Also clear the commands
+	list.
+	(clear_breakpoint_conditions, clear_breakpoint_commands): Make
+	static.
+	(clear_breakpoint_conditions_and_commands): New function.
+	* mem-break.h (clear_breakpoint_conditions): Delete declaration.
+	(clear_breakpoint_conditions_and_commands): New declaration.
+
 2014-05-23  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>
 
 	* linux-aarch64-low.c (asm/ptrace.h): Include.
diff --git a/gdb/gdbserver/ax.c b/gdb/gdbserver/ax.c
index ef659dc..8b28c72 100644
--- a/gdb/gdbserver/ax.c
+++ b/gdb/gdbserver/ax.c
@@ -110,6 +110,16 @@ gdb_parse_agent_expr (char **actparm)
   return aexpr;
 }
 
+void
+gdb_free_agent_expr (struct agent_expr *aexpr)
+{
+  if (aexpr != NULL)
+    {
+      free (aexpr->bytes);
+      free (aexpr);
+    }
+}
+
 /* Convert the bytes of an agent expression back into hex digits, so
    they can be printed or uploaded.  This allocates the buffer,
    callers should free when they are done with it.  */
diff --git a/gdb/gdbserver/ax.h b/gdb/gdbserver/ax.h
index ce4ff4f..6318004 100644
--- a/gdb/gdbserver/ax.h
+++ b/gdb/gdbserver/ax.h
@@ -58,6 +58,9 @@ struct agent_expr
    of bytes in expression, a comma, and then the bytes.  */
 struct agent_expr *gdb_parse_agent_expr (char **actparm);
 
+/* Release an agent expression.  */
+void gdb_free_agent_expr (struct agent_expr *aexpr);
+
 /* Convert the bytes of an agent expression back into hex digits, so
    they can be printed or uploaded.  This allocates the buffer,
    callers should free when they are done with it.  */
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index daeb521..71876f7 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -1045,9 +1045,9 @@ delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size)
   if (bp == NULL)
     return -1;
 
-  /* Before deleting the breakpoint, make sure to free
-     its condition list.  */
-  clear_breakpoint_conditions (bp);
+  /* Before deleting the breakpoint, make sure to free its condition
+     and command lists.  */
+  clear_breakpoint_conditions_and_commands (bp);
   err = delete_breakpoint (bp);
   if (err != 0)
     return -1;
@@ -1087,7 +1087,7 @@ delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int size)
 
 /* Clear all conditions associated with a breakpoint.  */
 
-void
+static void
 clear_breakpoint_conditions (struct breakpoint *bp)
 {
   struct point_cond_list *cond;
@@ -1102,8 +1102,7 @@ clear_breakpoint_conditions (struct breakpoint *bp)
       struct point_cond_list *cond_next;
 
       cond_next = cond->next;
-      free (cond->cond->bytes);
-      free (cond->cond);
+      gdb_free_agent_expr (cond->cond);
       free (cond);
       cond = cond_next;
     }
@@ -1111,6 +1110,38 @@ clear_breakpoint_conditions (struct breakpoint *bp)
   bp->cond_list = NULL;
 }
 
+/* Clear all commands associated with a breakpoint.  */
+
+static void
+clear_breakpoint_commands (struct breakpoint *bp)
+{
+  struct point_command_list *cmd;
+
+  if (bp->command_list == NULL)
+    return;
+
+  cmd = bp->command_list;
+
+  while (cmd != NULL)
+    {
+      struct point_command_list *cmd_next;
+
+      cmd_next = cmd->next;
+      gdb_free_agent_expr (cmd->cmd);
+      free (cmd);
+      cmd = cmd_next;
+    }
+
+  bp->command_list = NULL;
+}
+
+void
+clear_breakpoint_conditions_and_commands (struct breakpoint *bp)
+{
+  clear_breakpoint_conditions (bp);
+  clear_breakpoint_commands (bp);
+}
+
 /* Add condition CONDITION to GDBserver's breakpoint BP.  */
 
 static void
diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index 2649462..c84c688 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -90,9 +90,10 @@ int breakpoint_here (CORE_ADDR addr);
 
 int breakpoint_inserted_here (CORE_ADDR addr);
 
-/* Clear all breakpoint conditions associated with this address.  */
+/* Clear all breakpoint conditions and commands associated with a
+   breakpoint.  */
 
-void clear_breakpoint_conditions (struct breakpoint *bp);
+void clear_breakpoint_conditions_and_commands (struct breakpoint *bp);
 
 /* Set target-side condition CONDITION to the breakpoint at ADDR.
    Returns false on failure.  On success, advances CONDITION pointer
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index ebc7735..cea56c1 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -3746,7 +3746,7 @@ process_serial_event (void)
 		   here.  If we already have a list of parameters, GDB
 		   is telling us to drop that list and use this one
 		   instead.  */
-		clear_breakpoint_conditions (bp);
+		clear_breakpoint_conditions_and_commands (bp);
 		process_point_options (bp, &dataptr);
 	      }
 	  }
-- 
1.9.0

[pushed 0/3] dprintf and other simultaneous breakpoints/events - don't lose control

[Pedro Alves]

While reviewing / working on Joel's patch for PR 17000
(https://sourceware.org/ml/gdb-patches/2014-05/msg00761.html), I
played with stepping over a dprintf with software single-stepping.
Fixing that made me re-stumble on the issue that when a _regular_
breakpoint is set at the same address as a dprintf, GDB also loses
control.  There's a wider issue to handle here (PR 15180), which I
know Hui has a proposed fix, but while that will take a while to
digest and see through to mainline, and with 7.8 around the corner,
it's best if GDB at least doesn't lose control of the inferior.  In
any case, I think that even with the proposed fix for 15180, we'll
still need this against un-fixed gdbservers.

Tested on x86_64 Fedora 20, native and gdbserver.

Pedro Alves (3):
  gdbserver: on GDB breakpoint reinsertion, also delete the breakpoint's
    commands.
  dprintf-style agent can't explain a trap.
  Installing a breakpoint on top of a dprintf makes GDB lose control.

 gdb/ChangeLog                                   | 11 +++++
 gdb/gdbserver/ChangeLog                         | 12 +++++
 gdb/testsuite/ChangeLog                         |  5 ++
 gdb/breakpoint.c                                | 38 ++++++++++++--
 gdb/gdbserver/ax.c                              | 10 ++++
 gdb/gdbserver/ax.h                              |  3 ++
 gdb/gdbserver/mem-break.c                       | 43 +++++++++++++---
 gdb/gdbserver/mem-break.h                       |  5 +-
 gdb/gdbserver/server.c                          |  2 +-
 gdb/testsuite/gdb.base/dprintf-bp-same-addr.c   | 28 +++++++++++
 gdb/testsuite/gdb.base/dprintf-bp-same-addr.exp | 66 +++++++++++++++++++++++++
 11 files changed, 210 insertions(+), 13 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/dprintf-bp-same-addr.c
 create mode 100644 gdb/testsuite/gdb.base/dprintf-bp-same-addr.exp

-- 
1.9.0