[PATCH] Avoid software breakpoint's instruction shadow inconsistency

[PATCH] Avoid software breakpoint's instruction shadow inconsistency

[Maciej W. Rozycki]

Hi,

 This change:

commit b775012e845380ed4c7421a1b87caf7bfae39f5f
Author: Luis Machado <luisgpm@br.ibm.com>
Date:   Fri Feb 24 15:10:59 2012 +0000

    2012-02-24  Luis Machado  <lgustavo@codesourcery.com>

    	* remote.c (remote_supports_cond_breakpoints): New forward
    	declaration.
[...]

changed the way breakpoints are inserted and removed such that 
`insert_bp_location' can now be called with the breakpoint being handled 
already in place, while previously the call was only ever made for 
breakpoints that have not been put in place.  This in turn caused an issue 
for software breakpoints and targets for which a breakpoint's 
`placed_address' may not be the same as the original requested address.

 The issue is `insert_bp_location' overwrites the previously adjusted 
value in `placed_address' with the original address, that is only replaced 
back with the correct adjusted address later on when 
`gdbarch_breakpoint_from_pc' is called.  Meanwhile there's a window where 
the value in `placed_address' does not correspond to data stored in 
`shadow_contents', leading to incorrect instruction bytes being supplied 
when `one_breakpoint_xfer_memory' is called to supply the instruction 
overlaid by the breakpoint.

 And this is exactly what happens on the MIPS target with software 
breakpoints placed in microMIPS code.  There not only `placed_address' is 
not the original address because of the ISA bit, but also 
`mips_breakpoint_from_pc' has to read the original instruction to 
determine which one of the two software breakpoint instruction encodings 
to choose.  The 16-bit encoding is used to replace 16-bit instructions and 
similarly the 32-bit one is used with 32-bit instructions, to satisfy 
branch delay slot size requirements.

 The mismatch between `placed_address' and the address data in 
`shadow_contents' has been obtained from leads to the wrong encoding being 
used in some cases, which in the case of a 32-bit software breakpoint 
instruction replacing a 16-bit instruction causes corruption to the 
adjacent following instruction and leads the debug session astray if 
execution reaches there e.g. with a jump.

 To address this problem I propose the change below, that adds a 
`reqstd_address' field to `struct bp_target_info' and leaves 
`placed_address' unchanged once it has been set.  This ensures data in 
`shadow_contents' is always consistent with `placed_address'.

 This approach also has this good side effect that all the places that 
examine the breakpoint's address see a consistent value, either 
`reqstd_address' or `placed_address', as required.  Currently some places 
see either the original or the adjusted address in `placed_address', 
depending on whether they have been called before 
`gdbarch_remote_breakpoint_from_pc' or afterwards.  This is in particular 
true for subsequent calls to `gdbarch_remote_breakpoint_from_pc' itself, 
e.g. from `one_breakpoint_xfer_memory'.  This is also important for places 
like `find_single_step_breakpoint' where a breakpoint's address is 
compared to the raw value of $pc.

 I have regression tested it with the mips-linux-gnu target and the 
following multilibs:

-EB
-EB -msoft-float
-EB -mips16
-EB -mips16 -msoft-float
-EB -mmicromips
-EB -mmicromips -msoft-float
-EB -mabi=n32
-EB -mabi=n32 -msoft-float
-EB -mabi=64
-EB -mabi=64 -msoft-float

and the -EL variants of same with no regressions and removing ~900 
seemingly random failures for each of the little-endian microMIPS 
multilibs.  Obviously in the big-endian case the corrupt `placed_address' 
vs `shadow_contents' state did not cause the wrong breakpoint instruction 
to have been chosen.

 FAOD: I chose `reqstd_address' rather than `requested_address' to avoid 
having to wrap overlong lines, the name of the new member has the same 
length as `placed_address' and I think is as clear with some of its vowels 
removed as it would be with them retained.

2014-09-23  Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/
	* breakpoint.h (bp_target_info): Add `reqstd_address' member,
	update comments.
	* breakpoint.c (one_breakpoint_xfer_memory): Use `reqstd_address' 
	for the breakpoint's address.  Don't preinitialize `placed_size'.
	(insert_bp_location): Set `reqstd_address' rather than 
	`placed_address'.
	(bp_target_info_copy_insertion_state): Also copy `placed_address'.
	(bkpt_insert_location): Use `reqstd_address' for the breakpoint's 
	address.
	(bkpt_remove_location): Likewise.
	(deprecated_insert_raw_breakpoint): Likewise.
	(deprecated_remove_raw_breakpoint): Likewise.
	(find_single_step_breakpoint): Likewise.
	* mem-break.c (default_memory_insert_breakpoint): Use 
	`reqstd_address' for the breakpoint's address.  Don't set 
	`placed_address' or `placed_size' if breakpoint contents couldn't
	have been determined.
	* remote.c (remote_insert_breakpoint): Use `reqstd_address' for 
	the breakpoint's address.
	(remote_insert_hw_breakpoint): Likewise.  Don't set 
	`placed_address' or `placed_size' if breakpoint couldn't have been 
	set.
	* aarch64-linux-nat.c (aarch64_linux_insert_hw_breakpoint): Use
	`reqstd_address' for the breakpoint's address.
	* arm-linux-nat.c (arm_linux_hw_breakpoint_initialize): Likewise.
	* ia64-tdep.c (ia64_memory_insert_breakpoint): Likewise.
	* m32r-tdep.c (m32r_memory_insert_breakpoint): Likewise.
	* microblaze-linux-tdep.c 
	(microblaze_linux_memory_remove_breakpoint): Likewise.
	* monitor.c (monitor_insert_breakpoint): Likewise.
	* nto-procfs.c (procfs_insert_breakpoint): Likewise.
	(procfs_insert_hw_breakpoint): Likewise.
	* ppc-linux-nat.c (ppc_linux_insert_hw_breakpoint): Likewise.
	* ppc-linux-tdep.c (ppc_linux_memory_remove_breakpoint): Likewise.
	* remote-m32r-sdi.c (m32r_insert_breakpoint): Likewise.
	* remote-mips.c (mips_insert_breakpoint): Likewise.
	* x86-nat.c (x86_insert_hw_breakpoint): Likewise.

  Maciej

gdb-bp-update-inserted.diff
Index: gdb-fsf-trunk-quilt/gdb/aarch64-linux-nat.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/aarch64-linux-nat.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/aarch64-linux-nat.c	2014-09-18 05:45:20.737876353 +0100
@@ -1183,7 +1183,7 @@ aarch64_handle_breakpoint (int type, COR
     return aarch64_dr_state_remove_one_point (state, type, addr, len);
 }
 
-/* Insert a hardware-assisted breakpoint at BP_TGT->placed_address.
+/* Insert a hardware-assisted breakpoint at BP_TGT->reqstd_address.
    Return 0 on success, -1 on failure.  */
 
 static int
@@ -1192,7 +1192,7 @@ aarch64_linux_insert_hw_breakpoint (stru
 				    struct bp_target_info *bp_tgt)
 {
   int ret;
-  CORE_ADDR addr = bp_tgt->placed_address;
+  CORE_ADDR addr = bp_tgt->placed_address = bp_tgt->reqstd_address;
   const int len = 4;
   const int type = hw_execute;
 
Index: gdb-fsf-trunk-quilt/gdb/arm-linux-nat.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/arm-linux-nat.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/arm-linux-nat.c	2014-09-18 05:45:20.737876353 +0100
@@ -975,7 +975,7 @@ arm_linux_hw_breakpoint_initialize (stru
 				    struct arm_linux_hw_breakpoint *p)
 {
   unsigned mask;
-  CORE_ADDR address = bp_tgt->placed_address;
+  CORE_ADDR address = bp_tgt->placed_address = bp_tgt->reqstd_address;
 
   /* We have to create a mask for the control register which says which bits
      of the word pointed to by address to break on.  */
Index: gdb-fsf-trunk-quilt/gdb/breakpoint.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/breakpoint.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/breakpoint.c	2014-09-18 05:45:20.737876353 +0100
@@ -1495,8 +1495,8 @@ one_breakpoint_xfer_memory (gdb_byte *re
   else
     {
       const unsigned char *bp;
-      CORE_ADDR placed_address = target_info->placed_address;
-      int placed_size = target_info->placed_size;
+      CORE_ADDR addr = target_info->reqstd_address;
+      int placed_size;
 
       /* Update the shadow with what we want to write to memory.  */
       memcpy (target_info->shadow_contents + bptoffset,
@@ -1504,7 +1504,7 @@ one_breakpoint_xfer_memory (gdb_byte *re
 
       /* Determine appropriate breakpoint contents and size for this
 	 address.  */
-      bp = gdbarch_breakpoint_from_pc (gdbarch, &placed_address, &placed_size);
+      bp = gdbarch_breakpoint_from_pc (gdbarch, &addr, &placed_size);
 
       /* Update the final write buffer with this inserted
 	 breakpoint's INSN.  */
@@ -2543,7 +2543,7 @@ insert_bp_location (struct bp_location *
      we have a breakpoint inserted at that address and thus
      read the breakpoint instead of returning the data saved in
      the breakpoint location's shadow contents.  */
-  bl->target_info.placed_address = bl->address;
+  bl->target_info.reqstd_address = bl->address;
   bl->target_info.placed_address_space = bl->pspace->aspace;
   bl->target_info.length = bl->length;
 
@@ -2584,7 +2584,7 @@ insert_bp_location (struct bp_location *
 	     program, but it's not going to work anyway with current
 	     gdb.  */
 	  struct mem_region *mr 
-	    = lookup_mem_region (bl->target_info.placed_address);
+	    = lookup_mem_region (bl->target_info.reqstd_address);
 	  
 	  if (mr)
 	    {
@@ -2658,7 +2658,7 @@ insert_bp_location (struct bp_location *
 							     bl->section);
 		  /* Set a software (trap) breakpoint at the LMA.  */
 		  bl->overlay_target_info = bl->target_info;
-		  bl->overlay_target_info.placed_address = addr;
+		  bl->overlay_target_info.reqstd_address = addr;
 
 		  /* No overlay handling: just set the breakpoint.  */
 		  TRY_CATCH (e, RETURN_MASK_ALL)
@@ -13242,6 +13242,7 @@ bp_target_info_copy_insertion_state (str
 {
   dest->shadow_len = src->shadow_len;
   memcpy (dest->shadow_contents, src->shadow_contents, src->shadow_len);
+  dest->placed_address = src->placed_address;
   dest->placed_size = src->placed_size;
 }
 
@@ -13260,7 +13261,7 @@ bkpt_insert_location (struct bp_location
       /* There is no need to insert a breakpoint if an unconditional
 	 raw/sss breakpoint is already inserted at that location.  */
       sss_slot = find_single_step_breakpoint (bp_tgt->placed_address_space,
-					      bp_tgt->placed_address);
+					      bp_tgt->reqstd_address);
       if (sss_slot >= 0)
 	{
 	  struct bp_target_info *sss_bp_tgt = single_step_breakpoints[sss_slot];
@@ -13282,7 +13283,7 @@ bkpt_remove_location (struct bp_location
     {
       struct bp_target_info *bp_tgt = &bl->target_info;
       struct address_space *aspace = bp_tgt->placed_address_space;
-      CORE_ADDR address = bp_tgt->placed_address;
+      CORE_ADDR address = bp_tgt->reqstd_address;
 
       /* Only remove the breakpoint if there is no raw/sss breakpoint
 	 still inserted at this location.  Otherwise, we would be
@@ -15305,7 +15306,7 @@ deprecated_insert_raw_breakpoint (struct
   bp_tgt = XCNEW (struct bp_target_info);
 
   bp_tgt->placed_address_space = aspace;
-  bp_tgt->placed_address = pc;
+  bp_tgt->reqstd_address = pc;
 
   /* If an unconditional non-raw breakpoint is already inserted at
      that location, there's no need to insert another.  However, with
@@ -15342,7 +15343,7 @@ deprecated_remove_raw_breakpoint (struct
 {
   struct bp_target_info *bp_tgt = bp;
   struct address_space *aspace = bp_tgt->placed_address_space;
-  CORE_ADDR address = bp_tgt->placed_address;
+  CORE_ADDR address = bp_tgt->reqstd_address;
   struct bp_location *bl;
   int ret;
 
@@ -15484,7 +15485,7 @@ find_single_step_breakpoint (struct addr
       struct bp_target_info *bp_tgt = single_step_breakpoints[i];
       if (bp_tgt
 	  && breakpoint_address_match (bp_tgt->placed_address_space,
-				       bp_tgt->placed_address,
+				       bp_tgt->reqstd_address,
 				       aspace, pc))
 	return i;
     }
Index: gdb-fsf-trunk-quilt/gdb/breakpoint.h
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/breakpoint.h	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/breakpoint.h	2014-09-18 05:45:20.737876353 +0100
@@ -235,13 +235,16 @@ struct bp_target_info
   /* Address space at which the breakpoint was placed.  */
   struct address_space *placed_address_space;
 
-  /* Address at which the breakpoint was placed.  This is normally the
-     same as ADDRESS from the bp_location, except when adjustment
-     happens in gdbarch_breakpoint_from_pc.  The most common form of
-     adjustment is stripping an alternate ISA marker from the PC which
-     is used to determine the type of breakpoint to insert.  */
+  /* Address at which the breakpoint was placed.  This is normally
+     the same as REQUESTED_ADDRESS, except when adjustment happens in
+     gdbarch_breakpoint_from_pc.  The most common form of adjustment
+     is stripping an alternate ISA marker from the PC which is used
+     to determine the type of breakpoint to insert.  */
   CORE_ADDR placed_address;
 
+  /* Address at which the breakpoint was requested.  */
+  CORE_ADDR reqstd_address;
+
   /* If this is a ranged breakpoint, then this field contains the
      length of the range that will be watched for execution.  */
   int length;
Index: gdb-fsf-trunk-quilt/gdb/ia64-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/ia64-tdep.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/ia64-tdep.c	2014-09-18 05:45:20.737876353 +0100
@@ -637,7 +637,7 @@ static int
 ia64_memory_insert_breakpoint (struct gdbarch *gdbarch,
 			       struct bp_target_info *bp_tgt)
 {
-  CORE_ADDR addr = bp_tgt->placed_address;
+  CORE_ADDR addr = bp_tgt->placed_address = bp_tgt->reqstd_address;
   gdb_byte bundle[BUNDLE_LEN];
   int slotnum = (int) (addr & 0x0f) / SLOT_MULTIPLIER, shadow_slotnum;
   long long instr_breakpoint;
Index: gdb-fsf-trunk-quilt/gdb/m32r-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/m32r-tdep.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/m32r-tdep.c	2014-09-18 05:45:20.737876353 +0100
@@ -79,7 +79,7 @@ static int
 m32r_memory_insert_breakpoint (struct gdbarch *gdbarch,
 			       struct bp_target_info *bp_tgt)
 {
-  CORE_ADDR addr = bp_tgt->placed_address;
+  CORE_ADDR addr = bp_tgt->placed_address = bp_tgt->reqstd_address;
   int val;
   gdb_byte buf[4];
   gdb_byte contents_cache[4];
Index: gdb-fsf-trunk-quilt/gdb/mem-break.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/mem-break.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/mem-break.c	2014-09-18 05:45:20.737876353 +0100
@@ -37,27 +37,29 @@ int
 default_memory_insert_breakpoint (struct gdbarch *gdbarch,
 				  struct bp_target_info *bp_tgt)
 {
-  int val;
+  CORE_ADDR addr = bp_tgt->reqstd_address;
   const unsigned char *bp;
   gdb_byte *readbuf;
+  int bplen;
+  int val;
 
   /* Determine appropriate breakpoint contents and size for this address.  */
-  bp = gdbarch_breakpoint_from_pc
-       (gdbarch, &bp_tgt->placed_address, &bp_tgt->placed_size);
+  bp = gdbarch_breakpoint_from_pc (gdbarch, &addr, &bplen);
   if (bp == NULL)
     error (_("Software breakpoints not implemented for this target."));
 
+  bp_tgt->placed_address = addr;
+  bp_tgt->placed_size = bplen;
+
   /* Save the memory contents in the shadow_contents buffer and then
      write the breakpoint instruction.  */
-  bp_tgt->shadow_len = bp_tgt->placed_size;
-  readbuf = alloca (bp_tgt->placed_size);
-  val = target_read_memory (bp_tgt->placed_address, readbuf,
-			    bp_tgt->placed_size);
+  bp_tgt->shadow_len = bplen;
+  readbuf = alloca (bplen);
+  val = target_read_memory (addr, readbuf, bplen);
   if (val == 0)
     {
-      memcpy (bp_tgt->shadow_contents, readbuf, bp_tgt->placed_size);
-      val = target_write_raw_memory (bp_tgt->placed_address, bp,
-				     bp_tgt->placed_size);
+      memcpy (bp_tgt->shadow_contents, readbuf, bplen);
+      val = target_write_raw_memory (addr, bp, bplen);
     }
 
   return val;
Index: gdb-fsf-trunk-quilt/gdb/microblaze-linux-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/microblaze-linux-tdep.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/microblaze-linux-tdep.c	2014-09-18 05:45:20.737876353 +0100
@@ -41,7 +41,7 @@ static int
 microblaze_linux_memory_remove_breakpoint (struct gdbarch *gdbarch, 
 					   struct bp_target_info *bp_tgt)
 {
-  CORE_ADDR addr = bp_tgt->placed_address;
+  CORE_ADDR addr = bp_tgt->reqstd_address;
   const gdb_byte *bp;
   int val;
   int bplen;
Index: gdb-fsf-trunk-quilt/gdb/monitor.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/monitor.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/monitor.c	2014-09-18 05:45:20.737876353 +0100
@@ -2103,7 +2103,7 @@ static int
 monitor_insert_breakpoint (struct target_ops *ops, struct gdbarch *gdbarch,
 			   struct bp_target_info *bp_tgt)
 {
-  CORE_ADDR addr = bp_tgt->placed_address;
+  CORE_ADDR addr = bp_tgt->placed_address = bp_tgt->reqstd_address;
   int i;
   int bplen;
 
Index: gdb-fsf-trunk-quilt/gdb/nto-procfs.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/nto-procfs.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/nto-procfs.c	2014-09-18 05:45:20.737876353 +0100
@@ -928,6 +928,7 @@ static int
 procfs_insert_breakpoint (struct target_ops *ops, struct gdbarch *gdbarch,
 			  struct bp_target_info *bp_tgt)
 {
+  bp_tgt->placed_address = bp_tgt->reqstd_address;
   return procfs_breakpoint (bp_tgt->placed_address, _DEBUG_BREAK_EXEC, 0);
 }
 
@@ -942,6 +943,7 @@ static int
 procfs_insert_hw_breakpoint (struct target_ops *self, struct gdbarch *gdbarch,
 			     struct bp_target_info *bp_tgt)
 {
+  bp_tgt->placed_address = bp_tgt->reqstd_address;
   return procfs_breakpoint (bp_tgt->placed_address,
 			    _DEBUG_BREAK_EXEC | _DEBUG_BREAK_HW, 0);
 }
Index: gdb-fsf-trunk-quilt/gdb/ppc-linux-nat.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/ppc-linux-nat.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/ppc-linux-nat.c	2014-09-18 05:45:20.737876353 +0100
@@ -1690,7 +1690,7 @@ ppc_linux_insert_hw_breakpoint (struct t
   p.version = PPC_DEBUG_CURRENT_VERSION;
   p.trigger_type = PPC_BREAKPOINT_TRIGGER_EXECUTE;
   p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE;
-  p.addr = (uint64_t) bp_tgt->placed_address;
+  p.addr = (uint64_t) (bp_tgt->placed_address = bp_tgt->reqstd_address);
   p.condition_value = 0;
 
   if (bp_tgt->length)
Index: gdb-fsf-trunk-quilt/gdb/ppc-linux-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/ppc-linux-tdep.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/ppc-linux-tdep.c	2014-09-18 05:45:20.737876353 +0100
@@ -211,7 +211,7 @@ static int
 ppc_linux_memory_remove_breakpoint (struct gdbarch *gdbarch,
 				    struct bp_target_info *bp_tgt)
 {
-  CORE_ADDR addr = bp_tgt->placed_address;
+  CORE_ADDR addr = bp_tgt->reqstd_address;
   const unsigned char *bp;
   int val;
   int bplen;
Index: gdb-fsf-trunk-quilt/gdb/remote-m32r-sdi.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/remote-m32r-sdi.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/remote-m32r-sdi.c	2014-09-18 05:45:20.737876353 +0100
@@ -1172,7 +1172,7 @@ m32r_insert_breakpoint (struct target_op
 			struct gdbarch *gdbarch,
 			struct bp_target_info *bp_tgt)
 {
-  CORE_ADDR addr = bp_tgt->placed_address;
+  CORE_ADDR addr = bp_tgt->placed_address = bp_tgt->reqstd_address;
   int ib_breakpoints;
   unsigned char buf[13];
   int i, c;
Index: gdb-fsf-trunk-quilt/gdb/remote-mips.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/remote-mips.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/remote-mips.c	2014-09-18 05:45:20.737876353 +0100
@@ -2375,8 +2375,11 @@ mips_insert_breakpoint (struct target_op
 			struct bp_target_info *bp_tgt)
 {
   if (monitor_supports_breakpoints)
-    return mips_set_breakpoint (bp_tgt->placed_address, MIPS_INSN32_SIZE,
-				BREAK_FETCH);
+    {
+      bp_tgt->placed_address = bp_tgt->reqstd_address;
+      return mips_set_breakpoint (bp_tgt->placed_address, MIPS_INSN32_SIZE,
+				  BREAK_FETCH);
+    }
   else
     return memory_insert_breakpoint (ops, gdbarch, bp_tgt);
 }
Index: gdb-fsf-trunk-quilt/gdb/remote.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/remote.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/remote.c	2014-09-18 05:45:20.737876353 +0100
@@ -8079,7 +8079,7 @@ remote_insert_breakpoint (struct target_
 
   if (packet_support (PACKET_Z0) != PACKET_DISABLE)
     {
-      CORE_ADDR addr = bp_tgt->placed_address;
+      CORE_ADDR addr = bp_tgt->reqstd_address;
       struct remote_state *rs;
       char *p, *endbuf;
       int bpsize;
@@ -8351,16 +8351,16 @@ static int
 remote_insert_hw_breakpoint (struct target_ops *self, struct gdbarch *gdbarch,
 			     struct bp_target_info *bp_tgt)
 {
-  CORE_ADDR addr;
+  CORE_ADDR addr = bp_tgt->reqstd_address;
   struct remote_state *rs;
   char *p, *endbuf;
   char *message;
+  int bpsize;
 
   /* The length field should be set to the size of a breakpoint
      instruction, even though we aren't inserting one ourselves.  */
 
-  gdbarch_remote_breakpoint_from_pc
-    (gdbarch, &bp_tgt->placed_address, &bp_tgt->placed_size);
+  gdbarch_remote_breakpoint_from_pc (gdbarch, &addr, &bpsize);
 
   if (packet_support (PACKET_Z1) == PACKET_DISABLE)
     return -1;
@@ -8378,9 +8378,9 @@ remote_insert_hw_breakpoint (struct targ
   *(p++) = '1';
   *(p++) = ',';
 
-  addr = remote_address_masked (bp_tgt->placed_address);
+  addr = remote_address_masked (addr);
   p += hexnumstr (p, (ULONGEST) addr);
-  xsnprintf (p, endbuf - p, ",%x", bp_tgt->placed_size);
+  xsnprintf (p, endbuf - p, ",%x", bpsize);
 
   if (remote_supports_cond_breakpoints (self))
     remote_add_target_side_condition (gdbarch, bp_tgt, p, endbuf);
@@ -8404,6 +8404,8 @@ remote_insert_hw_breakpoint (struct targ
     case PACKET_UNKNOWN:
       return -1;
     case PACKET_OK:
+      bp_tgt->placed_address = addr;
+      bp_tgt->placed_size = bpsize;
       return 0;
     }
   internal_error (__FILE__, __LINE__,
Index: gdb-fsf-trunk-quilt/gdb/x86-nat.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/x86-nat.c	2014-09-18 05:44:50.727603472 +0100
+++ gdb-fsf-trunk-quilt/gdb/x86-nat.c	2014-09-18 05:45:20.737876353 +0100
@@ -211,7 +211,7 @@ x86_stopped_by_watchpoint (struct target
   return x86_dr_stopped_by_watchpoint (state);
 }
 
-/* Insert a hardware-assisted breakpoint at BP_TGT->placed_address.
+/* Insert a hardware-assisted breakpoint at BP_TGT->reqstd_address.
    Return 0 on success, EBUSY on failure.  */
 
 static int
@@ -221,6 +221,7 @@ x86_insert_hw_breakpoint (struct target_
   struct x86_debug_reg_state *state
     = x86_debug_reg_state (ptid_get_pid (inferior_ptid));
 
+  bp_tgt->placed_address = bp_tgt->reqstd_address;
   return x86_dr_insert_watchpoint (state, hw_execute,
 				   bp_tgt->placed_address, 1) ? EBUSY : 0;
 }

Re: [PATCH] Avoid software breakpoint's instruction shadow inconsistency

[Pedro Alves]

On 09/23/2014 06:08 PM, Maciej W. Rozycki wrote:
> Hi,
> 
>  This change:
> 
> commit b775012e845380ed4c7421a1b87caf7bfae39f5f
> Author: Luis Machado <luisgpm@br.ibm.com>
> Date:   Fri Feb 24 15:10:59 2012 +0000
> 
>     2012-02-24  Luis Machado  <lgustavo@codesourcery.com>
> 
>     	* remote.c (remote_supports_cond_breakpoints): New forward
>     	declaration.
> [...]
> 
> changed the way breakpoints are inserted and removed such that 
> `insert_bp_location' can now be called with the breakpoint being handled 
> already in place, while previously the call was only ever made for 
> breakpoints that have not been put in place.  This in turn caused an issue 
> for software breakpoints and targets for which a breakpoint's 
> `placed_address' may not be the same as the original requested address.
> 
>  The issue is `insert_bp_location' overwrites the previously adjusted 
> value in `placed_address' with the original address, that is only replaced 
> back with the correct adjusted address later on when 
> `gdbarch_breakpoint_from_pc' is called.  Meanwhile there's a window where 
> the value in `placed_address' does not correspond to data stored in 
> `shadow_contents', leading to incorrect instruction bytes being supplied 
> when `one_breakpoint_xfer_memory' is called to supply the instruction 
> overlaid by the breakpoint.
> 
>  And this is exactly what happens on the MIPS target with software 
> breakpoints placed in microMIPS code.  There not only `placed_address' is 
> not the original address because of the ISA bit, but also 
> `mips_breakpoint_from_pc' has to read the original instruction to 
> determine which one of the two software breakpoint instruction encodings 
> to choose.  The 16-bit encoding is used to replace 16-bit instructions and 
> similarly the 32-bit one is used with 32-bit instructions, to satisfy 
> branch delay slot size requirements.
> 
>  The mismatch between `placed_address' and the address data in 
> `shadow_contents' has been obtained from leads to the wrong encoding being 
> used in some cases, which in the case of a 32-bit software breakpoint 
> instruction replacing a 16-bit instruction causes corruption to the 
> adjacent following instruction and leads the debug session astray if 
> execution reaches there e.g. with a jump.
> 
>  To address this problem I propose the change below, that adds a 
> `reqstd_address' field to `struct bp_target_info' and leaves 
> `placed_address' unchanged once it has been set.  This ensures data in 
> `shadow_contents' is always consistent with `placed_address'.
> 
>  This approach also has this good side effect that all the places that 
> examine the breakpoint's address see a consistent value, either 
> `reqstd_address' or `placed_address', as required.  Currently some places 
> see either the original or the adjusted address in `placed_address', 
> depending on whether they have been called before 
> `gdbarch_remote_breakpoint_from_pc' or afterwards.  This is in particular 
> true for subsequent calls to `gdbarch_remote_breakpoint_from_pc' itself, 
> e.g. from `one_breakpoint_xfer_memory'.

ITYM gdbarch_breakpoint_from_pc, as there's no call to
gdbarch_remote_breakpoint_from_pc in one_breakpoint_xfer_memory.

It's totally fine to call gdbarch_breakpoint_from_pc on an already
adjusted address.  That method naturally has to be idempotent.  It'll
just return without adjusting anything, as the input address must already
be a fine address to place a breakpoint, otherwise the first call that
actually adjusted the address when the breakpoint was first
inserted wouldn't have returned it in the first place.

Might be worth it to add an assertion to the effect, just for
code clarity.

> This is also important for places
> like `find_single_step_breakpoint' where a breakpoint's address is 
> compared to the raw value of $pc.
> 

AFAICS, insert_single_step_breakpoint also doesn't do
gdbarch_breakpoint_from_pc, so there doesn't seem to be a mismatch.

In any case, find_single_step_breakpoint and insert_single_step_breakpoint
and related deprecated bits are scheduled for deletion
very soon: https://sourceware.org/ml/gdb-patches/2014-09/msg00242.html

Before adding more fields, I'd like to first consider something like this:

static int
insert_bp_location (struct bp_location *bl,
		    struct ui_file *tmp_error_stream,
		    int *disabled_breaks,
		    int *hw_breakpoint_error,
		    int *hw_bp_error_explained_already)
{
  enum errors bp_err = GDB_NO_ERROR;
  const char *bp_err_message = NULL;
  volatile struct gdb_exception e;

  if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
    return 0;

  /* Note we don't initialize bl->target_info, as that wipes out
     the breakpoint location's shadow_contents if the breakpoint
     is still inserted at that location.  This in turn breaks
     target_read_memory which depends on these buffers when
     a memory read is requested at the breakpoint location:
     Once the target_info has been wiped, we fail to see that
     we have a breakpoint inserted at that address and thus
     read the breakpoint instead of returning the data saved in
     the breakpoint location's shadow contents.  */
-  bl->target_info.placed_address = bl->address;
-  bl->target_info.placed_address_space = bl->pspace->aspace;
-  bl->target_info.length = bl->length;
+ if (!bl->inserted)
+ {
+  bl->target_info.placed_address = bl->address;
+  bl->target_info.placed_address_space = bl->pspace->aspace;
+  bl->target_info.length = bl->length;
+ }

Doesn't what work?  Note the comment above already talking about
related concerns.  If we're reinserting a breakpoint to update
its conditions, we're certainly inserting it at the exact
same address, so no need to touch placed_address at all.

Thanks,
Pedro Alves

Re: [PATCH] Avoid software breakpoint's instruction shadow inconsistency

[Maciej W. Rozycki]

On Tue, 23 Sep 2014, Pedro Alves wrote:

> >  This change:
> > 
> > commit b775012e845380ed4c7421a1b87caf7bfae39f5f
> > Author: Luis Machado <luisgpm@br.ibm.com>
> > Date:   Fri Feb 24 15:10:59 2012 +0000
> > 
> >     2012-02-24  Luis Machado  <lgustavo@codesourcery.com>
> > 
> >     	* remote.c (remote_supports_cond_breakpoints): New forward
> >     	declaration.
> > [...]
> > 
> > changed the way breakpoints are inserted and removed such that 
> > `insert_bp_location' can now be called with the breakpoint being handled 
> > already in place, while previously the call was only ever made for 
> > breakpoints that have not been put in place.  This in turn caused an issue 
> > for software breakpoints and targets for which a breakpoint's 
> > `placed_address' may not be the same as the original requested address.
> > 
> >  The issue is `insert_bp_location' overwrites the previously adjusted 
> > value in `placed_address' with the original address, that is only replaced 
> > back with the correct adjusted address later on when 
> > `gdbarch_breakpoint_from_pc' is called.  Meanwhile there's a window where 
> > the value in `placed_address' does not correspond to data stored in 
> > `shadow_contents', leading to incorrect instruction bytes being supplied 
> > when `one_breakpoint_xfer_memory' is called to supply the instruction 
> > overlaid by the breakpoint.
> > 
> >  And this is exactly what happens on the MIPS target with software 
> > breakpoints placed in microMIPS code.  There not only `placed_address' is 
> > not the original address because of the ISA bit, but also 
> > `mips_breakpoint_from_pc' has to read the original instruction to 
> > determine which one of the two software breakpoint instruction encodings 
> > to choose.  The 16-bit encoding is used to replace 16-bit instructions and 
> > similarly the 32-bit one is used with 32-bit instructions, to satisfy 
> > branch delay slot size requirements.
> > 
> >  The mismatch between `placed_address' and the address data in 
> > `shadow_contents' has been obtained from leads to the wrong encoding being 
> > used in some cases, which in the case of a 32-bit software breakpoint 
> > instruction replacing a 16-bit instruction causes corruption to the 
> > adjacent following instruction and leads the debug session astray if 
> > execution reaches there e.g. with a jump.
> > 
> >  To address this problem I propose the change below, that adds a 
> > `reqstd_address' field to `struct bp_target_info' and leaves 
> > `placed_address' unchanged once it has been set.  This ensures data in 
> > `shadow_contents' is always consistent with `placed_address'.
> > 
> >  This approach also has this good side effect that all the places that 
> > examine the breakpoint's address see a consistent value, either 
> > `reqstd_address' or `placed_address', as required.  Currently some places 
> > see either the original or the adjusted address in `placed_address', 
> > depending on whether they have been called before 
> > `gdbarch_remote_breakpoint_from_pc' or afterwards.  This is in particular 
> > true for subsequent calls to `gdbarch_remote_breakpoint_from_pc' itself, 
> > e.g. from `one_breakpoint_xfer_memory'.
> 
> ITYM gdbarch_breakpoint_from_pc, as there's no call to
> gdbarch_remote_breakpoint_from_pc in one_breakpoint_xfer_memory.

 Indeed, a cut & paste typo there, sorry.

> It's totally fine to call gdbarch_breakpoint_from_pc on an already
> adjusted address.  That method naturally has to be idempotent.  It'll
> just return without adjusting anything, as the input address must already
> be a fine address to place a breakpoint, otherwise the first call that
> actually adjusted the address when the breakpoint was first
> inserted wouldn't have returned it in the first place.

 The thing is it can't, because on MIPS targets the ISA bit can be the 
only place where the breakpoint type requirement is encoded -- if you set 
a breakpoint by address in code that has no symbol information, e.g.:

(gdb) break *0x87654321

is not the same as:

(gdb) break *0x87654320

and consequently if there's no symbol information available for either of 
0x87654321 or 0x87654320, then `mips_breakpoint_from_pc' will return a 
microMIPS (or MIPS16, as determined elsewhere) breakpoint for the value of 
0x87654321 in `placed_address' and a standard MIPS breakpoint for the 
value of 0x87654320 there.  So the value of 0x87654321 has to be stored 
somewhere and subsequent calls to `mips_breakpoint_from_pc' have to see it 
again (and convert to 0x87654320 in `placed_address').

 Please note that this is not a theoretical or corner case, because we can 
easily use breakpoints in code with no symbol information (e.g. 
system-installed shared libraries; dynamic symbols associated with 
exported entry points will likely not cover all the code) when 
single-stepping with a software watchpoint enabled.

> Might be worth it to add an assertion to the effect, just for
> code clarity.

 So not an option, sorry.

> > This is also important for places
> > like `find_single_step_breakpoint' where a breakpoint's address is 
> > compared to the raw value of $pc.
> > 
> 
> AFAICS, insert_single_step_breakpoint also doesn't do
> gdbarch_breakpoint_from_pc, so there doesn't seem to be a mismatch.

 The problem is `find_single_step_breakpoint' is called in the ordinary 
breakpoint removal path too, so that if a single-step breakpoint is placed 
at the same address, it is retained.  I saw this place do bad things in 
testing my change before I adjusted it to use `reqstd_address'.

> In any case, find_single_step_breakpoint and insert_single_step_breakpoint
> and related deprecated bits are scheduled for deletion
> very soon: https://sourceware.org/ml/gdb-patches/2014-09/msg00242.html

 The relevant parts of my change can easily be removed if they do not make 
it beforehand.

> Before adding more fields, I'd like to first consider something like this:
> 
> static int
> insert_bp_location (struct bp_location *bl,
> 		    struct ui_file *tmp_error_stream,
> 		    int *disabled_breaks,
> 		    int *hw_breakpoint_error,
> 		    int *hw_bp_error_explained_already)
> {
>   enum errors bp_err = GDB_NO_ERROR;
>   const char *bp_err_message = NULL;
>   volatile struct gdb_exception e;
> 
>   if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
>     return 0;
> 
>   /* Note we don't initialize bl->target_info, as that wipes out
>      the breakpoint location's shadow_contents if the breakpoint
>      is still inserted at that location.  This in turn breaks
>      target_read_memory which depends on these buffers when
>      a memory read is requested at the breakpoint location:
>      Once the target_info has been wiped, we fail to see that
>      we have a breakpoint inserted at that address and thus
>      read the breakpoint instead of returning the data saved in
>      the breakpoint location's shadow contents.  */
> -  bl->target_info.placed_address = bl->address;
> -  bl->target_info.placed_address_space = bl->pspace->aspace;
> -  bl->target_info.length = bl->length;
> + if (!bl->inserted)
> + {
> +  bl->target_info.placed_address = bl->address;
> +  bl->target_info.placed_address_space = bl->pspace->aspace;
> +  bl->target_info.length = bl->length;
> + }
> 
> Doesn't what work?  Note the comment above already talking about
> related concerns.  If we're reinserting a breakpoint to update
> its conditions, we're certainly inserting it at the exact
> same address, so no need to touch placed_address at all.

 That's actually what I tried first (being lazy and trying to save myself 
from extra work) before I realised that wouldn't work due to the issue 
above.

  Maciej

Re: [PATCH] Avoid software breakpoint's instruction shadow inconsistency

[Maciej W. Rozycki]

On Tue, 23 Sep 2014, Maciej W. Rozycki wrote:

> > > This is also important for places
> > > like `find_single_step_breakpoint' where a breakpoint's address is 
> > > compared to the raw value of $pc.
> > > 
> > 
> > AFAICS, insert_single_step_breakpoint also doesn't do
> > gdbarch_breakpoint_from_pc, so there doesn't seem to be a mismatch.
> 
>  The problem is `find_single_step_breakpoint' is called in the ordinary 
> breakpoint removal path too, so that if a single-step breakpoint is placed 
> at the same address, it is retained.  I saw this place do bad things in 
> testing my change before I adjusted it to use `reqstd_address'.

 And this change alone is I believe what is responsible for the removal of 
the following regressions:

FAIL: gdb.base/sss-bp-on-user-bp-2.exp: before/after disassembly matches
FAIL: gdb.base/sss-bp-on-user-bp-2.exp: stepi_del_break

in little-endian MIPS16 multilib testing and this regression:

FAIL: gdb.base/sss-bp-on-user-bp-2.exp: before/after disassembly matches

in big-endian MIPS16 multilib testing with this patch in place.

  Maciej

Re: [PATCH] Avoid software breakpoint's instruction shadow inconsistency

[Pedro Alves]

On 09/23/2014 07:11 PM, Maciej W. Rozycki wrote:
> On Tue, 23 Sep 2014, Pedro Alves wrote:

>> It's totally fine to call gdbarch_breakpoint_from_pc on an already
>> adjusted address.  That method naturally has to be idempotent.  It'll
>> just return without adjusting anything, as the input address must already
>> be a fine address to place a breakpoint, otherwise the first call that
>> actually adjusted the address when the breakpoint was first
>> inserted wouldn't have returned it in the first place.
> 
>  The thing is it can't, because on MIPS targets the ISA bit can be the 
> only place where the breakpoint type requirement is encoded -- if you set 
> a breakpoint by address in code that has no symbol information, e.g.:
> 
> (gdb) break *0x87654321
> 
> is not the same as:
> 
> (gdb) break *0x87654320
> 
> and consequently if there's no symbol information available for either of 
> 0x87654321 or 0x87654320, then `mips_breakpoint_from_pc' will return a 
> microMIPS (or MIPS16, as determined elsewhere) breakpoint for the value of 
> 0x87654321 in `placed_address' and a standard MIPS breakpoint for the 
> value of 0x87654320 there.  So the value of 0x87654321 has to be stored 
> somewhere and subsequent calls to `mips_breakpoint_from_pc' have to see it 
> again (and convert to 0x87654320 in `placed_address').
> 
>  Please note that this is not a theoretical or corner case, because we can 
> easily use breakpoints in code with no symbol information (e.g. 
> system-installed shared libraries; dynamic symbols associated with 
> exported entry points will likely not cover all the code) when 
> single-stepping with a software watchpoint enabled.

OK, missed that.  I was assuming things like mapping symbols sorting
things out.  But MIPS doesn't have those, and they can be stripped
on ARM too, in any case.

>>> This is also important for places
>>> like `find_single_step_breakpoint' where a breakpoint's address is 
>>> compared to the raw value of $pc.
>>>
>>
>> AFAICS, insert_single_step_breakpoint also doesn't do
>> gdbarch_breakpoint_from_pc, so there doesn't seem to be a mismatch.
> 
>  The problem is `find_single_step_breakpoint' is called in the ordinary 
> breakpoint removal path too, so that if a single-step breakpoint is placed 
> at the same address, it is retained.  I saw this place do bad things in 
> testing my change before I adjusted it to use `reqstd_address'.
> 
>> In any case, find_single_step_breakpoint and insert_single_step_breakpoint
>> and related deprecated bits are scheduled for deletion
>> very soon: https://sourceware.org/ml/gdb-patches/2014-09/msg00242.html
> 
>  The relevant parts of my change can easily be removed if they do not make 
> it beforehand.

The thing is that all that code was only added to get 7.8 out of
the door and then do things better post 7.8.

See discussion around: https://sourceware.org/ml/gdb-patches/2014-06/msg00052.html

So not just the single-step bits; ISTM most of the patch ends up
unnecessary.

As soon as breakpoint_xfer_memory doesn't have to care
about single-step breakpoints, one_breakpoint_xfer_memory can
work with the location's bl->address instead of
bl->target_info.placed_address.  bl->address is always the same
as your bl->target_info.reqstd_address, afaics.  The reason
one_breakpoint_xfer_memory can't work with bl->address instead today
is that software single-step breakpoints aren't a location,
only a target_info, so the original address is lost...

Piling on workarounds due to these single-step breakpoint issues
is exactly the sort of thing I'd like to avoid further.

It's of course also completely unnecessary/bogus for GDB to be
reinserting breakpoints when the target doesn't handle
breakpoints itself, but that's another story.

I'm confused on this part of your original description:

>  The issue is `insert_bp_location' overwrites the previously adjusted 
> value in `placed_address' with the original address, that is only replaced 
> back with the correct adjusted address later on when 
> `gdbarch_breakpoint_from_pc' is called.  Meanwhile there's a window where 
> the value in `placed_address' does not correspond to data stored in 
> `shadow_contents', leading to incorrect instruction bytes being supplied 
> when `one_breakpoint_xfer_memory' is called to supply the instruction 
> overlaid by the breakpoint.

It doesn't look like to me that this is really the problem, since
default_memory_insert_breakpoint adjusts bp_tgt->placed_address
before reading memory.


Instead, the issue is that because the breakpoint is supposed to be
inserted (we're re-inserting it), one_breakpoint_xfer_memory needs
to store the breakpoint instruction on top of the memory we're
about to write.  And then one_breakpoint_xfer_memory gets the
breakpoint instruction wrong exactly because it lost the ISA bit.

Although we can now see this more easily since we now reinsert
already inserted breakpoints, we should be able to trigger the
issue even with that, if the user writes to memory at an address
where a breakpoint with the ISA bit was inserted.

Like:

(gdb) set breakpoint always-inserted on
(gdb) p /x *(char*) 0x87654321
$4 = 0x55
(gdb) break *0x87654321
(gdb) p /x *(char*) 0x87654321 = 0x55
$5 = 0x55

At this point, 0x87654321 should still contain the correct
breakpoint insn (on the target), but due to the
one_breakpoint_xfer_memory issue, it won't.


BTW, if instead of:

@@ -2543,7 +2543,7 @@ insert_bp_location (struct bp_location *
      we have a breakpoint inserted at that address and thus
      read the breakpoint instead of returning the data saved in
      the breakpoint location's shadow contents.  */
-  bl->target_info.placed_address = bl->address;
+  bl->target_info.reqstd_address = bl->address;

you did:

   bl->target_info.placed_address = bl->address;
+  bl->target_info.reqstd_address = bl->address;

then it seems to me that most of the hunks that do something
like this:

@@ -975,7 +975,7 @@ arm_linux_hw_breakpoint_initialize (stru
 				    struct arm_linux_hw_breakpoint *p)
 {
   unsigned mask;
-  CORE_ADDR address = bp_tgt->placed_address;
+  CORE_ADDR address = bp_tgt->placed_address = bp_tgt->reqstd_address;


including the default_memory_insert_breakpoint changes,

would be unnecessary.

I could be missing something else, of course.

The patch below is what I'd like to push on top of the software single-step
rework (which I've meanwhile slit and posted here
https://sourceware.org/ml/gdb-patches/2014-09/msg00755.html)

I've pushed that series with this patch on top here, for convenience:

  git@github.com:palves/gdb.git palves/mips_instruction_shadow_inconsistency

Obviously, the mips-linux-gnu testing mentioned in the log is tentative. :-)

-------- 8< --------
From e6e451ac1c15d69966233d07165c917d83e51d29 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 29 Sep 2014 17:24:47 +0100
Subject: [PATCH] [PATCH] Fix memory writes to regions with inserted
 breakpoints

This patch fixes an issue with writing memory to addresses where we
have breakpoints inserted, on architectures for which a breakpoint's
`placed_address' may not be the same as the original requested
address.

This was exposed by this change:

 commit b775012e845380ed4c7421a1b87caf7bfae39f5f
 Author: Luis Machado <luisgpm@br.ibm.com>
 Date:   Fri Feb 24 15:10:59 2012 +0000

     2012-02-24  Luis Machado  <lgustavo@codesourcery.com>

	 * remote.c (remote_supports_cond_breakpoints): New forward
	 declaration.
 [...]

Although the issue triggers just as well if the user manually writes
to memory at an address where such a breakpoint is inserted.

When the target reports support for target-side conditional breakpoint
evaluation, breakpoints are inserted and removed such that
`insert_bp_location' can now be called with the breakpoint being
handled already in place, while with such support the call is only
ever made for breakpoints that have not been put in place.

When writing a block of memory to an address where we have a
breakpoint inserted, we need to make sure that:

#1 - we update the inserted breakpoint's shadow buffers with the new
     memory contents, so that the breakpoint may later be correctly
     uninserted.

#2 - so that the breakpoint remains inserted in target memory, the
     breakpoint instruction is overlaid on top of the memory buffer
     the caller wanted to have writen to the target, and the result is
     what is actually committed to target's memory.

The problem is with #2: one_breakpoint_xfer_memory is passing the
target_info's `placed_address' address to `gdbarch_breakpoint_from_pc'
to determine which breakpoint instruction to overlay, but this address
is not the one that mem-break.c originally used to decide which
breakpoint instruction to use.  See the comment added by the patch for
further details.

Regression tested on the mips-linux-gnu target and the following
multilibs:

 -EB
 -EB -msoft-float
 -EB -mips16
 -EB -mips16 -msoft-float
 -EB -mmicromips
 -EB -mmicromips -msoft-float
 -EB -mabi=n32
 -EB -mabi=n32 -msoft-float
 -EB -mabi=64
 -EB -mabi=64 -msoft-float

and the -EL variants of same with no regressions and removing ~900
seemingly random failures for each of the little-endian microMIPS
multilibs.  Obviously in the big-endian case the bug did not cause the
wrong breakpoint instruction to have been chosen.

gdb/
2014-09-29  Pedro Alves  <palves@redhat.com>
	    Maciej W. Rozycki  <macro@codesourcery.com>

	* breakpoint.c (one_breakpoint_xfer_memory): Take a bl_location
	as parameter instead of a bp_target_info and a gdbarch.  Pass the
	location's address to gdbarch_breakpoint_from_pc instead of the
	target_info's placed address.  Add comment.
	(breakpoint_xfer_memory): Adjust.
---
 gdb/breakpoint.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index dacb867..de7320e 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1471,13 +1471,13 @@ static void
 one_breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
 			    const gdb_byte *writebuf_org,
 			    ULONGEST memaddr, LONGEST len,
-			    struct bp_target_info *target_info,
-			    struct gdbarch *gdbarch)
+			    struct bp_location *bl)
 {
   /* Now do full processing of the found relevant range of elements.  */
   CORE_ADDR bp_addr = 0;
   int bp_size = 0;
   int bptoffset = 0;
+  struct bp_target_info *target_info = &bl->target_info;
 
   if (!breakpoint_address_match (target_info->placed_address_space, 0,
 				 current_program_space->aspace, 0))
@@ -1536,16 +1536,35 @@ one_breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
   else
     {
       const unsigned char *bp;
-      CORE_ADDR placed_address = target_info->placed_address;
-      int placed_size = target_info->placed_size;
+      CORE_ADDR address;
+      int placed_size;
 
       /* Update the shadow with what we want to write to memory.  */
       memcpy (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);
+	 address.  Note this must be the same address
+	 default_memory_insert_breakpoint worked with.  Calling
+	 gdbarch_breakpoint_from_pc on an already adjusted address is
+	 not idempotent.  E.g., on MIPS targets the ISA bit can be the
+	 only place where the breakpoint type requirement is encoded,
+	 when one sets a breakpoint by address in code that has no
+	 symbol information.  For example:
+
+	   (gdb) break *0x87654321
+
+	 is not the same as:
+
+	   (gdb) break *0x87654320
+
+	 and consequently if there's no symbol information available
+	 for either of 0x87654321 or 0x87654320, then
+	 `mips_breakpoint_from_pc' will return a microMIPS (or MIPS16,
+	 as determined elsewhere) breakpoint for 0x87654321 and a
+	 standard MIPS breakpoint 0x87654320.  */
+      address = bl->address;
+      bp = gdbarch_breakpoint_from_pc (bl->gdbarch, &address, &placed_size);
 
       /* Update the final write buffer with this inserted
 	 breakpoint's INSN.  */
@@ -1655,7 +1674,7 @@ breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
       continue;
 
     one_breakpoint_xfer_memory (readbuf, writebuf, writebuf_org,
-				memaddr, len, &bl->target_info, bl->gdbarch);
+				memaddr, len, bl);
   }
 }
 
-- 
1.9.3

Re: [PATCH] Avoid software breakpoint's instruction shadow inconsistency

[Maciej W. Rozycki]

On Mon, 29 Sep 2014, Pedro Alves wrote:

> >>> This is also important for places
> >>> like `find_single_step_breakpoint' where a breakpoint's address is 
> >>> compared to the raw value of $pc.
> >>>
> >>
> >> AFAICS, insert_single_step_breakpoint also doesn't do
> >> gdbarch_breakpoint_from_pc, so there doesn't seem to be a mismatch.
> > 
> >  The problem is `find_single_step_breakpoint' is called in the ordinary 
> > breakpoint removal path too, so that if a single-step breakpoint is placed 
> > at the same address, it is retained.  I saw this place do bad things in 
> > testing my change before I adjusted it to use `reqstd_address'.
> > 
> >> In any case, find_single_step_breakpoint and insert_single_step_breakpoint
> >> and related deprecated bits are scheduled for deletion
> >> very soon: https://sourceware.org/ml/gdb-patches/2014-09/msg00242.html
> > 
> >  The relevant parts of my change can easily be removed if they do not make 
> > it beforehand.
> 
> The thing is that all that code was only added to get 7.8 out of
> the door and then do things better post 7.8.
> 
> See discussion around: https://sourceware.org/ml/gdb-patches/2014-06/msg00052.html
> 
> So not just the single-step bits; ISTM most of the patch ends up
> unnecessary.
> 
> As soon as breakpoint_xfer_memory doesn't have to care
> about single-step breakpoints, one_breakpoint_xfer_memory can
> work with the location's bl->address instead of
> bl->target_info.placed_address.  bl->address is always the same
> as your bl->target_info.reqstd_address, afaics.  The reason
> one_breakpoint_xfer_memory can't work with bl->address instead today
> is that software single-step breakpoints aren't a location,
> only a target_info, so the original address is lost...

 Exactly, if originating `bl->address' was available, it could be used by 
`gdbarch_breakpoint_from_pc'.

> Piling on workarounds due to these single-step breakpoint issues
> is exactly the sort of thing I'd like to avoid further.
> 
> It's of course also completely unnecessary/bogus for GDB to be
> reinserting breakpoints when the target doesn't handle
> breakpoints itself, but that's another story.

 Agreed, for locally created memory breakpoints it looks horrible to me 
too.  That's not how I'd write this code if designing the feature from the 
beginning rather than retrofitting it.

> I'm confused on this part of your original description:
> 
> >  The issue is `insert_bp_location' overwrites the previously adjusted 
> > value in `placed_address' with the original address, that is only replaced 
> > back with the correct adjusted address later on when 
> > `gdbarch_breakpoint_from_pc' is called.  Meanwhile there's a window where 
> > the value in `placed_address' does not correspond to data stored in 
> > `shadow_contents', leading to incorrect instruction bytes being supplied 
> > when `one_breakpoint_xfer_memory' is called to supply the instruction 
> > overlaid by the breakpoint.
> 
> It doesn't look like to me that this is really the problem, since
> default_memory_insert_breakpoint adjusts bp_tgt->placed_address
> before reading memory.

 Not true (from `mips_breakpoint_from_pc'):

	  insn = mips_fetch_instruction (gdbarch, ISA_MICROMIPS, pc, &status);
	  size = status ? 2
			: mips_insn_size (ISA_MICROMIPS, insn) == 2 ? 2 : 4;
          *pcptr = unmake_compact_addr (pc);

(hmm, weird indentation here, will have to fix) -- as you can see 
`mips_fetch_instruction' (that reads the instruction under `pc') is called 
before the ISA bit is stripped as `pc' is written back to `*pcptr', and 
`pc' has to have the ISA bit set for the reasons I stated in the last 
mail.

 Maybe I could work it around by writing `*pcptr' back first (and still 
calling `mips_fetch_instruction' with the original `pc'), but that looks 
hackish to me; first of all there is no contract in the API between the 
implementation of `gdbarch_breakpoint_from_pc' and its callers that memory 
behind `pcptr' is the address used for breakpoint shadowing.  I think the 
data structures used for shadowing should simply be consistent all the 
time.

> Instead, the issue is that because the breakpoint is supposed to be
> inserted (we're re-inserting it), one_breakpoint_xfer_memory needs
> to store the breakpoint instruction on top of the memory we're
> about to write.  And then one_breakpoint_xfer_memory gets the
> breakpoint instruction wrong exactly because it lost the ISA bit.
> 
> Although we can now see this more easily since we now reinsert
> already inserted breakpoints, we should be able to trigger the
> issue even with that, if the user writes to memory at an address
> where a breakpoint with the ISA bit was inserted.
> 
> Like:
> 
> (gdb) set breakpoint always-inserted on
> (gdb) p /x *(char*) 0x87654321
> $4 = 0x55
> (gdb) break *0x87654321
> (gdb) p /x *(char*) 0x87654321 = 0x55
> $5 = 0x55
> 
> At this point, 0x87654321 should still contain the correct
> breakpoint insn (on the target), but due to the
> one_breakpoint_xfer_memory issue, it won't.
> 
> 
> BTW, if instead of:
> 
> @@ -2543,7 +2543,7 @@ insert_bp_location (struct bp_location *
>       we have a breakpoint inserted at that address and thus
>       read the breakpoint instead of returning the data saved in
>       the breakpoint location's shadow contents.  */
> -  bl->target_info.placed_address = bl->address;
> +  bl->target_info.reqstd_address = bl->address;
> 
> you did:
> 
>    bl->target_info.placed_address = bl->address;
> +  bl->target_info.reqstd_address = bl->address;
> 
> then it seems to me that most of the hunks that do something
> like this:
> 
> @@ -975,7 +975,7 @@ arm_linux_hw_breakpoint_initialize (stru
>  				    struct arm_linux_hw_breakpoint *p)
>  {
>    unsigned mask;
> -  CORE_ADDR address = bp_tgt->placed_address;
> +  CORE_ADDR address = bp_tgt->placed_address = bp_tgt->reqstd_address;
> 
> 
> including the default_memory_insert_breakpoint changes,
> 
> would be unnecessary.

 But as I noted that breaks mips_breakpoint_from_pc, you must not 
overwrite `placed_address' once the instruction shadow has been made.

> I could be missing something else, of course.
> 
> The patch below is what I'd like to push on top of the software single-step
> rework (which I've meanwhile slit and posted here
> https://sourceware.org/ml/gdb-patches/2014-09/msg00755.html)
> 
> I've pushed that series with this patch on top here, for convenience:
> 
>   git@github.com:palves/gdb.git palves/mips_instruction_shadow_inconsistency
> 
> Obviously, the mips-linux-gnu testing mentioned in the log is tentative. :-)

 I'll push it through testing, although given what I wrote above I have 
little hope, so it'll be just a smoke test with a microMIPS multilib and 
the offending test case first.

  Maciej

Re: [PATCH] Avoid software breakpoint's instruction shadow inconsistency

[Maciej W. Rozycki]

On Mon, 29 Sep 2014, Maciej W. Rozycki wrote:

> > I'm confused on this part of your original description:
> > 
> > >  The issue is `insert_bp_location' overwrites the previously adjusted 
> > > value in `placed_address' with the original address, that is only replaced 
> > > back with the correct adjusted address later on when 
> > > `gdbarch_breakpoint_from_pc' is called.  Meanwhile there's a window where 
> > > the value in `placed_address' does not correspond to data stored in 
> > > `shadow_contents', leading to incorrect instruction bytes being supplied 
> > > when `one_breakpoint_xfer_memory' is called to supply the instruction 
> > > overlaid by the breakpoint.
> > 
> > It doesn't look like to me that this is really the problem, since
> > default_memory_insert_breakpoint adjusts bp_tgt->placed_address
> > before reading memory.
> 
>  Not true (from `mips_breakpoint_from_pc'):
> 
> 	  insn = mips_fetch_instruction (gdbarch, ISA_MICROMIPS, pc, &status);
> 	  size = status ? 2
> 			: mips_insn_size (ISA_MICROMIPS, insn) == 2 ? 2 : 4;
>           *pcptr = unmake_compact_addr (pc);
> 
> (hmm, weird indentation here, will have to fix) -- as you can see 
> `mips_fetch_instruction' (that reads the instruction under `pc') is called 
> before the ISA bit is stripped as `pc' is written back to `*pcptr', and 
> `pc' has to have the ISA bit set for the reasons I stated in the last 
> mail.
> 
>  Maybe I could work it around by writing `*pcptr' back first (and still 
> calling `mips_fetch_instruction' with the original `pc'), but that looks 
> hackish to me; first of all there is no contract in the API between the 
> implementation of `gdbarch_breakpoint_from_pc' and its callers that memory 
> behind `pcptr' is the address used for breakpoint shadowing.  I think the 
> data structures used for shadowing should simply be consistent all the 
> time.
> 
> > Instead, the issue is that because the breakpoint is supposed to be
> > inserted (we're re-inserting it), one_breakpoint_xfer_memory needs
> > to store the breakpoint instruction on top of the memory we're
> > about to write.  And then one_breakpoint_xfer_memory gets the
> > breakpoint instruction wrong exactly because it lost the ISA bit.

 As a further note -- as long as symbol information is available the 
instruction used for the breakpoint is always a valid encoding for the ISA 
being used at the breakpoint location even if the ISA bit has been lost, 
because the ISA bit is only used to determine the encoding as the last 
resort.  The ELF `st_other' symbol flags are a preferred way to determine 
the ISA and are available for the majority of failures I see because of 
this defect.  Only cases like software watchpoint tests that single-step 
through possibly stripped system library code will trip on the lost ISA 
bit.

 Of course getting the ISA right does not guarantee the right size of the 
breakpoint instruction, which is the matter of this bug.

> > I could be missing something else, of course.
> > 
> > The patch below is what I'd like to push on top of the software single-step
> > rework (which I've meanwhile slit and posted here
> > https://sourceware.org/ml/gdb-patches/2014-09/msg00755.html)
> > 
> > I've pushed that series with this patch on top here, for convenience:
> > 
> >   git@github.com:palves/gdb.git palves/mips_instruction_shadow_inconsistency
> > 
> > Obviously, the mips-linux-gnu testing mentioned in the log is tentative. :-)
> 
>  I'll push it through testing, although given what I wrote above I have 
> little hope, so it'll be just a smoke test with a microMIPS multilib and 
> the offending test case first.

 So I smoke-tested gdb.base/break.exp that fails horribly with our current 
trunk and the `-EL -mmicromips' multilib and it still fails the same way 
with your tree:

(gdb) PASS: gdb.base/break.exp: run until file:function(1) breakpoint
continue
Continuing.
720

Breakpoint 2, marker2 (a=43) at /scratch/macro/mips-linux/src/gdb-trunk/gdb/testsuite/gdb.base/break1.c:63
63	int marker2 (a) int a; { return (1); }	/* set breakpoint 9 here */
(gdb) PASS: gdb.base/break.exp: run until quoted breakpoint
continue
Continuing.

Program received signal SIGILL, Illegal instruction.
0x0040098d in marker2 (a=43) at /scratch/macro/mips-linux/src/gdb-trunk/gdb/testsuite/gdb.base/break1.c:63
63	int marker2 (a) int a; { return (1); }	/* set breakpoint 9 here */
(gdb) FAIL: gdb.base/break.exp: run until file:linenum breakpoint
break +1
Breakpoint 10 at 0x4009a3: file /scratch/macro/mips-linux/src/gdb-trunk/gdb/testsuite/gdb.base/break1.c, 
line 64.
(gdb) FAIL: gdb.base/break.exp: breakpoint offset +1
step

Program terminated with signal SIGILL, Illegal instruction.
The program no longer exists.
(gdb) FAIL: gdb.base/break.exp: step onto breakpoint

Child terminated with signal = 0x4 (SIGILL)
GDBserver exiting
[...]

 Sorry.  This test script scores "all passed" with my change.

  Maciej

Re: [PATCH] Avoid software breakpoint's instruction shadow inconsistency

[Pedro Alves]

On 09/29/2014 08:11 PM, Maciej W. Rozycki wrote:
> On Mon, 29 Sep 2014, Pedro Alves wrote:

>> It doesn't look like to me that this is really the problem, since
>> default_memory_insert_breakpoint adjusts bp_tgt->placed_address
>> before reading memory.
> 
>  Not true (from `mips_breakpoint_from_pc'):
> 
> 	  insn = mips_fetch_instruction (gdbarch, ISA_MICROMIPS, pc, &status);
> 	  size = status ? 2
> 			: mips_insn_size (ISA_MICROMIPS, insn) == 2 ? 2 : 4;
>           *pcptr = unmake_compact_addr (pc);
> 
> (hmm, weird indentation here, will have to fix) -- as you can see 
> `mips_fetch_instruction' (that reads the instruction under `pc') is called 
> before the ISA bit is stripped as `pc' is written back to `*pcptr', and 
> `pc' has to have the ISA bit set for the reasons I stated in the last 
> mail.

Ah!  That's the part that I was missing.  I see now.

> 
>  Maybe I could work it around by writing `*pcptr' back first (and still 
> calling `mips_fetch_instruction' with the original `pc'), but that looks 
> hackish to me; first of all there is no contract in the API between the 
> implementation of `gdbarch_breakpoint_from_pc' and its callers that memory 
> behind `pcptr' is the address used for breakpoint shadowing.  I think the 
> data structures used for shadowing should simply be consistent all the 
> time.

Agreed.

So, we could fix this by not ever trying to re-insert a memory
breakpoint that has a shadow buffer.  But, if we ever decide
we want to record a shadow buffer for target-managed breakpoint
that ends up reinserted, we'll end up with the same problem again.

So might as well go with your patch.

>> would be unnecessary.
> 
>  But as I noted that breaks mips_breakpoint_from_pc, you must not 
> overwrite `placed_address' once the instruction shadow has been made.
> 

Indeed.

>> I could be missing something else, of course.

That's what I was missing...

Patch is OK.  Please push.

Thanks,
Pedro Alves

Re: [PATCH] Avoid software breakpoint's instruction shadow inconsistency

[Maciej W. Rozycki]

On Mon, 29 Sep 2014, Pedro Alves wrote:

> Patch is OK.  Please push.

 Applied now, thanks.

  Maciej