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

Pedro Alves
2014-09-29T20:56:08Z

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