Avoid software breakpoint's instruction shadow inconsistency

  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
  

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
  

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
  

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