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

["Maciej W. Rozycki" <macro at codesourcery dot com>]

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