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

[Richard Henderson <richard.henderson@linaro.org>]

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.
> 
> 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.

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


r~