Re: [PATCH for-6.1 v6 11/17] hw/core: Introduce CPUClass.gdb_adjust_breakpoint

[Philippe Mathieu-Daudé]

On 7/20/21 11:53 PM, Philippe Mathieu-Daudé wrote:
> On 7/20/21 11:08 PM, Richard Henderson wrote:
>> On 7/20/21 10:56 AM, Peter Maydell wrote:
>>> On Tue, 20 Jul 2021 at 20:54, Richard Henderson
>>> <richard.henderson@linaro.org> wrote:
>>>>
>>>> This will allow a breakpoint hack to move out of AVR's translator.
>>>>
>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>
>>>> diff --git a/cpu.c b/cpu.c
>>>> index 83059537d7..91d9e38acb 100644
>>>> --- a/cpu.c
>>>> +++ b/cpu.c
>>>> @@ -267,8 +267,13 @@ static void breakpoint_invalidate(CPUState *cpu,
>>>> target_ulong pc)
>>>>   int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
>>>>                             CPUBreakpoint **breakpoint)
>>>>   {
>>>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>>>>       CPUBreakpoint *bp;
>>>>
>>>> +    if (cc->gdb_adjust_breakpoint) {
>>>> +        pc = cc->gdb_adjust_breakpoint(cpu, pc);
>>>> +    }
>>>> +
>>>>       bp = g_malloc(sizeof(*bp));
>>>>
>>>>       bp->pc = pc;
>>>> @@ -294,8 +299,13 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr
>>>> pc, int flags,
>>>>   /* Remove a specific breakpoint.  */
>>>>   int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags)
>>>>   {
>>>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>>>>       CPUBreakpoint *bp;
>>>>
>>>> +    if (cc->gdb_adjust_breakpoint) {
>>>> +        pc = cc->gdb_adjust_breakpoint(cpu, pc);
>>>> +    }
>>>> +
>>>>       QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
>>>>           if (bp->pc == pc && bp->flags == flags) {
>>>>               cpu_breakpoint_remove_by_ref(cpu, bp);
>>>> -- 
>>>
>>> So previously for AVR we would have considered the bp at 0x100
>>> and the one at 0x800100 as distinct (in the sense that the only way
>>> the gdb remote protocol distinguishes breakpoints is by "what address",
>>> and these have different addresses). After this change, they won't
>>> be distinct, because if you set a bp at 0x100 and 0x800100 and then
>>> try to remove the one at 0x100 we might remove the 0x800100 one,
>>> because we're storing only the adjusted-address, not the one gdb used.

Yes. Looks good.

>>>
>>> This might not matter in practice...
>>
>> I don't think it will matter.
>>
>> Currently, if it sets both 0x100 and 0x800100, then we'll record two
>> breakpoints, and with either we'll raise EXCP_DEBUG when pc == 0x100.
>>
>> Afterward, we'll have two CPUBreakpoint structures that both contain
>> 0x100, and when pc == 0x100 we'll raise EXCP_DEBUG.  If gdb removes the
>> breakpoint at 0x800100, we'll remove one of the two CPUBreakpoint.  But
>> we'll still stop at 0x100, as expected.  When it removes the breakpoint
>> at 0x100, both CPUBreakpoint structures will be gone.
>>
>> In principal, gdb could now add a breakpoint at 0x800100 and remove it
>> with 0x100, where it could not before.  But I don't expect that to
>> happen.  If we reported any kind of status to gdb re the breakpoint
>> insertion or removal (e.g. bp not found), then it might matter, but we
>> don't.

IIUC QEMU model is "hardware breakpoint". I don't know how gdb deals
if user add both soft/hard bp. Neither do I know how many soft/hard
bp are "annouced" via gdbstub monitor to gdb (Alex?). Amusingly
gdb-xml/avr-cpu.xml announces itself as a riscv cpu:

<feature name="org.gnu.gdb.riscv.cpu">

Maybe because there is no official XML declaration for AVR?
https://sourceware.org/gdb/current/onlinedocs/gdb/Standard-Target-Features.html#Standard-Target-Features

>> Practically, this is working around what I'd call a gdb bug wrt avr. 
>> Which may even have been fixed -- I haven't looked.

I agree this won't matter much (and this patch makes it cleaner) so:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>