Re: [RFC/WIP PATCH 02/14] Mask software breakpoints from memory writes too

[Pedro Alves <pedro at codesourcery dot com>]

On Monday 28 November 2011 15:39:04, Pedro Alves wrote:

> Now that I've written this, I wonder if it's safe against a remote
> target that implements software breakpoints itself...

Duh, it is.  Memory breakpoints don't get shadows (on gdb) in that case.

I've now committed this updated version, which remembers to update
the breakpoints' shadows on writes this time (duh2!), and also adds
new simple tests that don't rely on displaced stepping.

2011-12-06  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* breakpoint.c (breakpoint_restore_shadows): Rename to ...
	(breakpoint_xfer_memory): ... this.  Change prototype.  Handle
	memory writes too.
	* breakpoint.h (breakpoint_restore_shadows): Delete.
	(breakpoint_xfer_memory): Declare.
	* mem-break.c (default_memory_insert_breakpoint)
	(default_memory_remove_breakpoint): Use target_write_raw_memory.
	(memory_xfer_partial): Rename to ...
	(memory_xfer_partial_1): ... this.  Don't mask out breakpoints
	here.
	(memory_xfer_partial): New.
	(target_write_raw_memory): New.
	* target.h (target_write_raw_memory): New.

	gdb/testsuite/
	* gdb.base/break-always.exp: Test changing memory at addresses
	with breakpoints inserted.

---
 gdb/breakpoint.c                        |   32 +++++++++++-
 gdb/breakpoint.h                        |   15 ++++--
 gdb/mem-break.c                         |    8 ++-
 gdb/target.c                            |   80 +++++++++++++++++++++++++------
 gdb/target.h                            |    3 +
 gdb/testsuite/gdb.base/break-always.exp |   37 ++++++++++++++
 6 files changed, 148 insertions(+), 27 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index bd0a0e3..d9d5bbe 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1049,7 +1049,9 @@ bp_location_has_shadow (struct bp_location *bl)
      bl->address + bp_location_shadow_len_after_address_max <= memaddr  */
 
 void
-breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
+breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
+			const gdb_byte *writebuf_org,
+			ULONGEST memaddr, LONGEST len)
 {
   /* Left boundary, right boundary and median element of our binary
      search.  */
@@ -1161,8 +1163,32 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
 	bp_size -= (bp_addr + bp_size) - (memaddr + len);
       }
 
-    memcpy (buf + bp_addr - memaddr,
-	    bl->target_info.shadow_contents + bptoffset, bp_size);
+    if (readbuf != NULL)
+      {
+	/* Update the read buffer with this inserted breakpoint's
+	   shadow.  */
+	memcpy (readbuf + bp_addr - memaddr,
+		bl->target_info.shadow_contents + bptoffset, bp_size);
+      }
+    else
+      {
+	struct gdbarch *gdbarch = bl->gdbarch;
+	const unsigned char *bp;
+	CORE_ADDR placed_address = bl->target_info.placed_address;
+	unsigned placed_size = bl->target_info.placed_size;
+
+	/* Update the shadow with what we want to write to memory.  */
+	memcpy (bl->target_info.shadow_contents + bptoffset,
+		writebuf_org + bp_addr - memaddr, bp_size);
+
+	/* Determine appropriate breakpoint contents and size for this
+	   address.  */
+	bp = gdbarch_breakpoint_from_pc (gdbarch, &placed_address, &placed_size);
+
+	/* Update the final write buffer with this inserted
+	   breakpoint's INSN.  */
+	memcpy (writebuf + bp_addr - memaddr, bp + bptoffset, bp_size);
+      }
   }
 }
 
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index ead3930..ddf1881 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1296,10 +1296,17 @@ extern int deprecated_remove_raw_breakpoint (struct gdbarch *, void *);
    target.  */
 int watchpoints_triggered (struct target_waitstatus *);
 
-/* Update BUF, which is LEN bytes read from the target address MEMADDR,
-   by replacing any memory breakpoints with their shadowed contents.  */
-void breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, 
-				 LONGEST len);
+/* Helper for transparent breakpoint hiding for memory read and write
+   routines.
+
+   Update one of READBUF or WRITEBUF with either the shadows
+   (READBUF), or the breakpoint instructions (WRITEBUF) of inserted
+   breakpoints at the memory range defined by MEMADDR and extending
+   for LEN bytes.  If writing, then WRITEBUF is a copy of WRITEBUF_ORG
+   on entry.*/
+extern void breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
+				    const gdb_byte *writebuf_org,
+				    ULONGEST memaddr, LONGEST len);
 
 extern int breakpoints_always_inserted_mode (void);
 
diff --git a/gdb/mem-break.c b/gdb/mem-break.c
index ba7dc24..31ca45c 100644
--- a/gdb/mem-break.c
+++ b/gdb/mem-break.c
@@ -60,8 +60,8 @@ default_memory_insert_breakpoint (struct gdbarch *gdbarch,
 
   /* Write the breakpoint.  */
   if (val == 0)
-    val = target_write_memory (bp_tgt->placed_address, bp,
-			       bp_tgt->placed_size);
+    val = target_write_raw_memory (bp_tgt->placed_address, bp,
+				   bp_tgt->placed_size);
 
   return val;
 }
@@ -71,8 +71,8 @@ int
 default_memory_remove_breakpoint (struct gdbarch *gdbarch,
 				  struct bp_target_info *bp_tgt)
 {
-  return target_write_memory (bp_tgt->placed_address, bp_tgt->shadow_contents,
-			      bp_tgt->placed_size);
+  return target_write_raw_memory (bp_tgt->placed_address, bp_tgt->shadow_contents,
+				  bp_tgt->placed_size);
 }
 
 
diff --git a/gdb/target.c b/gdb/target.c
index 6358b00..5700066 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1388,19 +1388,15 @@ memory_xfer_live_readonly_partial (struct target_ops *ops,
    For docs see target.h, to_xfer_partial.  */
 
 static LONGEST
-memory_xfer_partial (struct target_ops *ops, enum target_object object,
-		     void *readbuf, const void *writebuf, ULONGEST memaddr,
-		     LONGEST len)
+memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
+		       void *readbuf, const void *writebuf, ULONGEST memaddr,
+		       LONGEST len)
 {
   LONGEST res;
   int reg_len;
   struct mem_region *region;
   struct inferior *inf;
 
-  /* Zero length requests are ok and require no work.  */
-  if (len == 0)
-    return 0;
-
   /* For accesses to unmapped overlay sections, read directly from
      files.  Must do this first, as MEMADDR may need adjustment.  */
   if (readbuf != NULL && overlay_debugging)
@@ -1551,11 +1547,7 @@ memory_xfer_partial (struct target_ops *ops, enum target_object object,
       if (res <= 0)
 	return -1;
       else
-	{
-	  if (readbuf && !show_memory_breakpoints)
-	    breakpoint_restore_shadows (readbuf, memaddr, reg_len);
-	  return res;
-	}
+	return res;
     }
 
   /* If none of those methods found the memory we wanted, fall back
@@ -1584,9 +1576,6 @@ memory_xfer_partial (struct target_ops *ops, enum target_object object,
     }
   while (ops != NULL);
 
-  if (res > 0 && readbuf != NULL && !show_memory_breakpoints)
-    breakpoint_restore_shadows (readbuf, memaddr, reg_len);
-
   /* Make sure the cache gets updated no matter what - if we are writing
      to the stack.  Even if this write is not tagged as such, we still need
      to update the cache.  */
@@ -1606,6 +1595,48 @@ memory_xfer_partial (struct target_ops *ops, enum target_object object,
   return res;
 }
 
+/* Perform a partial memory transfer.  For docs see target.h,
+   to_xfer_partial.  */
+
+static LONGEST
+memory_xfer_partial (struct target_ops *ops, enum target_object object,
+		     void *readbuf, const void *writebuf, ULONGEST memaddr,
+		     LONGEST len)
+{
+  int res;
+
+  /* Zero length requests are ok and require no work.  */
+  if (len == 0)
+    return 0;
+
+  /* Fill in READBUF with breakpoint shadows, or WRITEBUF with
+     breakpoint insns, thus hiding out from higher layers whether
+     there are software breakpoints inserted in the code stream.  */
+  if (readbuf != NULL)
+    {
+      res = memory_xfer_partial_1 (ops, object, readbuf, NULL, memaddr, len);
+
+      if (res > 0 && !show_memory_breakpoints)
+	breakpoint_xfer_memory (readbuf, NULL, NULL, memaddr, res);
+    }
+  else
+    {
+      void *buf;
+      struct cleanup *old_chain;
+
+      buf = xmalloc (len);
+      old_chain = make_cleanup (xfree, buf);
+      memcpy (buf, writebuf, len);
+
+      breakpoint_xfer_memory (NULL, buf, writebuf, memaddr, len);
+      res = memory_xfer_partial_1 (ops, object, NULL, buf, memaddr, len);
+
+      do_cleanups (old_chain);
+    }
+
+  return res;
+}
+
 static void
 restore_show_memory_breakpoints (void *arg)
 {
@@ -1761,6 +1792,25 @@ target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, int len)
     return EIO;
 }
 
+/* Write LEN bytes from MYADDR to target raw memory at address
+   MEMADDR.  Returns either 0 for success or an errno value if any
+   error occurs.  If an error occurs, no guarantee is made about how
+   much data got written.  Callers that can deal with partial writes
+   should call target_write.  */
+
+int
+target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, int len)
+{
+  /* Dispatch to the topmost target, not the flattened current_target.
+     Memory accesses check target->to_has_(all_)memory, and the
+     flattened target doesn't inherit those.  */
+  if (target_write (current_target.beneath, TARGET_OBJECT_RAW_MEMORY, NULL,
+		    myaddr, memaddr, len) == len)
+    return 0;
+  else
+    return EIO;
+}
+
 /* Fetch the target's memory map.  */
 
 VEC(mem_region_s) *
diff --git a/gdb/target.h b/gdb/target.h
index 1261d90..fd488d6 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -941,6 +941,9 @@ extern int target_read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, int len);
 extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 				int len);
 
+extern int target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
+				    int len);
+
 /* Fetches the target's memory map.  If one is found it is sorted
    and returned, after some consistency checking.  Otherwise, NULL
    is returned.  */
diff --git a/gdb/testsuite/gdb.base/break-always.exp b/gdb/testsuite/gdb.base/break-always.exp
index ce76af7..e20794e 100644
--- a/gdb/testsuite/gdb.base/break-always.exp
+++ b/gdb/testsuite/gdb.base/break-always.exp
@@ -49,7 +49,42 @@ gdb_test_no_output "enable 2" "enable 2.H"
 gdb_test_no_output "disable 2" "disable 2.I"
 gdb_test "info breakpoints" "keep n.*keep n.*keep y.*keep n.*keep n.*" "before re-enable check breakpoint state"
 gdb_test_no_output "enable" "re-enable all breakpoints"
-gdb_continue_to_breakpoint "bar" ".*break-always.c:$bar_location.*"
 
+set bp_address 0
+set test "set breakpoint on bar 2"
+gdb_test_multiple "break bar" $test {
+    -re "Breakpoint 6 at ($hex).*$gdb_prompt $" {
+	set bp_address $expect_out(1,string)
+	pass $test
+    }
+}
+
+# Save the original INSN under the breakpoint.
+gdb_test "p /x \$shadow = *(char *) $bp_address" \
+    " = $hex" \
+    "save shadow"
 
+# Overwrite memory where the breakpoint is planted.  GDB should update
+# its memory breakpoint's shadows, to account for the new contents,
+# and still leave the breakpoint insn planted.  Try twice with
+# different values, in case we happen to be writting exactly what was
+# there already.
+gdb_test "p /x *(char *) $bp_address = 0" \
+    " = 0x0" \
+    "write 0 to breakpoint's address"
+gdb_test "p /x *(char *) $bp_address" \
+    " = 0x0" \
+    "read back 0 from the breakpoint's address"
 
+gdb_test "p /x *(char *) $bp_address = 1" \
+    " = 0x1" \
+    "write 1 to breakpoint's address"
+gdb_test "p /x *(char *) $bp_address" \
+    " = 0x1" \
+    "read back 1 from the breakpoint's address"
+
+# Restore the original contents.
+gdb_test "p /x *(char *) $bp_address = \$shadow" ""
+
+# Run to breakpoint.
+gdb_continue_to_breakpoint "bar" ".*break-always.c:$bar_location.*"