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

[Pedro Alves <palves at redhat dot com>]

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