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.hender...@linaro.org> wrote:
>>>>
>>>> This will allow a breakpoint hack to move out of AVR's translator.
>>>>
>>>> Signed-off-by: Richard Henderson <richard.hender...@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é <f4...@amsat.org>
