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

[Pedro Alves <palves@redhat.com>]

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