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

[Pedro Alves <palves at redhat dot com>]

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