Re: [pushed 1/2] PR gdb/18002: Fix reinsert of a permanent breakpoints

[Yao Qi <qiyaoltc at gmail dot com>]

Pedro Alves <palves@redhat.com> writes:

> Permanent breakpoints
> are always marked as inserted.  So if the permanent breakpoint doesn't
> have a shadow yet in its shadow buffer, but we set shadow_len before
> calling target_read_memory, then the still clear shadow_contents
> buffer will be used by the breakpoint masking code...  And then from
> there on, the permanent breakpoint has a broken shadow buffer, and
> thus any memory read out of that address will read bogus code, and
> many random bad things fall out from that.

Yes, that is what I observed on aarch64-linux too.

>
> The fix is just to set shadow_len at the same time shadow_contents is
> set, not one before and another after...
>
> Fixes all gdb.base/bp-permanent.exp FAILs on PPC64 GNU/Linux gdbserver
> and probably any other gdbserver port that doesn't do z0 breakpoints.

This patch fixes gdb.base/bp-permanent.exp FAILs on aarch64-linux too,
but there are some remains, which are not related.

> diff --git a/gdb/mem-break.c b/gdb/mem-break.c
> index aeffc93..0fb53cf 100644
> --- a/gdb/mem-break.c
> +++ b/gdb/mem-break.c
> @@ -53,12 +53,21 @@ default_memory_insert_breakpoint (struct gdbarch *gdbarch,
>  
>    /* Save the memory contents in the shadow_contents buffer and then
>       write the breakpoint instruction.  */
> -  bp_tgt->shadow_len = bplen;
>    readbuf = alloca (bplen);
>    val = target_read_memory (addr, readbuf, bplen);
>    if (val == 0)
>      {
> +      /* These must be set together, either before or after the shadow
> +	 read, so that if we're "reinserting" a breakpoint that
> +	 doesn't have a shadow yet, the breakpoint masking code inside
> +	 target_read_memory doesn't mask out this breakpoint using an
> +	 unfilled shadow buffer.  The core may be trying to reinsert a
> +	 permanent breakpoint, for targets that support breakpoint
> +	 conditions/commands on the target side for some types of
> +	 breakpoints, such as target remote.  */
> +      bp_tgt->shadow_len = bplen;
>        memcpy (bp_tgt->shadow_contents, readbuf, bplen);
> +

Your fix looks right to me, although I am testing a different one, in
which bp_location_has_shadow returns false if bl->permanent is true.
Anyway, Thanks for fixing this bug, Pedro.

-- 
Yao (éå)